[ RFC net-next v2 0/2] net: flow_offload: add support for per action hw stats

[ RFC net-next v2 0/2] net: flow_offload: add support for per action hw stats

[Oz Shlomo]

There are currently two mechanisms for populating hardware stats:
1. Using flow_offload api to query the flow's statistics.
   The api assumes that the same stats values apply to all
   the flow's actions.
   This assumption breaks when action drops or jumps over following
   actions.
2. Using hw_action api to query specific action stats via a driver
   callback method. This api assures the correct action stats for
   the offloaded action, however, it does not apply to the rest of the
   actions in the flow's actions array, as elaborated below.

The current hw_action api does not apply to the following use cases:
1. Actions that are implicitly created by filters (aka bind actions).
   In the following example only one counter will apply to the rule:
   tc filter add dev $DEV prio 2 protocol ip parent ffff: \
        flower ip_proto tcp dst_ip $IP2 \
        action police rate 1mbit burst 100k conform-exceed drop/pipe \
        action mirred egress redirect dev $DEV2
  
2. Action preceding a hw action.
   In the following example the same flow stats will apply to the sample and
   mirred actions:
    tc action add police rate 1mbit burst 100k conform-exceed drop / pipe
    tc filter add dev $DEV prio 2 protocol ip parent ffff: \
        flower ip_proto tcp dst_ip $IP2 \
        action sample rate 1 group 10 trunc 60 pipe \
        action police index 1 \
        action mirred egress redirect dev $DEV2
        
3. Meter action using jump control.
   In the following example the same flow stats will apply to both
   mirred actions:
    tc action add police rate 1mbit burst 100k conform-exceed jump 2 / pipe
    tc filter add dev $DEV prio 2 protocol ip parent ffff: \
        flower ip_proto tcp dst_ip $IP2 \
        action police index 1 \
        action mirred egress redirect dev $DEV2
        action mirred egress redirect dev $DEV3

This series provides the platform to query per action stats for in_hw flows.

The first patch is a preparation patch

The second patch extends the flow_offload api to return stats array corresponding
to the flow's actions list.
The api populates all the actions' stats in a single callback invocation.
It also allows drivers to avoid per-action lookups by maintain pre-processed
array of the flow's action counters.

Note that this series does not change the existing functionality, thus preserving
the current stats per flow design.

Mellanox driver implementation of the proposed api will follow the rfc discussion.

-----

v1 -> v2:
- Change flow_offload action stats to a static array
- Assign action_cookie to flow_offload actions
- Use action cookie to dereference the action to be updated
- Remove single action update

Oz Shlomo (1):
  net: flow_offload: add action stats api

Roi Dayan (1):
  net: sched: Pass flow_stats instead of multiple stats args

 include/net/flow_offload.h | 10 ++++++++++
 include/net/pkt_cls.h      | 27 ++++++++++++++++++++-------
 net/sched/cls_api.c        |  1 +
 net/sched/cls_flower.c     |  8 ++------
 net/sched/cls_matchall.c   |  6 +-----
 5 files changed, 34 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[ RFC net-next v2 1/2] net: sched: Pass flow_stats instead of multiple stats args

[Oz Shlomo]

From: Roi Dayan <roid@nvidia.com>

Instead of passing 6 stats related args, pass the flow_stats.

Signed-off-by: Roi Dayan <roid@nvidia.com>
Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
---
 include/net/pkt_cls.h    | 11 +++++------
 net/sched/cls_flower.c   |  7 +------
 net/sched/cls_matchall.c |  6 +-----
 3 files changed, 7 insertions(+), 17 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index d376c995d906..d5b8fa01da87 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -282,8 +282,7 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
 
 static inline void
 tcf_exts_hw_stats_update(const struct tcf_exts *exts,
-			 u64 bytes, u64 packets, u64 drops, u64 lastuse,
-			 u8 used_hw_stats, bool used_hw_stats_valid)
+			 struct flow_stats *stats)
 {
 #ifdef CONFIG_NET_CLS_ACT
 	int i;
@@ -294,12 +293,12 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
 		/* if stats from hw, just skip */
 		if (tcf_action_update_hw_stats(a)) {
 			preempt_disable();
-			tcf_action_stats_update(a, bytes, packets, drops,
-						lastuse, true);
+			tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
+						stats->lastused, true);
 			preempt_enable();
 
-			a->used_hw_stats = used_hw_stats;
-			a->used_hw_stats_valid = used_hw_stats_valid;
+			a->used_hw_stats = stats->used_hw_stats;
+			a->used_hw_stats_valid = stats->used_hw_stats_valid;
 		}
 	}
 #endif
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 22d32b82bc09..82b3e8ff656c 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -500,12 +500,7 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
 	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false,
 			 rtnl_held);
 
-	tcf_exts_hw_stats_update(&f->exts, cls_flower.stats.bytes,
-				 cls_flower.stats.pkts,
-				 cls_flower.stats.drops,
-				 cls_flower.stats.lastused,
-				 cls_flower.stats.used_hw_stats,
-				 cls_flower.stats.used_hw_stats_valid);
+	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats);
 }
 
 static void __fl_put(struct cls_fl_filter *f)
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 63b99ffb7dbc..225e87740ec5 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -329,11 +329,7 @@ static void mall_stats_hw_filter(struct tcf_proto *tp,
 
 	tc_setup_cb_call(block, TC_SETUP_CLSMATCHALL, &cls_mall, false, true);
 
-	tcf_exts_hw_stats_update(&head->exts, cls_mall.stats.bytes,
-				 cls_mall.stats.pkts, cls_mall.stats.drops,
-				 cls_mall.stats.lastused,
-				 cls_mall.stats.used_hw_stats,
-				 cls_mall.stats.used_hw_stats_valid);
+	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats);
 }
 
 static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,
-- 
1.8.3.1

[ RFC net-next v2 2/2] net: flow_offload: add action stats api

[Oz Shlomo]

The current offload api provides visibility to flow hw stats.
This works as long as the flow stats values apply to all the flow's
actions. However, this assumption breaks when an action, such as police,
drops or jumps over other actions.

Extend the flow_offload api to return stat record per action instance.
Use the specific action, identified with an action cookie, stats value
when updating the action's hardware stats.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
---
 include/net/flow_offload.h | 10 ++++++++++
 include/net/pkt_cls.h      | 18 ++++++++++++++++--
 net/sched/cls_api.c        |  1 +
 net/sched/cls_flower.c     |  3 ++-
 net/sched/cls_matchall.c   |  2 +-
 5 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index e343f9f8363e..1c88ca113544 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -213,6 +213,8 @@ struct flow_action_cookie {
 	u8 cookie[];
 };
 
+#define FLOW_OFFLOAD_MAX_ACT_STATS 32
+
 struct flow_action_cookie *flow_action_cookie_create(void *data,
 						     unsigned int len,
 						     gfp_t gfp);
@@ -221,6 +223,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data,
 struct flow_action_entry {
 	enum flow_action_id		id;
 	u32				hw_index;
+	unsigned long			act_cookie;
 	enum flow_action_hw_stats	hw_stats;
 	action_destr			destructor;
 	void				*destructor_priv;
@@ -442,6 +445,11 @@ struct flow_stats {
 	bool used_hw_stats_valid;
 };
 
+struct flow_act_stat {
+	unsigned long		act_cookie;
+	struct flow_stats	stats;
+};
+
 static inline void flow_stats_update(struct flow_stats *flow_stats,
 				     u64 bytes, u64 pkts,
 				     u64 drops, u64 lastused,
@@ -588,6 +596,8 @@ struct flow_cls_offload {
 	unsigned long cookie;
 	struct flow_rule *rule;
 	struct flow_stats stats;
+	struct flow_act_stat act_stats[FLOW_OFFLOAD_MAX_ACT_STATS];
+	bool use_act_stats;
 	u32 classid;
 };
 
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index d5b8fa01da87..642e6f07cbf0 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -282,13 +282,27 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
 
 static inline void
 tcf_exts_hw_stats_update(const struct tcf_exts *exts,
-			 struct flow_stats *stats)
+			 struct flow_stats *flow_stats,
+			 struct flow_act_stat *act_stats,
+			 bool use_act_stats)
 {
 #ifdef CONFIG_NET_CLS_ACT
+	int nr_actions = exts->nr_actions;
 	int i;
 
-	for (i = 0; i < exts->nr_actions; i++) {
+	if (use_act_stats)
+		nr_actions = FLOW_OFFLOAD_MAX_ACT_STATS;
+
+	for (i = 0; i < nr_actions; i++) {
 		struct tc_action *a = exts->actions[i];
+		struct flow_stats *stats = flow_stats;
+
+		if (use_act_stats) {
+			a = (struct tc_action *)act_stats[i].act_cookie;
+			if (!a)
+				break;
+			stats = &act_stats[i].stats;
+		}
 
 		/* if stats from hw, just skip */
 		if (tcf_action_update_hw_stats(a)) {
diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 50566db45949..c5a6a0d7f7a1 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -3553,6 +3553,7 @@ int tc_setup_action(struct flow_action *flow_action,
 		for (k = 0; k < index ; k++) {
 			entry[k].hw_stats = tc_act_hw_stats(act->hw_stats);
 			entry[k].hw_index = act->tcfa_index;
+			entry[k].act_cookie = (unsigned long)act;
 		}
 
 		j += index;
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index 82b3e8ff656c..ff004f13d0c9 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -500,7 +500,8 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
 	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false,
 			 rtnl_held);
 
-	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats);
+	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats,
+				 cls_flower.act_stats, cls_flower.use_act_stats);
 }
 
 static void __fl_put(struct cls_fl_filter *f)
diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
index 225e87740ec5..3d441063113d 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -329,7 +329,7 @@ static void mall_stats_hw_filter(struct tcf_proto *tp,
 
 	tc_setup_cb_call(block, TC_SETUP_CLSMATCHALL, &cls_mall, false, true);
 
-	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats);
+	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats, NULL, false);
 }
 
 static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,
-- 
1.8.3.1

Re: [ RFC net-next v2 2/2] net: flow_offload: add action stats api

[Oz Shlomo]

On 10/3/2022 9:17 AM, Oz Shlomo wrote:
> The current offload api provides visibility to flow hw stats.
> This works as long as the flow stats values apply to all the flow's
> actions. However, this assumption breaks when an action, such as police,
> drops or jumps over other actions.
> 
> Extend the flow_offload api to return stat record per action instance.
> Use the specific action, identified with an action cookie, stats value
> when updating the action's hardware stats.
> 
> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
> ---
>   include/net/flow_offload.h | 10 ++++++++++
>   include/net/pkt_cls.h      | 18 ++++++++++++++++--
>   net/sched/cls_api.c        |  1 +
>   net/sched/cls_flower.c     |  3 ++-
>   net/sched/cls_matchall.c   |  2 +-
>   5 files changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
> index e343f9f8363e..1c88ca113544 100644
> --- a/include/net/flow_offload.h
> +++ b/include/net/flow_offload.h
> @@ -213,6 +213,8 @@ struct flow_action_cookie {
>   	u8 cookie[];
>   };
>   
> +#define FLOW_OFFLOAD_MAX_ACT_STATS 32
> +
>   struct flow_action_cookie *flow_action_cookie_create(void *data,
>   						     unsigned int len,
>   						     gfp_t gfp);
> @@ -221,6 +223,7 @@ struct flow_action_cookie *flow_action_cookie_create(void *data,
>   struct flow_action_entry {
>   	enum flow_action_id		id;
>   	u32				hw_index;
> +	unsigned long			act_cookie;
>   	enum flow_action_hw_stats	hw_stats;
>   	action_destr			destructor;
>   	void				*destructor_priv;
> @@ -442,6 +445,11 @@ struct flow_stats {
>   	bool used_hw_stats_valid;
>   };
>   
> +struct flow_act_stat {
> +	unsigned long		act_cookie;
> +	struct flow_stats	stats;
> +};
> +
>   static inline void flow_stats_update(struct flow_stats *flow_stats,
>   				     u64 bytes, u64 pkts,
>   				     u64 drops, u64 lastused,
> @@ -588,6 +596,8 @@ struct flow_cls_offload {
>   	unsigned long cookie;
>   	struct flow_rule *rule;
>   	struct flow_stats stats;
> +	struct flow_act_stat act_stats[FLOW_OFFLOAD_MAX_ACT_STATS];

Apparently this array can exceed the stack frame size for several archs 
(reported by the kernel test bot).
I will change this array to be dynamically allocated.

> +	bool use_act_stats;
>   	u32 classid;
>   };
>   
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index d5b8fa01da87..642e6f07cbf0 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -282,13 +282,27 @@ static inline void tcf_exts_put_net(struct tcf_exts *exts)
>   
>   static inline void
>   tcf_exts_hw_stats_update(const struct tcf_exts *exts,
> -			 struct flow_stats *stats)
> +			 struct flow_stats *flow_stats,
> +			 struct flow_act_stat *act_stats,
> +			 bool use_act_stats)
>   {
>   #ifdef CONFIG_NET_CLS_ACT
> +	int nr_actions = exts->nr_actions;
>   	int i;
>   
> -	for (i = 0; i < exts->nr_actions; i++) {
> +	if (use_act_stats)
> +		nr_actions = FLOW_OFFLOAD_MAX_ACT_STATS;
> +
> +	for (i = 0; i < nr_actions; i++) {
>   		struct tc_action *a = exts->actions[i];
> +		struct flow_stats *stats = flow_stats;
> +
> +		if (use_act_stats) {
> +			a = (struct tc_action *)act_stats[i].act_cookie;
> +			if (!a)
> +				break;
> +			stats = &act_stats[i].stats;
> +		}
>   
>   		/* if stats from hw, just skip */
>   		if (tcf_action_update_hw_stats(a)) {
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 50566db45949..c5a6a0d7f7a1 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -3553,6 +3553,7 @@ int tc_setup_action(struct flow_action *flow_action,
>   		for (k = 0; k < index ; k++) {
>   			entry[k].hw_stats = tc_act_hw_stats(act->hw_stats);
>   			entry[k].hw_index = act->tcfa_index;
> +			entry[k].act_cookie = (unsigned long)act;
>   		}
>   
>   		j += index;
> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 82b3e8ff656c..ff004f13d0c9 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -500,7 +500,8 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
>   	tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false,
>   			 rtnl_held);
>   
> -	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats);
> +	tcf_exts_hw_stats_update(&f->exts, &cls_flower.stats,
> +				 cls_flower.act_stats, cls_flower.use_act_stats);
>   }
>   
>   static void __fl_put(struct cls_fl_filter *f)
> diff --git a/net/sched/cls_matchall.c b/net/sched/cls_matchall.c
> index 225e87740ec5..3d441063113d 100644
> --- a/net/sched/cls_matchall.c
> +++ b/net/sched/cls_matchall.c
> @@ -329,7 +329,7 @@ static void mall_stats_hw_filter(struct tcf_proto *tp,
>   
>   	tc_setup_cb_call(block, TC_SETUP_CLSMATCHALL, &cls_mall, false, true);
>   
> -	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats);
> +	tcf_exts_hw_stats_update(&head->exts, &cls_mall.stats, NULL, false);
>   }
>   
>   static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,

Re: [ RFC net-next v2 2/2] net: flow_offload: add action stats api

[Edward Cree]

On 04/10/2022 09:22, Oz Shlomo wrote:
>> @@ -588,6 +596,8 @@ struct flow_cls_offload {
>>       unsigned long cookie;
>>       struct flow_rule *rule;
>>       struct flow_stats stats;
>> +    struct flow_act_stat act_stats[FLOW_OFFLOAD_MAX_ACT_STATS];
> 
> Apparently this array can exceed the stack frame size for several archs (reported by the kernel test bot).

Seems to me like yet another sign this array shouldn't exist in the
 first place and you should be calling a driver offload function per
 action and not trying to squeeze all this through the existing
 flow-rule-centric API.
Why can't the action stats be stored in struct flow_action_entry,
 inside struct flow_rule?  Then, if you really want a single call
 for performance reasons, action-stats-aware drivers could just walk
 through flow_action_for_each(..., &flow_rule->action) and update
 each action's stats based on action->act_cookie.  That should still
 allow proper shared action handling, and seems far more elegant
 than this array.  (Any time you have two arrays and there's a close
 connection between a[i] and b[i], that's a good sign one's elements
 should be a struct member of the other's, rather than them being
 tied together only by index.)

-ed