Netfilter-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [RFC PATCH net] net: flow_offload: simplify hw stats check handling
@ 2020-05-07 14:59 Edward Cree
  2020-05-07 15:32 ` Pablo Neira Ayuso
  2020-05-07 15:36 ` Pablo Neira Ayuso
  0 siblings, 2 replies; 8+ messages in thread
From: Edward Cree @ 2020-05-07 14:59 UTC (permalink / raw)
  To: netdev; +Cc: davem, netfilter-devel, jiri, kuba, pablo

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(-)

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 &&

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH net] net: flow_offload: simplify hw stats check handling
  2020-05-07 14:59 [RFC PATCH net] net: flow_offload: simplify hw stats check handling Edward Cree
@ 2020-05-07 15:32 ` Pablo Neira Ayuso
  2020-05-07 15:49   ` Edward Cree
  2020-05-07 15:36 ` Pablo Neira Ayuso
  1 sibling, 1 reply; 8+ messages in thread
From: Pablo Neira Ayuso @ 2020-05-07 15:32 UTC (permalink / raw)
  To: Edward Cree; +Cc: netdev, davem, netfilter-devel, jiri, kuba

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH net] net: flow_offload: simplify hw stats check handling
  2020-05-07 14:59 [RFC PATCH net] net: flow_offload: simplify hw stats check handling Edward Cree
  2020-05-07 15:32 ` Pablo Neira Ayuso
@ 2020-05-07 15:36 ` Pablo Neira Ayuso
  1 sibling, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2020-05-07 15:36 UTC (permalink / raw)
  To: Edward Cree; +Cc: netdev, davem, netfilter-devel, jiri, kuba

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH net] net: flow_offload: simplify hw stats check handling
  2020-05-07 15:32 ` Pablo Neira Ayuso
@ 2020-05-07 15:49   ` Edward Cree
  2020-05-07 16:46     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 8+ messages in thread
From: Edward Cree @ 2020-05-07 15:49 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: netdev, davem, netfilter-devel, jiri, kuba

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH net] net: flow_offload: simplify hw stats check handling
  2020-05-07 15:49   ` Edward Cree
@ 2020-05-07 16:46     ` Pablo Neira Ayuso
  2020-05-07 23:48       ` Jakub Kicinski
  0 siblings, 1 reply; 8+ messages in thread
From: Pablo Neira Ayuso @ 2020-05-07 16:46 UTC (permalink / raw)
  To: Edward Cree; +Cc: netdev, davem, netfilter-devel, jiri, kuba

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH net] net: flow_offload: simplify hw stats check handling
  2020-05-07 16:46     ` Pablo Neira Ayuso
@ 2020-05-07 23:48       ` Jakub Kicinski
  2020-05-11  5:33         ` Jiri Pirko
  0 siblings, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2020-05-07 23:48 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Edward Cree, netdev, davem, netfilter-devel, jiri

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH net] net: flow_offload: simplify hw stats check handling
  2020-05-07 23:48       ` Jakub Kicinski
@ 2020-05-11  5:33         ` Jiri Pirko
  2020-05-11 10:09           ` Edward Cree
  0 siblings, 1 reply; 8+ messages in thread
From: Jiri Pirko @ 2020-05-11  5:33 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Pablo Neira Ayuso, Edward Cree, netdev, davem, netfilter-devel

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH net] net: flow_offload: simplify hw stats check handling
  2020-05-11  5:33         ` Jiri Pirko
@ 2020-05-11 10:09           ` Edward Cree
  0 siblings, 0 replies; 8+ messages in thread
From: Edward Cree @ 2020-05-11 10:09 UTC (permalink / raw)
  To: Jiri Pirko, Jakub Kicinski
  Cc: Pablo Neira Ayuso, netdev, davem, netfilter-devel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-07 14:59 [RFC PATCH net] net: flow_offload: simplify hw stats check handling Edward Cree
2020-05-07 15:32 ` Pablo Neira Ayuso
2020-05-07 15:49   ` Edward Cree
2020-05-07 16:46     ` Pablo Neira Ayuso
2020-05-07 23:48       ` Jakub Kicinski
2020-05-11  5:33         ` Jiri Pirko
2020-05-11 10:09           ` Edward Cree
2020-05-07 15:36 ` Pablo Neira Ayuso

Netfilter-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netfilter-devel/0 netfilter-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netfilter-devel netfilter-devel/ https://lore.kernel.org/netfilter-devel \
		netfilter-devel@vger.kernel.org
	public-inbox-index netfilter-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netfilter-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git