| Message ID | 20200306132856.6041-10-jiri@resnulli.us |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: allow user specify TC action HW stats type | expand |
On Fri, 6 Mar 2020 14:28:55 +0100 Jiri Pirko wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Introduce new type for disabled HW stats and allow the value in > mlxsw offload. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > --- > v2->v3: > - moved to bitfield > v1->v2: > - moved to action > --- > drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 +- > include/net/flow_offload.h | 1 + > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > index 4bf3ac1cb20d..88aa554415df 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > @@ -36,7 +36,7 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp, > err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack); > if (err) > return err; > - } else { > + } else if (act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_DISABLED) { > NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type"); > return -EOPNOTSUPP; > } > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > index d597d500a5df..b700c570f7f1 100644 > --- a/include/net/flow_offload.h > +++ b/include/net/flow_offload.h > @@ -159,6 +159,7 @@ enum flow_action_mangle_base { > #define FLOW_ACTION_HW_STATS_TYPE_DELAYED BIT(1) > #define FLOW_ACTION_HW_STATS_TYPE_ANY (FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE | \ > FLOW_ACTION_HW_STATS_TYPE_DELAYED) > +#define FLOW_ACTION_HW_STATS_TYPE_DISABLED 0 Would it fit better for the bitfield if disabled internally is BIT(last type + 1)?
Fri, Mar 06, 2020 at 08:31:16PM CET, kuba@kernel.org wrote: >On Fri, 6 Mar 2020 14:28:55 +0100 Jiri Pirko wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> Introduce new type for disabled HW stats and allow the value in >> mlxsw offload. >> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> --- >> v2->v3: >> - moved to bitfield >> v1->v2: >> - moved to action >> --- >> drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 +- >> include/net/flow_offload.h | 1 + >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c >> index 4bf3ac1cb20d..88aa554415df 100644 >> --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c >> +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c >> @@ -36,7 +36,7 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp, >> err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack); >> if (err) >> return err; >> - } else { >> + } else if (act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_DISABLED) { >> NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type"); >> return -EOPNOTSUPP; >> } >> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h >> index d597d500a5df..b700c570f7f1 100644 >> --- a/include/net/flow_offload.h >> +++ b/include/net/flow_offload.h >> @@ -159,6 +159,7 @@ enum flow_action_mangle_base { >> #define FLOW_ACTION_HW_STATS_TYPE_DELAYED BIT(1) >> #define FLOW_ACTION_HW_STATS_TYPE_ANY (FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE | \ >> FLOW_ACTION_HW_STATS_TYPE_DELAYED) >> +#define FLOW_ACTION_HW_STATS_TYPE_DISABLED 0 > >Would it fit better for the bitfield if disabled internally is >BIT(last type + 1)? I don't see why. Anyway, if needed, this can be always tweaked.
On Sat, 7 Mar 2020 07:56:37 +0100 Jiri Pirko wrote: > >Would it fit better for the bitfield if disabled internally is > >BIT(last type + 1)? > > I don't see why. Anyway, if needed, this can be always tweaked. Because it makes it impossible for drivers to pass to flow_action_hw_stats_types_check() that they support disabled. I thought disabled means "must have no stats" but I think you may want it to mean "no stats needed". Which probably makes more sense, right..
Mon, Mar 09, 2020 at 08:20:14PM CET, kuba@kernel.org wrote: >On Sat, 7 Mar 2020 07:56:37 +0100 Jiri Pirko wrote: >> >Would it fit better for the bitfield if disabled internally is >> >BIT(last type + 1)? >> >> I don't see why. Anyway, if needed, this can be always tweaked. > >Because it makes it impossible for drivers to pass to >flow_action_hw_stats_types_check() that they support disabled. Right, they should not call flow_action_hw_stats_types_check() > >I thought disabled means "must have no stats" but I think you may want >it to mean "no stats needed". Which probably makes more sense, right.. Nope, means "must have no stats"
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c index 4bf3ac1cb20d..88aa554415df 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c @@ -36,7 +36,7 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp, err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack); if (err) return err; - } else { + } else if (act->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_DISABLED) { NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type"); return -EOPNOTSUPP; } diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index d597d500a5df..b700c570f7f1 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -159,6 +159,7 @@ enum flow_action_mangle_base { #define FLOW_ACTION_HW_STATS_TYPE_DELAYED BIT(1) #define FLOW_ACTION_HW_STATS_TYPE_ANY (FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE | \ FLOW_ACTION_HW_STATS_TYPE_DELAYED) +#define FLOW_ACTION_HW_STATS_TYPE_DISABLED 0 typedef void (*action_destr)(void *priv);