[patch net-next 0/3] flow_offload: follow-ups to HW stats type patchset

[patch net-next 0/3] flow_offload: follow-ups to HW stats type patchset

[Jiri Pirko]

This patchset includes couple of patches in reaction to the discussions
to the original HW stats patchset. The first patch is a fix,
the other two patches are basically cosmetics.

Jiri Pirko (3):
  flow_offload: fix allowed types check
  flow_offload: turn hw_stats_type into dedicated enum
  flow_offload: restrict driver to pass one allowed bit to
    flow_action_hw_stats_types_check()

 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  4 +-
 include/net/flow_offload.h                    | 46 +++++++++++++------
 2 files changed, 35 insertions(+), 15 deletions(-)

-- 
2.21.1

[patch net-next 1/3] flow_offload: fix allowed types check

[Jiri Pirko]

Change the check to see if the passed allowed type bit is enabled.

Fixes: 319a1d19471e ("flow_offload: check for basic action hw stats type")
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 include/net/flow_offload.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 891e15055708..2fda4178ba35 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -306,7 +306,7 @@ flow_action_hw_stats_types_check(const struct flow_action *action,
 		NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
 		return false;
 	} else if (allowed_hw_stats_type != 0 &&
-		   action_entry->hw_stats_type != allowed_hw_stats_type) {
+		   !(action_entry->hw_stats_type & allowed_hw_stats_type)) {
 		NL_SET_ERR_MSG_MOD(extack, "Driver does not support selected HW stats type");
 		return false;
 	}
-- 
2.21.1

[patch net-next 2/3] flow_offload: turn hw_stats_type into dedicated enum

[Jiri Pirko]

Put the values into enum and add an enum to define the bits.

Suggested-by: Edward Cree <ecree@solarflare.com>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 include/net/flow_offload.h | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 2fda4178ba35..6849cb5d4883 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -155,11 +155,21 @@ enum flow_action_mangle_base {
 	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
 };
 
-#define FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE BIT(0)
-#define FLOW_ACTION_HW_STATS_TYPE_DELAYED BIT(1)
-#define FLOW_ACTION_HW_STATS_TYPE_ANY (FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE | \
-				       FLOW_ACTION_HW_STATS_TYPE_DELAYED)
-#define FLOW_ACTION_HW_STATS_TYPE_DISABLED 0
+enum flow_action_hw_stats_type_bit {
+	FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE_BIT,
+	FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT,
+};
+
+enum flow_action_hw_stats_type {
+	FLOW_ACTION_HW_STATS_TYPE_DISABLED = 0,
+	FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE =
+		BIT(FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE_BIT),
+	FLOW_ACTION_HW_STATS_TYPE_DELAYED =
+		BIT(FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT),
+	FLOW_ACTION_HW_STATS_TYPE_ANY =
+		FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE |
+		FLOW_ACTION_HW_STATS_TYPE_DELAYED,
+};
 
 typedef void (*action_destr)(void *priv);
 
@@ -175,7 +185,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
 
 struct flow_action_entry {
 	enum flow_action_id		id;
-	u8				hw_stats_type;
+	enum flow_action_hw_stats_type	hw_stats_type;
 	action_destr			destructor;
 	void				*destructor_priv;
 	union {
-- 
2.21.1

[patch net-next 3/3] flow_offload: restrict driver to pass one allowed bit to flow_action_hw_stats_types_check()

[Jiri Pirko]

The intention of this helper was to allow driver to specify one type
that it supports, so not only "any" value would pass. So make the API
more strict and allow driver to pass only 1 bit that is going
to be checked.

Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
 .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  4 ++--
 include/net/flow_offload.h                    | 24 +++++++++++++------
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 33d3e70418fb..f285713def77 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -2879,7 +2879,7 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
 		return -EINVAL;
 
 	if (!flow_action_hw_stats_types_check(flow_action, extack,
-					      FLOW_ACTION_HW_STATS_TYPE_DELAYED))
+					      FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT))
 		return -EOPNOTSUPP;
 
 	attr->flow_tag = MLX5_FS_DEFAULT_FLOW_TAG;
@@ -3374,7 +3374,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
 		return -EINVAL;
 
 	if (!flow_action_hw_stats_types_check(flow_action, extack,
-					      FLOW_ACTION_HW_STATS_TYPE_DELAYED))
+					      FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT))
 		return -EOPNOTSUPP;
 
 	flow_action_for_each(i, act, flow_action) {
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 6849cb5d4883..d1b1e4aa310a 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -300,9 +300,10 @@ flow_action_first_entry_get(const struct flow_action *action)
 }
 
 static inline bool
-flow_action_hw_stats_types_check(const struct flow_action *action,
-				 struct netlink_ext_ack *extack,
-				 u8 allowed_hw_stats_type)
+__flow_action_hw_stats_types_check(const struct flow_action *action,
+				   struct netlink_ext_ack *extack,
+				   bool check_allow_bit,
+				   enum flow_action_hw_stats_type_bit allow_bit)
 {
 	const struct flow_action_entry *action_entry;
 
@@ -311,23 +312,32 @@ flow_action_hw_stats_types_check(const struct flow_action *action,
 	if (!flow_action_mixed_hw_stats_types_check(action, extack))
 		return false;
 	action_entry = flow_action_first_entry_get(action);
-	if (allowed_hw_stats_type == 0 &&
+	if (!check_allow_bit &&
 	    action_entry->hw_stats_type != FLOW_ACTION_HW_STATS_TYPE_ANY) {
 		NL_SET_ERR_MSG_MOD(extack, "Driver supports only default HW stats type \"any\"");
 		return false;
-	} else if (allowed_hw_stats_type != 0 &&
-		   !(action_entry->hw_stats_type & allowed_hw_stats_type)) {
+	} else if (check_allow_bit &&
+		   !(action_entry->hw_stats_type & BIT(allow_bit))) {
 		NL_SET_ERR_MSG_MOD(extack, "Driver does not support selected HW stats type");
 		return false;
 	}
 	return true;
 }
 
+static inline bool
+flow_action_hw_stats_types_check(const struct flow_action *action,
+				 struct netlink_ext_ack *extack,
+				 enum flow_action_hw_stats_type_bit allow_bit)
+{
+	return __flow_action_hw_stats_types_check(action, extack,
+						  true, allow_bit);
+}
+
 static inline bool
 flow_action_basic_hw_stats_types_check(const struct flow_action *action,
 				       struct netlink_ext_ack *extack)
 {
-	return flow_action_hw_stats_types_check(action, extack, 0);
+	return __flow_action_hw_stats_types_check(action, extack, false, 0);
 }
 
 struct flow_rule {
-- 
2.21.1

Re: [patch net-next 0/3] flow_offload: follow-ups to HW stats type patchset

[Edward Cree]

On 10/03/2020 15:49, Jiri Pirko wrote:
> This patchset includes couple of patches in reaction to the discussions
> to the original HW stats patchset. The first patch is a fix,
> the other two patches are basically cosmetics.
>
> Jiri Pirko (3):
>   flow_offload: fix allowed types check
>   flow_offload: turn hw_stats_type into dedicated enum
>   flow_offload: restrict driver to pass one allowed bit to
>     flow_action_hw_stats_types_check()
>
>  .../net/ethernet/mellanox/mlx5/core/en_tc.c   |  4 +-
>  include/net/flow_offload.h                    | 46 +++++++++++++------
>  2 files changed, 35 insertions(+), 15 deletions(-)
>
Acked-by: Edward Cree <ecree@solarflare.com>

Re: [patch net-next 0/3] flow_offload: follow-ups to HW stats type patchset

[Jakub Kicinski]

On Tue, 10 Mar 2020 16:49:06 +0100 Jiri Pirko wrote:
> This patchset includes couple of patches in reaction to the discussions
> to the original HW stats patchset. The first patch is a fix,
> the other two patches are basically cosmetics.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

This problem already exists, but writing a patch for nfp I noticed that
there is no way for this:

	if (!flow_action_hw_stats_types_check(flow_action, extack,
					      FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT))
		return -EOPNOTSUPP;

to fit on a line for either bit, which kind of sucks.

I may send a rename...

Re: [patch net-next 0/3] flow_offload: follow-ups to HW stats type patchset

[Jiri Pirko]

Tue, Mar 10, 2020 at 08:05:19PM CET, kuba@kernel.org wrote:
>On Tue, 10 Mar 2020 16:49:06 +0100 Jiri Pirko wrote:
>> This patchset includes couple of patches in reaction to the discussions
>> to the original HW stats patchset. The first patch is a fix,
>> the other two patches are basically cosmetics.
>
>Reviewed-by: Jakub Kicinski <kuba@kernel.org>
>
>This problem already exists, but writing a patch for nfp I noticed that
>there is no way for this:
>
>	if (!flow_action_hw_stats_types_check(flow_action, extack,
>					      FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT))
>		return -EOPNOTSUPP;
>
>to fit on a line for either bit, which kind of sucks.

Yeah, I was thinking about having flow_action_hw_stats_types_check as a
macro and then just simply have:

	if (!flow_action_hw_stats_types_check(flow_action, extack, DELAYED))
		return -EOPNOTSUPP;

WDYT?


>
>I may send a rename...

Re: [patch net-next 0/3] flow_offload: follow-ups to HW stats type patchset

[Jakub Kicinski]

On Wed, 11 Mar 2020 08:19:55 +0100 Jiri Pirko wrote:
> Tue, Mar 10, 2020 at 08:05:19PM CET, kuba@kernel.org wrote:
> >On Tue, 10 Mar 2020 16:49:06 +0100 Jiri Pirko wrote:  
> >> This patchset includes couple of patches in reaction to the discussions
> >> to the original HW stats patchset. The first patch is a fix,
> >> the other two patches are basically cosmetics.  
> >
> >Reviewed-by: Jakub Kicinski <kuba@kernel.org>
> >
> >This problem already exists, but writing a patch for nfp I noticed that
> >there is no way for this:
> >
> >	if (!flow_action_hw_stats_types_check(flow_action, extack,
> >					      FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT))
> >		return -EOPNOTSUPP;
> >
> >to fit on a line for either bit, which kind of sucks.  
> 
> Yeah, I was thinking about having flow_action_hw_stats_types_check as a
> macro and then just simply have:
> 
> 	if (!flow_action_hw_stats_types_check(flow_action, extack, DELAYED))
> 		return -EOPNOTSUPP;
> 
> WDYT?

I'd rather have the 80+ lines than not be able to grep for it :(

What's wrong with flow_action_stats_ok()? Also perhaps, flow_act 
as a prefix?

Re: [patch net-next 0/3] flow_offload: follow-ups to HW stats type patchset

[Jiri Pirko]

Wed, Mar 11, 2020 at 09:30:28PM CET, kuba@kernel.org wrote:
>On Wed, 11 Mar 2020 08:19:55 +0100 Jiri Pirko wrote:
>> Tue, Mar 10, 2020 at 08:05:19PM CET, kuba@kernel.org wrote:
>> >On Tue, 10 Mar 2020 16:49:06 +0100 Jiri Pirko wrote:  
>> >> This patchset includes couple of patches in reaction to the discussions
>> >> to the original HW stats patchset. The first patch is a fix,
>> >> the other two patches are basically cosmetics.  
>> >
>> >Reviewed-by: Jakub Kicinski <kuba@kernel.org>
>> >
>> >This problem already exists, but writing a patch for nfp I noticed that
>> >there is no way for this:
>> >
>> >	if (!flow_action_hw_stats_types_check(flow_action, extack,
>> >					      FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT))
>> >		return -EOPNOTSUPP;
>> >
>> >to fit on a line for either bit, which kind of sucks.  
>> 
>> Yeah, I was thinking about having flow_action_hw_stats_types_check as a
>> macro and then just simply have:
>> 
>> 	if (!flow_action_hw_stats_types_check(flow_action, extack, DELAYED))
>> 		return -EOPNOTSUPP;
>> 
>> WDYT?
>
>I'd rather have the 80+ lines than not be able to grep for it :(
>
>What's wrong with flow_action_stats_ok()? Also perhaps, flow_act 
>as a prefix?

Well nothing, just that we'd loose consistency. Everything is
"flow_action_*" and also, the name you suggest might indicate that you
are checking sw stats. :/

Re: [patch net-next 0/3] flow_offload: follow-ups to HW stats type patchset

[Jakub Kicinski]

On Thu, 12 Mar 2020 08:03:59 +0100 Jiri Pirko wrote:
> Wed, Mar 11, 2020 at 09:30:28PM CET, kuba@kernel.org wrote:
> >On Wed, 11 Mar 2020 08:19:55 +0100 Jiri Pirko wrote:  
> >> Tue, Mar 10, 2020 at 08:05:19PM CET, kuba@kernel.org wrote:  
> >> >On Tue, 10 Mar 2020 16:49:06 +0100 Jiri Pirko wrote:    
> >> >> This patchset includes couple of patches in reaction to the discussions
> >> >> to the original HW stats patchset. The first patch is a fix,
> >> >> the other two patches are basically cosmetics.    
> >> >
> >> >Reviewed-by: Jakub Kicinski <kuba@kernel.org>
> >> >
> >> >This problem already exists, but writing a patch for nfp I noticed that
> >> >there is no way for this:
> >> >
> >> >	if (!flow_action_hw_stats_types_check(flow_action, extack,
> >> >					      FLOW_ACTION_HW_STATS_TYPE_DELAYED_BIT))
> >> >		return -EOPNOTSUPP;
> >> >
> >> >to fit on a line for either bit, which kind of sucks.    
> >> 
> >> Yeah, I was thinking about having flow_action_hw_stats_types_check as a
> >> macro and then just simply have:
> >> 
> >> 	if (!flow_action_hw_stats_types_check(flow_action, extack, DELAYED))
> >> 		return -EOPNOTSUPP;
> >> 
> >> WDYT?  
> >
> >I'd rather have the 80+ lines than not be able to grep for it :(
> >
> >What's wrong with flow_action_stats_ok()? Also perhaps, flow_act 
> >as a prefix?  
> 
> Well nothing, just that we'd loose consistency. Everything is
> "flow_action_*" and also, the name you suggest might indicate that you
> are checking sw stats. :/

SW stats in flow action? flow stuff is an abstraction for HW/drivers.

Re: [patch net-next 0/3] flow_offload: follow-ups to HW stats type patchset

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 10 Mar 2020 16:49:06 +0100

> This patchset includes couple of patches in reaction to the discussions
> to the original HW stats patchset. The first patch is a fix,
> the other two patches are basically cosmetics.

Series applied, thanks Jiri.