| Message ID | 20200505174736.29414-1-pablo@netfilter.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE | expand |
Tue, May 05, 2020 at 07:47:36PM CEST, pablo@netfilter.org wrote: >This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver >that the frontend does not need counters, this hw stats type request >never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests >the driver to disable the stats, however, if the driver cannot disable >counters, it bails out. > >TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_* >except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED >(this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between >TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*. > >Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type") >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> Looks great. Thanks! Reviewed-by: Jiri Pirko <jiri@mellanox.com>
On Tue, 5 May 2020 19:47:36 +0200 Pablo Neira Ayuso wrote: > This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver > that the frontend does not need counters, this hw stats type request > never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests > the driver to disable the stats, however, if the driver cannot disable > counters, it bails out. > > TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_* > except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED > (this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between > TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*. > > Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type") > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > --- > v2: define FLOW_ACTION_HW_STATS_DISABLED at the end of the enumeration > as Jiri suggested. Keep the 1:1 mapping between TCA_ACT_HW_STATS_* > and FLOW_ACTION_HW_STATS_* except by the disabled case. > > include/net/flow_offload.h | 9 ++++++++- > net/sched/cls_api.c | 14 ++++++++++++-- > 2 files changed, 20 insertions(+), 3 deletions(-) > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > index 3619c6acf60f..efc8350b42fb 100644 > --- a/include/net/flow_offload.h > +++ b/include/net/flow_offload.h > @@ -166,15 +166,18 @@ enum flow_action_mangle_base { > enum flow_action_hw_stats_bit { > FLOW_ACTION_HW_STATS_IMMEDIATE_BIT, > FLOW_ACTION_HW_STATS_DELAYED_BIT, > + FLOW_ACTION_HW_STATS_DISABLED_BIT, > }; > > enum flow_action_hw_stats { > - FLOW_ACTION_HW_STATS_DISABLED = 0, > + FLOW_ACTION_HW_STATS_DONT_CARE = 0, Why not ~0? Or ANY | DISABLED? Otherwise you may confuse drivers which check bit by bit. > FLOW_ACTION_HW_STATS_IMMEDIATE = > BIT(FLOW_ACTION_HW_STATS_IMMEDIATE_BIT), > FLOW_ACTION_HW_STATS_DELAYED = BIT(FLOW_ACTION_HW_STATS_DELAYED_BIT), > FLOW_ACTION_HW_STATS_ANY = FLOW_ACTION_HW_STATS_IMMEDIATE | > FLOW_ACTION_HW_STATS_DELAYED, > + FLOW_ACTION_HW_STATS_DISABLED = > + BIT(FLOW_ACTION_HW_STATS_DISABLED_BIT), > }; > > typedef void (*action_destr)(void *priv); > @@ -325,7 +328,11 @@ __flow_action_hw_stats_check(const struct flow_action *action, > return true; > if (!flow_action_mixed_hw_stats_check(action, extack)) > return false; > + > action_entry = flow_action_first_entry_get(action); > + if (action_entry->hw_stats == FLOW_ACTION_HW_STATS_DONT_CARE) > + return true; > + > if (!check_allow_bit && > action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) { > NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\""); > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 55bd1429678f..56cf1b9e1e24 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -3523,6 +3523,16 @@ static void tcf_sample_get_group(struct flow_action_entry *entry, > #endif > } > > +static enum flow_action_hw_stats tc_act_hw_stats(u8 hw_stats) > +{ > + if (WARN_ON_ONCE(hw_stats > TCA_ACT_HW_STATS_ANY)) > + return FLOW_ACTION_HW_STATS_DONT_CARE; > + else if (!hw_stats) > + return FLOW_ACTION_HW_STATS_DISABLED; > + > + return hw_stats; > +} > + > int tc_setup_flow_action(struct flow_action *flow_action, > const struct tcf_exts *exts) > { > @@ -3546,7 +3556,7 @@ int tc_setup_flow_action(struct flow_action *flow_action, > if (err) > goto err_out_locked; > > - entry->hw_stats = act->hw_stats; > + entry->hw_stats = tc_act_hw_stats(act->hw_stats); > > if (is_tcf_gact_ok(act)) { > entry->id = FLOW_ACTION_ACCEPT; > @@ -3614,7 +3624,7 @@ int tc_setup_flow_action(struct flow_action *flow_action, > entry->mangle.mask = tcf_pedit_mask(act, k); > entry->mangle.val = tcf_pedit_val(act, k); > entry->mangle.offset = tcf_pedit_offset(act, k); > - entry->hw_stats = act->hw_stats; > + entry->hw_stats = tc_act_hw_stats(act->hw_stats); > entry = &flow_action->entries[++j]; > } > } else if (is_tcf_csum(act)) {
On Tue, 5 May 2020 20:36:43 +0200 Jiri Pirko wrote: > Tue, May 05, 2020 at 07:47:36PM CEST, pablo@netfilter.org wrote: > >This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver > >that the frontend does not need counters, this hw stats type request > >never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests > >the driver to disable the stats, however, if the driver cannot disable > >counters, it bails out. > > > >TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_* > >except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED > >(this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between > >TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*. > > > >Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type") > >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > Looks great. Thanks! > > Reviewed-by: Jiri Pirko <jiri@mellanox.com> Is this going to "just work" for mlxsw? act = flow_action_first_entry_get(flow_action); if (act->hw_stats == FLOW_ACTION_HW_STATS_ANY || act->hw_stats == FLOW_ACTION_HW_STATS_IMMEDIATE) { /* Count action is inserted first */ err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack); if (err) return err; } else if (act->hw_stats != FLOW_ACTION_HW_STATS_DISABLED) { NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type"); return -EOPNOTSUPP; } if hw_stats is 0 we'll get into the else and bail. That doesn't deliver on the "don't care" promise, no?
On Tue, May 05, 2020 at 11:40:10AM -0700, Jakub Kicinski wrote: > On Tue, 5 May 2020 19:47:36 +0200 Pablo Neira Ayuso wrote: > > This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver > > that the frontend does not need counters, this hw stats type request > > never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests > > the driver to disable the stats, however, if the driver cannot disable > > counters, it bails out. > > > > TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_* > > except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED > > (this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between > > TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*. > > > > Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type") > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > --- > > v2: define FLOW_ACTION_HW_STATS_DISABLED at the end of the enumeration > > as Jiri suggested. Keep the 1:1 mapping between TCA_ACT_HW_STATS_* > > and FLOW_ACTION_HW_STATS_* except by the disabled case. > > > > include/net/flow_offload.h | 9 ++++++++- > > net/sched/cls_api.c | 14 ++++++++++++-- > > 2 files changed, 20 insertions(+), 3 deletions(-) > > > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > > index 3619c6acf60f..efc8350b42fb 100644 > > --- a/include/net/flow_offload.h > > +++ b/include/net/flow_offload.h > > @@ -166,15 +166,18 @@ enum flow_action_mangle_base { > > enum flow_action_hw_stats_bit { > > FLOW_ACTION_HW_STATS_IMMEDIATE_BIT, > > FLOW_ACTION_HW_STATS_DELAYED_BIT, > > + FLOW_ACTION_HW_STATS_DISABLED_BIT, > > }; > > > > enum flow_action_hw_stats { > > - FLOW_ACTION_HW_STATS_DISABLED = 0, > > + FLOW_ACTION_HW_STATS_DONT_CARE = 0, > > Why not ~0? Or ANY | DISABLED? > Otherwise you may confuse drivers which check bit by bit. I'm confused, you agreed with this behaviour: https://lore.kernel.org/netfilter-devel/20200427111220.7b07aae1@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com/T/#m6091486b2b0ddac512fe6c17f5508f280f630b60
On Tue, May 05, 2020 at 11:46:16AM -0700, Jakub Kicinski wrote: > On Tue, 5 May 2020 20:36:43 +0200 Jiri Pirko wrote: > > Tue, May 05, 2020 at 07:47:36PM CEST, pablo@netfilter.org wrote: > > >This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver > > >that the frontend does not need counters, this hw stats type request > > >never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests > > >the driver to disable the stats, however, if the driver cannot disable > > >counters, it bails out. > > > > > >TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_* > > >except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED > > >(this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between > > >TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*. > > > > > >Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type") > > >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > > Looks great. Thanks! > > > > Reviewed-by: Jiri Pirko <jiri@mellanox.com> > > Is this going to "just work" for mlxsw? > > act = flow_action_first_entry_get(flow_action); > if (act->hw_stats == FLOW_ACTION_HW_STATS_ANY || > act->hw_stats == FLOW_ACTION_HW_STATS_IMMEDIATE) { > /* Count action is inserted first */ > err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack); > if (err) > return err; > } else if (act->hw_stats != FLOW_ACTION_HW_STATS_DISABLED) { > NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type"); > return -EOPNOTSUPP; > } > > if hw_stats is 0 we'll get into the else and bail. > > That doesn't deliver on the "don't care" promise, no? I can send a v3 to handle the _DONT_CARE type from the mlxsw. Thank you.
On Tue, 5 May 2020 21:31:45 +0200 Pablo Neira Ayuso wrote: > On Tue, May 05, 2020 at 11:40:10AM -0700, Jakub Kicinski wrote: > > On Tue, 5 May 2020 19:47:36 +0200 Pablo Neira Ayuso wrote: > > > This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver > > > that the frontend does not need counters, this hw stats type request > > > never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests > > > the driver to disable the stats, however, if the driver cannot disable > > > counters, it bails out. > > > > > > TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_* > > > except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED > > > (this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between > > > TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*. > > > > > > Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type") > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > --- > > > v2: define FLOW_ACTION_HW_STATS_DISABLED at the end of the enumeration > > > as Jiri suggested. Keep the 1:1 mapping between TCA_ACT_HW_STATS_* > > > and FLOW_ACTION_HW_STATS_* except by the disabled case. > > > > > > include/net/flow_offload.h | 9 ++++++++- > > > net/sched/cls_api.c | 14 ++++++++++++-- > > > 2 files changed, 20 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > > > index 3619c6acf60f..efc8350b42fb 100644 > > > --- a/include/net/flow_offload.h > > > +++ b/include/net/flow_offload.h > > > @@ -166,15 +166,18 @@ enum flow_action_mangle_base { > > > enum flow_action_hw_stats_bit { > > > FLOW_ACTION_HW_STATS_IMMEDIATE_BIT, > > > FLOW_ACTION_HW_STATS_DELAYED_BIT, > > > + FLOW_ACTION_HW_STATS_DISABLED_BIT, > > > }; > > > > > > enum flow_action_hw_stats { > > > - FLOW_ACTION_HW_STATS_DISABLED = 0, > > > + FLOW_ACTION_HW_STATS_DONT_CARE = 0, > > > > Why not ~0? Or ANY | DISABLED? > > Otherwise you may confuse drivers which check bit by bit. > > I'm confused, you agreed with this behaviour: I was expecting the 0 to be exposed at UAPI level, and then kernel would translate that to a full mask internally. From the other reply: > I can send a v3 to handle the _DONT_CARE type from the mlxsw. Seems a little unnecessary for all drivers to cater to the special case, when we made the argument be a bitfield specifically so that the drivers can function as long as they match on any of the bits.
On Tue, May 05, 2020 at 12:43:43PM -0700, Jakub Kicinski wrote: > On Tue, 5 May 2020 21:31:45 +0200 Pablo Neira Ayuso wrote: > > On Tue, May 05, 2020 at 11:40:10AM -0700, Jakub Kicinski wrote: > > > On Tue, 5 May 2020 19:47:36 +0200 Pablo Neira Ayuso wrote: > > > > This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver > > > > that the frontend does not need counters, this hw stats type request > > > > never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests > > > > the driver to disable the stats, however, if the driver cannot disable > > > > counters, it bails out. > > > > > > > > TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_* > > > > except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED > > > > (this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between > > > > TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*. > > > > > > > > Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type") > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > > --- > > > > v2: define FLOW_ACTION_HW_STATS_DISABLED at the end of the enumeration > > > > as Jiri suggested. Keep the 1:1 mapping between TCA_ACT_HW_STATS_* > > > > and FLOW_ACTION_HW_STATS_* except by the disabled case. > > > > > > > > include/net/flow_offload.h | 9 ++++++++- > > > > net/sched/cls_api.c | 14 ++++++++++++-- > > > > 2 files changed, 20 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > > > > index 3619c6acf60f..efc8350b42fb 100644 > > > > --- a/include/net/flow_offload.h > > > > +++ b/include/net/flow_offload.h > > > > @@ -166,15 +166,18 @@ enum flow_action_mangle_base { > > > > enum flow_action_hw_stats_bit { > > > > FLOW_ACTION_HW_STATS_IMMEDIATE_BIT, > > > > FLOW_ACTION_HW_STATS_DELAYED_BIT, > > > > + FLOW_ACTION_HW_STATS_DISABLED_BIT, > > > > }; > > > > > > > > enum flow_action_hw_stats { > > > > - FLOW_ACTION_HW_STATS_DISABLED = 0, > > > > + FLOW_ACTION_HW_STATS_DONT_CARE = 0, > > > > > > Why not ~0? Or ANY | DISABLED? > > > Otherwise you may confuse drivers which check bit by bit. > > > > I'm confused, you agreed with this behaviour: > > I was expecting the 0 to be exposed at UAPI level, and then kernel > would translate that to a full mask internally. > > From the other reply: > > > I can send a v3 to handle the _DONT_CARE type from the mlxsw. > > Seems a little unnecessary for all drivers to cater to the special > case, when we made the argument be a bitfield specifically so that > the drivers can function as long as they match on any of the bits. Let's summarize semantics: - FLOW_ACTION_HW_STATS_DISABLED means disable counters, bail out if driver cannot disable them. - FLOW_ACTION_HW_STATS_IMMEDIATE means enable inmediate counters, bail out if driver cannot enable inmediate counters. - FLOW_ACTION_HW_STATS_DELAY means enable delayed counters, bail out if driver cannot enable delay counters. - FLOW_ACTION_HW_STATS_ANY means enable counters, either inmmediate or delayed, if driver cannot enable any of them, bail out. - FLOW_ACTION_HW_STATS_DONT_CARE (0) means counters are not needed, never bail out. How can you combine DISABLED and ANY? Look at the semantics above and combine them: this is asking for any counter otherwise bail out and DISABLED is asking for no counters at all, otherwise bail out. This sounds like asking for things that are opposed. So bit A means X, bit B means Y, but if you combine A and B, it means something complete different, say Z? In your proposal, drivers drivers will have to check for ANY | DISABLED for don't care? And what is the semantic for 0 (no bit set) in the kernel in your proposal? Jiri mentioned there will be more bits coming soon. How will you extend this model (all bit set on for DONT_CARE) if new bits with specific semantics are showing up? Combining ANY | DISABLED is non-sense, it should be rejected.
On Tue, 5 May 2020 23:43:21 +0200 Pablo Neira Ayuso wrote: > On Tue, May 05, 2020 at 12:43:43PM -0700, Jakub Kicinski wrote: > > On Tue, 5 May 2020 21:31:45 +0200 Pablo Neira Ayuso wrote: > > > On Tue, May 05, 2020 at 11:40:10AM -0700, Jakub Kicinski wrote: > > > > On Tue, 5 May 2020 19:47:36 +0200 Pablo Neira Ayuso wrote: > > > > > This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver > > > > > that the frontend does not need counters, this hw stats type request > > > > > never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests > > > > > the driver to disable the stats, however, if the driver cannot disable > > > > > counters, it bails out. > > > > > > > > > > TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_* > > > > > except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED > > > > > (this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between > > > > > TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*. > > > > > > > > > > Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type") > > > > > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> > > > > > --- > > > > > v2: define FLOW_ACTION_HW_STATS_DISABLED at the end of the enumeration > > > > > as Jiri suggested. Keep the 1:1 mapping between TCA_ACT_HW_STATS_* > > > > > and FLOW_ACTION_HW_STATS_* except by the disabled case. > > > > > > > > > > include/net/flow_offload.h | 9 ++++++++- > > > > > net/sched/cls_api.c | 14 ++++++++++++-- > > > > > 2 files changed, 20 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h > > > > > index 3619c6acf60f..efc8350b42fb 100644 > > > > > --- a/include/net/flow_offload.h > > > > > +++ b/include/net/flow_offload.h > > > > > @@ -166,15 +166,18 @@ enum flow_action_mangle_base { > > > > > enum flow_action_hw_stats_bit { > > > > > FLOW_ACTION_HW_STATS_IMMEDIATE_BIT, > > > > > FLOW_ACTION_HW_STATS_DELAYED_BIT, > > > > > + FLOW_ACTION_HW_STATS_DISABLED_BIT, > > > > > }; > > > > > > > > > > enum flow_action_hw_stats { > > > > > - FLOW_ACTION_HW_STATS_DISABLED = 0, > > > > > + FLOW_ACTION_HW_STATS_DONT_CARE = 0, > > > > > > > > Why not ~0? Or ANY | DISABLED? > > > > Otherwise you may confuse drivers which check bit by bit. > > > > > > I'm confused, you agreed with this behaviour: > > > > I was expecting the 0 to be exposed at UAPI level, and then kernel > > would translate that to a full mask internally. > > > > From the other reply: > > > > > I can send a v3 to handle the _DONT_CARE type from the mlxsw. > > > > Seems a little unnecessary for all drivers to cater to the special > > case, when we made the argument be a bitfield specifically so that > > the drivers can function as long as they match on any of the bits. > > Let's summarize semantics: > > - FLOW_ACTION_HW_STATS_DISABLED means disable counters, bail out if > driver cannot disable them. > > - FLOW_ACTION_HW_STATS_IMMEDIATE means enable inmediate counters, > bail out if driver cannot enable inmediate counters. > > - FLOW_ACTION_HW_STATS_DELAY means enable delayed counters, bail out > if driver cannot enable delay counters. > > - FLOW_ACTION_HW_STATS_ANY means enable counters, either inmmediate or > delayed, if driver cannot enable any of them, bail out. > > - FLOW_ACTION_HW_STATS_DONT_CARE (0) means counters are not needed, never > bail out. > > How can you combine DISABLED and ANY? Look at the semantics above and > combine them: this is asking for any counter otherwise bail out and > DISABLED is asking for no counters at all, otherwise bail out. > > This sounds like asking for things that are opposed. > > So bit A means X, bit B means Y, but if you combine A and B, it means > something complete different, say Z? > > In your proposal, drivers drivers will have to check for ANY | DISABLED > for don't care? > > And what is the semantic for 0 (no bit set) in the kernel in your > proposal? > > Jiri mentioned there will be more bits coming soon. How will you > extend this model (all bit set on for DONT_CARE) if new bits with > specific semantics are showing up? > > Combining ANY | DISABLED is non-sense, it should be rejected. IIRC we went from the pure bitfield implementation (which was my preference) to one where 0 means disabled. The initial suggestion was to have the hw_stats field on offload mean "user is okay with any of these types". In which case if users don't care about stats at all they should set all the bits, and the driver picks whatever it prefers. Driver would work like so: if (hw_stats & TYPE1) { /* TYPE1 could be disabled */ /* this is the preferred type */ } else if (hw_stats & TYPE2) { /* also support this one */ } else { return ECANTDO; } Unfortunately we ended up with a convoluted API where drivers have to check for magic 0 or 'any' values. I don't really care, we spent too much time talking about this simple feature, anyway. But you should adjust mlxsw, the only driver which actually supports this feature properly, to get a sense of what drivers have to do. Extra if statement times number of drivers - that will start to matter.
Tue, May 05, 2020 at 08:46:16PM CEST, kuba@kernel.org wrote: >On Tue, 5 May 2020 20:36:43 +0200 Jiri Pirko wrote: >> Tue, May 05, 2020 at 07:47:36PM CEST, pablo@netfilter.org wrote: >> >This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver >> >that the frontend does not need counters, this hw stats type request >> >never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests >> >the driver to disable the stats, however, if the driver cannot disable >> >counters, it bails out. >> > >> >TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_* >> >except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED >> >(this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between >> >TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*. >> > >> >Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type") >> >Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> >> >> Looks great. Thanks! >> >> Reviewed-by: Jiri Pirko <jiri@mellanox.com> > >Is this going to "just work" for mlxsw? > > act = flow_action_first_entry_get(flow_action); > if (act->hw_stats == FLOW_ACTION_HW_STATS_ANY || > act->hw_stats == FLOW_ACTION_HW_STATS_IMMEDIATE) { > /* Count action is inserted first */ > err = mlxsw_sp_acl_rulei_act_count(mlxsw_sp, rulei, extack); > if (err) > return err; > } else if (act->hw_stats != FLOW_ACTION_HW_STATS_DISABLED) { > NL_SET_ERR_MSG_MOD(extack, "Unsupported action HW stats type"); > return -EOPNOTSUPP; > } > >if hw_stats is 0 we'll get into the else and bail. > >That doesn't deliver on the "don't care" promise, no? Yeah, we need to handle dontcare there, you are right.
On 06/05/2020 00:29, Jakub Kicinski wrote: > IIRC we went from the pure bitfield implementation (which was my > preference) to one where 0 means disabled. > > Unfortunately we ended up with a convoluted API where drivers have to > check for magic 0 or 'any' values. Yeah, I said something dumb a couple of threads ago and combined the good idea (a DISABLED bit) with the bad idea (0 as magic DONT_CARE value), sorry for leading Pablo on a bit of a wild goose chase there. (It has some slightly nice properties if you're trying to write out-of- tree drivers that work with multiple kernel versions, but that's never a good argument for anything, especially when it requires a bunch of extra code in the in-tree drivers to handle it.) > On Tue, 5 May 2020 23:43:21 +0200 Pablo Neira Ayuso wrote: >> And what is the semantic for 0 (no bit set) in the kernel in your >> proposal? It's illegal, the kernel never does it, and if it ever does then the correct response from drivers is to say "None of the things I can support (including DISABLED) were in the bitmask, so -EOPNOTSUPP". Which is what drivers written in the natural way will do, for free. >> Jiri mentioned there will be more bits coming soon. How will you >> extend this model (all bit set on for DONT_CARE) if new bits with >> specific semantics are showing up? If those bits are additive (e.g. a new type like IMMEDIATE and DISABLED), then all-bits-on works fine. If they're orthogonal flags, ideally there should be two bits, one for "flag OFF is acceptable" and one for "flag ON is acceptable", that way 0b11 still means don't care. And 0b00 gets EOPNOTSUPP regardless of the rest of the bits. >> Combining ANY | DISABLED is non-sense, it should be rejected. It's not nonsense; it means what it says ("I accept any of the modes (which enable stats); I also accept disabled stats"). -ed
Wed, May 06, 2020 at 01:33:27PM CEST, ecree@solarflare.com wrote: >On 06/05/2020 00:29, Jakub Kicinski wrote: >> IIRC we went from the pure bitfield implementation (which was my >> preference) to one where 0 means disabled. >> >> Unfortunately we ended up with a convoluted API where drivers have to >> check for magic 0 or 'any' values. >Yeah, I said something dumb a couple of threads ago and combined the > good idea (a DISABLED bit) with the bad idea (0 as magic DONT_CARE > value), sorry for leading Pablo on a bit of a wild goose chase there. >(It has some slightly nice properties if you're trying to write out-of- > tree drivers that work with multiple kernel versions, but that's never > a good argument for anything, especially when it requires a bunch of > extra code in the in-tree drivers to handle it.) > >> On Tue, 5 May 2020 23:43:21 +0200 Pablo Neira Ayuso wrote: >>> And what is the semantic for 0 (no bit set) in the kernel in your >>> proposal? >It's illegal, the kernel never does it, and if it ever does then the > correct response from drivers is to say "None of the things I can > support (including DISABLED) were in the bitmask, so -EOPNOTSUPP". >Which is what drivers written in the natural way will do, for free. > >>> Jiri mentioned there will be more bits coming soon. How will you >>> extend this model (all bit set on for DONT_CARE) if new bits with >>> specific semantics are showing up? >If those bits are additive (e.g. a new type like IMMEDIATE and They are additive. > DISABLED), then all-bits-on works fine. If they're orthogonal flags, > ideally there should be two bits, one for "flag OFF is acceptable" > and one for "flag ON is acceptable", that way 0b11 still means don't > care. And 0b00 gets EOPNOTSUPP regardless of the rest of the bits. > >>> Combining ANY | DISABLED is non-sense, it should be rejected. >It's not nonsense; it means what it says ("I accept any of the modes > (which enable stats); I also accept disabled stats"). > >-ed
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h index 3619c6acf60f..efc8350b42fb 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -166,15 +166,18 @@ enum flow_action_mangle_base { enum flow_action_hw_stats_bit { FLOW_ACTION_HW_STATS_IMMEDIATE_BIT, FLOW_ACTION_HW_STATS_DELAYED_BIT, + FLOW_ACTION_HW_STATS_DISABLED_BIT, }; enum flow_action_hw_stats { - FLOW_ACTION_HW_STATS_DISABLED = 0, + FLOW_ACTION_HW_STATS_DONT_CARE = 0, FLOW_ACTION_HW_STATS_IMMEDIATE = BIT(FLOW_ACTION_HW_STATS_IMMEDIATE_BIT), FLOW_ACTION_HW_STATS_DELAYED = BIT(FLOW_ACTION_HW_STATS_DELAYED_BIT), FLOW_ACTION_HW_STATS_ANY = FLOW_ACTION_HW_STATS_IMMEDIATE | FLOW_ACTION_HW_STATS_DELAYED, + FLOW_ACTION_HW_STATS_DISABLED = + BIT(FLOW_ACTION_HW_STATS_DISABLED_BIT), }; typedef void (*action_destr)(void *priv); @@ -325,7 +328,11 @@ __flow_action_hw_stats_check(const struct flow_action *action, return true; if (!flow_action_mixed_hw_stats_check(action, extack)) return false; + action_entry = flow_action_first_entry_get(action); + if (action_entry->hw_stats == FLOW_ACTION_HW_STATS_DONT_CARE) + return true; + if (!check_allow_bit && action_entry->hw_stats != FLOW_ACTION_HW_STATS_ANY) { NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\""); diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 55bd1429678f..56cf1b9e1e24 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -3523,6 +3523,16 @@ static void tcf_sample_get_group(struct flow_action_entry *entry, #endif } +static enum flow_action_hw_stats tc_act_hw_stats(u8 hw_stats) +{ + if (WARN_ON_ONCE(hw_stats > TCA_ACT_HW_STATS_ANY)) + return FLOW_ACTION_HW_STATS_DONT_CARE; + else if (!hw_stats) + return FLOW_ACTION_HW_STATS_DISABLED; + + return hw_stats; +} + int tc_setup_flow_action(struct flow_action *flow_action, const struct tcf_exts *exts) { @@ -3546,7 +3556,7 @@ int tc_setup_flow_action(struct flow_action *flow_action, if (err) goto err_out_locked; - entry->hw_stats = act->hw_stats; + entry->hw_stats = tc_act_hw_stats(act->hw_stats); if (is_tcf_gact_ok(act)) { entry->id = FLOW_ACTION_ACCEPT; @@ -3614,7 +3624,7 @@ int tc_setup_flow_action(struct flow_action *flow_action, entry->mangle.mask = tcf_pedit_mask(act, k); entry->mangle.val = tcf_pedit_val(act, k); entry->mangle.offset = tcf_pedit_offset(act, k); - entry->hw_stats = act->hw_stats; + entry->hw_stats = tc_act_hw_stats(act->hw_stats); entry = &flow_action->entries[++j]; } } else if (is_tcf_csum(act)) {
This patch adds FLOW_ACTION_HW_STATS_DONT_CARE which tells the driver that the frontend does not need counters, this hw stats type request never fails. The FLOW_ACTION_HW_STATS_DISABLED type explicitly requests the driver to disable the stats, however, if the driver cannot disable counters, it bails out. TCA_ACT_HW_STATS_* maintains the 1:1 mapping with FLOW_ACTION_HW_STATS_* except by disabled which is mapped to FLOW_ACTION_HW_STATS_DISABLED (this is 0 in tc). Add tc_act_hw_stats() to perform the mapping between TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_*. Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type") Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- v2: define FLOW_ACTION_HW_STATS_DISABLED at the end of the enumeration as Jiri suggested. Keep the 1:1 mapping between TCA_ACT_HW_STATS_* and FLOW_ACTION_HW_STATS_* except by the disabled case. include/net/flow_offload.h | 9 ++++++++- net/sched/cls_api.c | 14 ++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-)