| Message ID | 49176c41-3696-86d9-f0eb-c20207cd6d23@solarflare.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | [RFC,net] net: flow_offload: simplify hw stats check handling | expand |
On Thu, May 07, 2020 at 03:59:09PM +0100, Edward Cree wrote: > Make FLOW_ACTION_HW_STATS_DONT_CARE be all bits, rather than none, so that > drivers and __flow_action_hw_stats_check can use simple bitwise checks. You have have to explain why this makes sense in terms of semantics. _DISABLED and _ANY are contradicting each other. > In mlxsw we check for DISABLED first, because we'd rather save the counter > resources in the DONT_CARE case. And this also is breaking netfilter again. > Signed-off-by: Edward Cree <ecree@solarflare.com> > --- > Compile tested only. > > drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 8 ++++---- > include/net/flow_offload.h | 8 ++++---- Turning DONT_CARE gives us nothing back at all.
On Thu, May 07, 2020 at 03:59:09PM +0100, Edward Cree wrote: [...] > diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > index 890b078851c9..1f0caeae24e1 100644 > --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c > @@ -30,14 +30,14 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp, > return -EOPNOTSUPP; > > 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) { > + if (act->hw_stats & FLOW_ACTION_HW_STATS_DISABLED) { > + /* Nothing to do */ What if the driver does not support to disable counters? It will have to check for _DONT_CARE here. And _DISABLED implies "bail out if you cannot disable". You cannot assume _DISABLE != _DONT_CARE, it's the driver that decides this.
On 07/05/2020 16:32, Pablo Neira Ayuso wrote: > On Thu, May 07, 2020 at 03:59:09PM +0100, Edward Cree wrote: >> Make FLOW_ACTION_HW_STATS_DONT_CARE be all bits, rather than none, so that >> drivers and __flow_action_hw_stats_check can use simple bitwise checks. > > You have have to explain why this makes sense in terms of semantics. > > _DISABLED and _ANY are contradicting each other. No, they aren't. The DISABLED bit means "I will accept disabled", it doesn't mean "I insist on disabled". What _does_ mean "I insist on disabled" is if the DISABLED bit is set and no other bits are. So DISABLED | ANY means "I accept disabled; I also accept immediate or delayed". A.k.a. "I don't care, do what you like". >> In mlxsw we check for DISABLED first, because we'd rather save the counter >> resources in the DONT_CARE case. > > And this also is breaking netfilter again. > > Turning DONT_CARE gives us nothing back at all. If you set DONT_CARE, then because that includes the DISABLED bit, you will get no counter on mlxsw. I thought that was what netfilter wanted (no counters by default)? On 07/05/2020 16:36, Pablo Neira Ayuso wrote: > What if the driver does not support to disable counters? > > It will have to check for _DONT_CARE here. No, it would just go if (hw_stats & _IMMEDIATE) { configure_me_a_counter(); } else { error("Only hw_stats_type immediate supported"); } And this will work fine, because _DONT_CARE & _IMMEDIATE == _IMMEDIATE, whereas _DISABLED & _IMMEDIATE == 0. > And _DISABLED implies "bail out if you cannot disable". See above; with the new semantics, the "bail out" condition is "if you cannot satisfy any of the bits that were set". Which means if _DISABLED is the only bit set, and you cannot disable, you bail out; but if _DISABLED and (say) _IMMEDIATE are both set, that means "bail out if you don't support _IMMEDIATE *and* cannot disable" (i.e. if you only support _DELAYED). -ed
On Thu, May 07, 2020 at 04:49:15PM +0100, Edward Cree wrote: > On 07/05/2020 16:32, Pablo Neira Ayuso wrote: > > On Thu, May 07, 2020 at 03:59:09PM +0100, Edward Cree wrote: > >> Make FLOW_ACTION_HW_STATS_DONT_CARE be all bits, rather than none, so that > >> drivers and __flow_action_hw_stats_check can use simple bitwise checks. > > > > You have have to explain why this makes sense in terms of semantics. > > > > _DISABLED and _ANY are contradicting each other. > No, they aren't. The DISABLED bit means "I will accept disabled", it doesn't > mean "I insist on disabled". What _does_ mean "I insist on disabled" is if > the DISABLED bit is set and no other bits are. > So DISABLED | ANY means "I accept disabled; I also accept immediate or > delayed". A.k.a. "I don't care, do what you like". Jiri said Disabled means: bail out if you cannot disable it. If the driver cannot disable, then it will have to check if the frontend is asking for Disabled (hence, report error to the frontend) or if it is actually asking for Don't care. What you propose is a context-based interpretation of the bits. So semantics depend on how you accumulate/combine bits. I really think bits semantics should be interpreted on the bit alone itself. There is one exception though, that is _ANY case, where you let the driver pick between delayed or immediate. But if the driver does not support for counters, it bails out in any case, so the outcome in both request is basically the same. You are asking for different outcome depending on how bits are combined, which can be done, but it sounds innecessarily complicated to me.
On Thu, 7 May 2020 18:46:43 +0200 Pablo Neira Ayuso wrote: > On Thu, May 07, 2020 at 04:49:15PM +0100, Edward Cree wrote: > > On 07/05/2020 16:32, Pablo Neira Ayuso wrote: > > > On Thu, May 07, 2020 at 03:59:09PM +0100, Edward Cree wrote: > > >> Make FLOW_ACTION_HW_STATS_DONT_CARE be all bits, rather than none, so that > > >> drivers and __flow_action_hw_stats_check can use simple bitwise checks. > > > > > > You have have to explain why this makes sense in terms of semantics. > > > > > > _DISABLED and _ANY are contradicting each other. > > No, they aren't. The DISABLED bit means "I will accept disabled", it doesn't > > mean "I insist on disabled". What _does_ mean "I insist on disabled" is if > > the DISABLED bit is set and no other bits are. > > So DISABLED | ANY means "I accept disabled; I also accept immediate or > > delayed". A.k.a. "I don't care, do what you like". > > Jiri said Disabled means: bail out if you cannot disable it. That's in TC uAPI Jiri chose... doesn't mean we have to do the same internally. > If the driver cannot disable, then it will have to check if the > frontend is asking for Disabled (hence, report error to the frontend) > or if it is actually asking for Don't care. > > What you propose is a context-based interpretation of the bits. So > semantics depend on how you accumulate/combine bits. > > I really think bits semantics should be interpreted on the bit alone > itself. These 3 paragraphs sound to me like you were arguing for Ed's approach.. > There is one exception though, that is _ANY case, where you let the > driver pick between delayed or immediate. But if the driver does not > support for counters, it bails out in any case, so the outcome in both > request is basically the same. > > You are asking for different outcome depending on how bits are > combined, which can be done, but it sounds innecessarily complicated > to me. No, quite the opposite, the code as committed to net has magic values which drivers have to check. The counter-proposal is that each bit represents a configuration, and if more than one bit is set the driver gets to choose which it prefers. What could be simpler? netfilter just has to explicitly set the field to DONT_CARE rather than depending on 0 form zalloc() coinciding with the correct value.
Fri, May 08, 2020 at 01:48:20AM CEST, kuba@kernel.org wrote: >On Thu, 7 May 2020 18:46:43 +0200 Pablo Neira Ayuso wrote: >> On Thu, May 07, 2020 at 04:49:15PM +0100, Edward Cree wrote: >> > On 07/05/2020 16:32, Pablo Neira Ayuso wrote: >> > > On Thu, May 07, 2020 at 03:59:09PM +0100, Edward Cree wrote: >> > >> Make FLOW_ACTION_HW_STATS_DONT_CARE be all bits, rather than none, so that >> > >> drivers and __flow_action_hw_stats_check can use simple bitwise checks. >> > > >> > > You have have to explain why this makes sense in terms of semantics. >> > > >> > > _DISABLED and _ANY are contradicting each other. >> > No, they aren't. The DISABLED bit means "I will accept disabled", it doesn't >> > mean "I insist on disabled". What _does_ mean "I insist on disabled" is if >> > the DISABLED bit is set and no other bits are. >> > So DISABLED | ANY means "I accept disabled; I also accept immediate or >> > delayed". A.k.a. "I don't care, do what you like". >> >> Jiri said Disabled means: bail out if you cannot disable it. > >That's in TC uAPI Jiri chose... doesn't mean we have to do the same >internally. Yeah, but if TC user says "disabled", please don't assign counter or fail. > >> If the driver cannot disable, then it will have to check if the >> frontend is asking for Disabled (hence, report error to the frontend) >> or if it is actually asking for Don't care. >> >> What you propose is a context-based interpretation of the bits. So >> semantics depend on how you accumulate/combine bits. >> >> I really think bits semantics should be interpreted on the bit alone >> itself. > >These 3 paragraphs sound to me like you were arguing for Ed's approach.. > >> There is one exception though, that is _ANY case, where you let the >> driver pick between delayed or immediate. But if the driver does not >> support for counters, it bails out in any case, so the outcome in both >> request is basically the same. >> >> You are asking for different outcome depending on how bits are >> combined, which can be done, but it sounds innecessarily complicated >> to me. > >No, quite the opposite, the code as committed to net has magic values >which drivers have to check. > >The counter-proposal is that each bit represents a configuration, and >if more than one bit is set the driver gets to choose which it prefers. >What could be simpler? > >netfilter just has to explicitly set the field to DONT_CARE rather than >depending on 0 form zalloc() coinciding with the correct value.
On 11/05/2020 06:33, Jiri Pirko wrote: > Fri, May 08, 2020 at 01:48:20AM CEST, kuba@kernel.org wrote: >> On Thu, 7 May 2020 18:46:43 +0200 Pablo Neira Ayuso wrote: >>> Jiri said Disabled means: bail out if you cannot disable it. >> That's in TC uAPI Jiri chose... doesn't mean we have to do the same >> internally. > Yeah, but if TC user says "disabled", please don't assign counter or > fail. Right, that's what happens with my proposal: TC "disabled" gets mapped to internal "disabled (and no other bits)", which means "disable or fail". In exactly the same way that TC "immediate" gets mapped to internal "immediate (and no other bits)" which means "immediate or fail". As Jakub says, "What could be simpler?" -ed
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c index 890b078851c9..1f0caeae24e1 100644 --- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c +++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c @@ -30,14 +30,14 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp, return -EOPNOTSUPP; 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) { + if (act->hw_stats & FLOW_ACTION_HW_STATS_DISABLED) { + /* Nothing to do */ + } else if (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 && - act->hw_stats != FLOW_ACTION_HW_STATS_DONT_CARE) { + } else { 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 efc8350b42fb..203dd54a095a 100644 --- a/include/net/flow_offload.h +++ b/include/net/flow_offload.h @@ -167,10 +167,11 @@ enum flow_action_hw_stats_bit { FLOW_ACTION_HW_STATS_IMMEDIATE_BIT, FLOW_ACTION_HW_STATS_DELAYED_BIT, FLOW_ACTION_HW_STATS_DISABLED_BIT, + + FLOW_ACTION_HW_STATS_NUM_BITS }; enum flow_action_hw_stats { - 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), @@ -178,6 +179,7 @@ enum flow_action_hw_stats { FLOW_ACTION_HW_STATS_DELAYED, FLOW_ACTION_HW_STATS_DISABLED = BIT(FLOW_ACTION_HW_STATS_DISABLED_BIT), + FLOW_ACTION_HW_STATS_DONT_CARE = BIT(FLOW_ACTION_HW_STATS_NUM_BITS) - 1, }; typedef void (*action_destr)(void *priv); @@ -330,11 +332,9 @@ __flow_action_hw_stats_check(const struct flow_action *action, 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) { + ~action_entry->hw_stats & FLOW_ACTION_HW_STATS_ANY) { NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\""); return false; } else if (check_allow_bit &&
Make FLOW_ACTION_HW_STATS_DONT_CARE be all bits, rather than none, so that drivers and __flow_action_hw_stats_check can use simple bitwise checks. In mlxsw we check for DISABLED first, because we'd rather save the counter resources in the DONT_CARE case. Signed-off-by: Edward Cree <ecree@solarflare.com> --- Compile tested only. drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 8 ++++---- include/net/flow_offload.h | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-)