[PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Pablo Neira Ayuso]

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,
 	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)) {
-- 
2.20.1

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Jiri Pirko]

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>

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Jakub Kicinski]

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?

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Pablo Neira Ayuso]

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.

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Jiri Pirko]

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.

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Jakub Kicinski]

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

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Pablo Neira Ayuso]

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

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Jakub Kicinski]

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.

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Pablo Neira Ayuso]

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.

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Jakub Kicinski]

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.

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Edward Cree]

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

Re: [PATCH net,v2] net: flow_offload: skip hw stats check for FLOW_ACTION_HW_STATS_DONT_CARE

[Jiri Pirko]

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