[PATCH net-next v2] net: flow_offload: simplify hw stats check handling

[PATCH net-next v2] net: flow_offload: simplify hw stats check handling

[Edward Cree]

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.

Only the kernel's internal API semantics change; the TC uAPI is unaffected.

v2: rebased on net-next, removed RFC tags.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
The discussion on this rather petered out without any consensus, so I'm
 reposting against net-next now that DONT_CARE has appeared there (as it's
 a bit late in the cycle for something so non-critical to go to 'net').

 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 b286fe158820..51e1b3930c56 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 4001ffb04f0d..d58741fcc984 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -168,10 +168,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),
@@ -179,6 +180,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);
@@ -340,11 +342,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 &&

Re: [PATCH net-next v2] net: flow_offload: simplify hw stats check handling

[Pablo Neira Ayuso]

On Tue, May 19, 2020 at 06:02:02PM +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.
> 
> Only the kernel's internal API semantics change; the TC uAPI is unaffected.

This is breaking netfilter again.

Re: [PATCH net-next v2] net: flow_offload: simplify hw stats check handling

[Edward Cree]

On 19/05/2020 18:19, Pablo Neira Ayuso wrote:
> This is breaking netfilter again. 
Still waiting for you to explain what this "breaks".  AFAICT the
 new DONT_CARE has exactly the same effect that the old DONT_CARE
 did, so as long as netfilter is using DONT_CARE rather than (say)
 a hard-coded 0, it should be fine.

Re: [PATCH net-next v2] net: flow_offload: simplify hw stats check handling

[Pablo Neira Ayuso]

On Tue, May 19, 2020 at 06:23:35PM +0100, Edward Cree wrote:
> On 19/05/2020 18:19, Pablo Neira Ayuso wrote:
> > This is breaking netfilter again. 
>
> Still waiting for you to explain what this "breaks".  AFAICT the
> new DONT_CARE has exactly the same effect that the old DONT_CARE
> did, so as long as netfilter is using DONT_CARE rather than (say)
> a hard-coded 0, it should be fine.

Did you test your patch with netfilter? I don't think.

Netfilter is a client of this flow offload API, you have to test that
your core updates do not break any of existing clients.

Please, do not make me think this is intentional.

I am pretty sure your motivation is to help get things better.

Re: [PATCH net-next v2] net: flow_offload: simplify hw stats check handling

[Edward Cree]

On 19/05/2020 18:35, Pablo Neira Ayuso wrote:
> Did you test your patch with netfilter? I don't think.
As I mentioned in v1 (and should have repeated on v2, sorry) this is
 compile tested only, as I don't have the hardware to test it.  (I have
 done some testing with the not-yet-upstream sfc_ef100 driver, though.)
But as I'm not a netfilter user, I don't have a handy set of netfilter
 rules to test this with anyway; and my previous attempts to find useful
 documentation on netfilter.org were not fruitful (although I've just
 now found wiki.nftables.org).  I was hoping someone with the domain
 knowledge (and the hardware) could test this.
(Also, for complicated reasons, getting nft built for my ef100 test
 system would be decidedly nontrivial; and any test I do without offload
 hardware at the bottom would necessarily be so synthetic I'm not sure
 I'd trust it to prove anything.)

> Netfilter is a client of this flow offload API, you have to test that
> your core updates do not break any of existing clients.
Okay, but can we distinguish between "this needs to be tested with
 netfilter before it can be merged" and "this is breaking netfilter"?
Or do you have a specific reason why you think this is broken, beyond
 merely 'it isn't tested'?

Re: [PATCH net-next v2] net: flow_offload: simplify hw stats check handling

[Pablo Neira Ayuso]

On Tue, May 19, 2020 at 07:26:42PM +0100, Edward Cree wrote:
> On 19/05/2020 18:35, Pablo Neira Ayuso wrote:
[...]
> > Netfilter is a client of this flow offload API, you have to test that
> > your core updates do not break any of existing clients.
>
> Okay, but can we distinguish between "this needs to be tested with
>  netfilter before it can be merged" and "this is breaking netfilter"?
> Or do you have a specific reason why you think this is broken, beyond
>  merely 'it isn't tested'?

This breaks netfilter in two ways !

#1 Drivers calling flow_action_hw_stats_check() fall within the
second branch (check_allow_bit is set on).

        } else if (check_allow_bit &&

@@ -340,11 +342,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 &&         <------ HERE

These drivers are not honoring the _DONT_CARE bit,
__flow_action_hw_stats_check() with check_allow_bit set on does not
honor the _DONT_CARE bit.

#2 Your patch needs to update Netfilter to set hw_stats to
   FLOW_ACTION_HW_STATS_DONT_CARE explicitly.

Re: [PATCH net-next v2] net: flow_offload: simplify hw stats check handling

[Edward Cree]

On 20/05/2020 15:33, Pablo Neira Ayuso wrote:
> #1 Drivers calling flow_action_hw_stats_check() fall within the
> second branch (check_allow_bit is set on).
>
>         } else if (check_allow_bit &&         <------ HERE
>
> These drivers are not honoring the _DONT_CARE bit,
> __flow_action_hw_stats_check() with check_allow_bit set on does not
> honor the _DONT_CARE bit.
I don't understand.  There isn't a _DONT_CARE bit; _DONT_CARE isa
 bitmask of *all* the bits: BIT(FLOW_ACTION_HW_STATS_NUM_BITS) - 1.
So if allow_bit < FLOW_ACTION_HW_STATS_NUM_BITS, then
 BIT(allow_bit) & FLOW_ACTION_HW_STATS_DONT_CARE is nonzero, and
 thus the function returns true.

> #2 Your patch needs to update Netfilter to set hw_stats to
>    FLOW_ACTION_HW_STATS_DONT_CARE explicitly.
Ahh, naïvely I had assumed that you would have done that in the
 patch that added _DONT_CARE; I should have checked that.  Will
 fix that for the next version.

Thank you for being specific.
And you'll be pleased to know that I've managed to bodge a working
 nft binary onto my test system, so hopefully I'll be able to test
 with netfilter offload.  Am I right in thinking that an ingress
 chain on the netdev table is the thing I want?

-ed

Re: [PATCH net-next v2] net: flow_offload: simplify hw stats check handling

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Tue, 19 May 2020 19:19:23 +0200

> This is breaking netfilter again.

This is vague and not useful feedback.

Please be specific about how the change breaks netfilter.

What breaks exactly, and how?