netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [ RFC  net-next 0/3] net: flow_offload: add support for per action hw stats
@ 2022-08-16  9:23 Oz Shlomo
  2022-08-16  9:23 ` [ RFC net-next 1/3] net: sched: Pass flow_stats instead of multiple stats args Oz Shlomo
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Oz Shlomo @ 2022-08-16  9:23 UTC (permalink / raw)
  To: netdev
  Cc: Jiri Pirko, Jamal Hadi Salim, Simon Horman, Baowen Zheng,
	Vlad Buslov, Ido Schimmel, Roi Dayan, 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.

The third patch refreshes the hardware action stats from the userspace tc action utility.
It uses the existing hardware action api to query stats per action.
The api has lower performance, compared to the filter refresh stats, as it requires
a driver callback invocation per action, while requiring the driver to lookup the stats
for a specific action id.

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.

Oz Shlomo (2):
  net: flow_offload: add action stats api
  net/sched: act_api: update hw stats for tc action list

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

 include/net/flow_offload.h |  6 ++++++
 include/net/pkt_cls.h      | 27 ++++++++++++++++-----------
 net/sched/act_api.c        | 15 +++++++++++----
 net/sched/cls_flower.c     |  9 +++------
 net/sched/cls_matchall.c   |  6 +-----
 5 files changed, 37 insertions(+), 26 deletions(-)

-- 
1.8.3.1


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

* [ RFC  net-next 1/3] net: sched: Pass flow_stats instead of multiple stats args
  2022-08-16  9:23 [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats Oz Shlomo
@ 2022-08-16  9:23 ` Oz Shlomo
  2022-08-16  9:23 ` [ RFC net-next 2/3] net: flow_offload: add action stats api Oz Shlomo
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Oz Shlomo @ 2022-08-16  9:23 UTC (permalink / raw)
  To: netdev
  Cc: Jiri Pirko, Jamal Hadi Salim, Simon Horman, Baowen Zheng,
	Vlad Buslov, Ido Schimmel, Roi Dayan, 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 d9d90e6925e1..27eac9e73c61 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -269,8 +269,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;
@@ -281,12 +280,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 041d63ff809a..7da3337c4356 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -499,12 +499,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 06cf22adbab7..b5520a9c35e6 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -332,11 +332,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


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

* [ RFC  net-next 2/3] net: flow_offload: add action stats api
  2022-08-16  9:23 [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats Oz Shlomo
  2022-08-16  9:23 ` [ RFC net-next 1/3] net: sched: Pass flow_stats instead of multiple stats args Oz Shlomo
@ 2022-08-16  9:23 ` Oz Shlomo
  2022-08-16 13:42   ` Edward Cree
  2022-08-16  9:23 ` [ RFC net-next 3/3] net/sched: act_api: update hw stats for tc action list Oz Shlomo
  2022-08-16 18:19 ` [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats Jakub Kicinski
  3 siblings, 1 reply; 10+ messages in thread
From: Oz Shlomo @ 2022-08-16  9:23 UTC (permalink / raw)
  To: netdev
  Cc: Jiri Pirko, Jamal Hadi Salim, Simon Horman, Baowen Zheng,
	Vlad Buslov, Ido Schimmel, Roi Dayan, 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,
decides to drop or jump over other actions.

Extend the flow_offload api to return stat record per action instance.
Use the per action stats value, if available, when updating the action
instance counters.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
---
 include/net/flow_offload.h |  6 ++++++
 include/net/pkt_cls.h      | 26 ++++++++++++++++----------
 net/sched/cls_flower.c     |  4 +++-
 net/sched/cls_matchall.c   |  2 +-
 4 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 2a9a9e42e7fd..5e1a34a76772 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -436,6 +436,11 @@ struct flow_stats {
 	bool used_hw_stats_valid;
 };
 
+struct flow_act_stats {
+	unsigned int		num_actions;
+	struct flow_stats	stats[];
+};
+
 static inline void flow_stats_update(struct flow_stats *flow_stats,
 				     u64 bytes, u64 pkts,
 				     u64 drops, u64 lastused,
@@ -583,6 +588,7 @@ struct flow_cls_offload {
 	struct flow_rule *rule;
 	struct flow_stats stats;
 	u32 classid;
+	struct flow_act_stats *act_stats;
 };
 
 enum offload_act_command  {
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 27eac9e73c61..f5e5582aef17 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -269,24 +269,30 @@ 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_stats *act_stats)
 {
 #ifdef CONFIG_NET_CLS_ACT
 	int i;
 
 	for (i = 0; i < exts->nr_actions; i++) {
 		struct tc_action *a = exts->actions[i];
+		struct flow_stats *stats = flow_stats;
 
 		/* if stats from hw, just skip */
-		if (tcf_action_update_hw_stats(a)) {
-			preempt_disable();
-			tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
-						stats->lastused, true);
-			preempt_enable();
-
-			a->used_hw_stats = stats->used_hw_stats;
-			a->used_hw_stats_valid = stats->used_hw_stats_valid;
-		}
+		if (!tcf_action_update_hw_stats(a))
+			continue;
+
+		if (act_stats)
+			stats = &act_stats->stats[i];
+
+		preempt_disable();
+		tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
+					stats->lastused, true);
+		preempt_enable();
+
+		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 7da3337c4356..7dc8a62796b5 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -499,7 +499,9 @@ 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);
+
+	kfree(cls_flower.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 b5520a9c35e6..0ba4392b93de 100644
--- a/net/sched/cls_matchall.c
+++ b/net/sched/cls_matchall.c
@@ -332,7 +332,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);
 }
 
 static int mall_dump(struct net *net, struct tcf_proto *tp, void *fh,
-- 
1.8.3.1


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

* [ RFC  net-next 3/3] net/sched: act_api: update hw stats for tc action list
  2022-08-16  9:23 [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats Oz Shlomo
  2022-08-16  9:23 ` [ RFC net-next 1/3] net: sched: Pass flow_stats instead of multiple stats args Oz Shlomo
  2022-08-16  9:23 ` [ RFC net-next 2/3] net: flow_offload: add action stats api Oz Shlomo
@ 2022-08-16  9:23 ` Oz Shlomo
  2022-08-16 18:19 ` [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats Jakub Kicinski
  3 siblings, 0 replies; 10+ messages in thread
From: Oz Shlomo @ 2022-08-16  9:23 UTC (permalink / raw)
  To: netdev
  Cc: Jiri Pirko, Jamal Hadi Salim, Simon Horman, Baowen Zheng,
	Vlad Buslov, Ido Schimmel, Roi Dayan, Oz Shlomo

Currently action hw stats are updated during the tc filter dump sequence.
HW actions are also updated during the tc action dump sequence.
However, tc action dump does not update the hw stats for actions created
during filter instantiation.

Use the existing hw action api to update hw stats during tc action dump.

Signed-off-by: Oz Shlomo <ozsh@nvidia.com>

---
 net/sched/act_api.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 817065aa2833..5d7b6e438085 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -301,14 +301,11 @@ static int tcf_action_offload_add(struct tc_action *action,
 	return tcf_action_offload_add_ex(action, extack, NULL, NULL);
 }
 
-int tcf_action_update_hw_stats(struct tc_action *action)
+static int tcf_action_set_hw_stats(struct tc_action *action)
 {
 	struct flow_offload_action fl_act = {};
 	int err;
 
-	if (!tc_act_in_hw(action))
-		return -EOPNOTSUPP;
-
 	err = offload_action_init(&fl_act, action, FLOW_ACT_STATS, NULL);
 	if (err)
 		return err;
@@ -330,6 +327,14 @@ int tcf_action_update_hw_stats(struct tc_action *action)
 
 	return 0;
 }
+
+int tcf_action_update_hw_stats(struct tc_action *action)
+{
+	if (!tc_act_in_hw(action))
+		return -EOPNOTSUPP;
+
+	return tcf_action_set_hw_stats(action);
+}
 EXPORT_SYMBOL(tcf_action_update_hw_stats);
 
 static int tcf_action_offload_del_ex(struct tc_action *action,
@@ -543,6 +548,8 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
 			index--;
 			goto nla_put_failure;
 		}
+		tcf_action_set_hw_stats(p);
+
 		err = (act_flags & TCA_ACT_FLAG_TERSE_DUMP) ?
 			tcf_action_dump_terse(skb, p, true) :
 			tcf_action_dump_1(skb, p, 0, 0);
-- 
1.8.3.1


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

* Re: [ RFC net-next 2/3] net: flow_offload: add action stats api
  2022-08-16  9:23 ` [ RFC net-next 2/3] net: flow_offload: add action stats api Oz Shlomo
@ 2022-08-16 13:42   ` Edward Cree
  2022-08-17 14:43     ` Oz Shlomo
  0 siblings, 1 reply; 10+ messages in thread
From: Edward Cree @ 2022-08-16 13:42 UTC (permalink / raw)
  To: Oz Shlomo, netdev
  Cc: Jiri Pirko, Jamal Hadi Salim, Simon Horman, Baowen Zheng,
	Vlad Buslov, Ido Schimmel, Roi Dayan

On 16/08/2022 10:23, 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,
> decides to drop or jump over other actions.
> 
> Extend the flow_offload api to return stat record per action instance.
> Use the per action stats value, if available, when updating the action
> instance counters.
> 
> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>

When I worked on this before I tried with a similar "array of action
 stats" API [1], but after some discussion it seemed cleaner to have
 a "get stats for one single action" callback [2] which then could
 be called in a loop for filter dumps but also called singly for
 action dumps (RTM_GETACTION).  I recommend this approach to your
 consideration.

[1]: https://lore.kernel.org/all/9804a392-c9fd-8d03-7900-e01848044fea@solarflare.com/
[2]: https://lore.kernel.org/all/a3f0a79a-7e2c-4cdc-8c97-dfebe959ab1f@solarflare.com/

> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
> index 7da3337c4356..7dc8a62796b5 100644
> --- a/net/sched/cls_flower.c
> +++ b/net/sched/cls_flower.c
> @@ -499,7 +499,9 @@ 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);
> +
> +	kfree(cls_flower.act_stats);
>  }

Perhaps I'm being dumb, but I don't see this being allocated
 anywhere.  Is the driver supposed to be responsible for doing so?
 That seems inelegant.

-ed

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

* Re: [ RFC  net-next 0/3] net: flow_offload: add support for per action hw stats
  2022-08-16  9:23 [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats Oz Shlomo
                   ` (2 preceding siblings ...)
  2022-08-16  9:23 ` [ RFC net-next 3/3] net/sched: act_api: update hw stats for tc action list Oz Shlomo
@ 2022-08-16 18:19 ` Jakub Kicinski
  2022-08-17 14:46   ` Oz Shlomo
  3 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2022-08-16 18:19 UTC (permalink / raw)
  To: Oz Shlomo
  Cc: netdev, Jiri Pirko, Jamal Hadi Salim, Simon Horman, Baowen Zheng,
	Vlad Buslov, Ido Schimmel, Roi Dayan

On Tue, 16 Aug 2022 12:23:35 +0300 Oz Shlomo wrote:
> This series provides the platform to query per action stats for in_hw flows.

I'd like to make sure we document the driver-facing APIs going forward,
please consider adding a doc.

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

* Re: [ RFC net-next 2/3] net: flow_offload: add action stats api
  2022-08-16 13:42   ` Edward Cree
@ 2022-08-17 14:43     ` Oz Shlomo
  2022-09-28 15:19       ` Oz Shlomo
  0 siblings, 1 reply; 10+ messages in thread
From: Oz Shlomo @ 2022-08-17 14:43 UTC (permalink / raw)
  To: Edward Cree, netdev
  Cc: Jiri Pirko, Jamal Hadi Salim, Simon Horman, Baowen Zheng,
	Vlad Buslov, Ido Schimmel, Roi Dayan

Hi Edward,

On 8/16/2022 4:42 PM, Edward Cree wrote:
> On 16/08/2022 10:23, 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,
>> decides to drop or jump over other actions.
>>
>> Extend the flow_offload api to return stat record per action instance.
>> Use the per action stats value, if available, when updating the action
>> instance counters.
>>
>> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
> 
> When I worked on this before I tried with a similar "array of action
>   stats" API [1], but after some discussion it seemed cleaner to have
>   a "get stats for one single action" callback [2] which then could
>   be called in a loop for filter dumps but also called singly for
>   action dumps (RTM_GETACTION).  I recommend this approach to your
>   consideration.
> 
> [1]: https://lore.kernel.org/all/9804a392-c9fd-8d03-7900-e01848044fea@solarflare.com/
> [2]: https://lore.kernel.org/all/a3f0a79a-7e2c-4cdc-8c97-dfebe959ab1f@solarflare.com/
> 

The recent hw_actions infrastructure provides the platform for updating 
stats per action.
However, the platform does introduce performance penalties as it invokes 
a driver api method call per action (compared to the current single api 
call). It also requires the driver to lookup the specific action counter 
- requiring more processing compared to the current flow cookie lookup.
Further more, the current single stats per filter (rather than per 
action) design only breaks when using branching actions (e.g. police), 
which probably applies to a small subset of the rules.

This series proposes two apis:
1. High performance api for filter dump update (ovs triggers a dump per 
rule per second) - extending the current api providing the driver an 
option to update stats per action, if required.
2. Re-use the hw_actions api for tc action list update (see patch #3)

>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>> index 7da3337c4356..7dc8a62796b5 100644
>> --- a/net/sched/cls_flower.c
>> +++ b/net/sched/cls_flower.c
>> @@ -499,7 +499,9 @@ 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);
>> +
>> +	kfree(cls_flower.act_stats);
>>   }
> 
> Perhaps I'm being dumb, but I don't see this being allocated
>   anywhere.  Is the driver supposed to be responsible for doing so?
>   That seems inelegant.

You are right, the intention is for the driver to allocate the array and 
for the calling method to free it.

While the proposed design is indeed inelegant, it is efficient compared 
to the possible other alternatives:
1. Dynamically allocated stats array - this will introduce an alloc/free 
calls per stats query (1 / filter/ second), even if per action stats is 
not required.
2. Static action stats array - this has size issues, as this api is 
shared for both tc and nft. Perhaps we can use a hard coded size and 
return an error if the actual counter array size is larger.


> 
> -ed

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

* Re: [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats
  2022-08-16 18:19 ` [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats Jakub Kicinski
@ 2022-08-17 14:46   ` Oz Shlomo
  2022-08-17 16:02     ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: Oz Shlomo @ 2022-08-17 14:46 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: netdev, Jiri Pirko, Jamal Hadi Salim, Simon Horman, Baowen Zheng,
	Vlad Buslov, Ido Schimmel, Roi Dayan



On 8/16/2022 9:19 PM, Jakub Kicinski wrote:
> On Tue, 16 Aug 2022 12:23:35 +0300 Oz Shlomo wrote:
>> This series provides the platform to query per action stats for in_hw flows.
> 
> I'd like to make sure we document the driver-facing APIs going forward,
> please consider adding a doc.

Is there an existing document that we can update?

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

* Re: [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats
  2022-08-17 14:46   ` Oz Shlomo
@ 2022-08-17 16:02     ` Jakub Kicinski
  0 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2022-08-17 16:02 UTC (permalink / raw)
  To: Oz Shlomo
  Cc: netdev, Jiri Pirko, Jamal Hadi Salim, Simon Horman, Baowen Zheng,
	Vlad Buslov, Ido Schimmel, Roi Dayan

On Wed, 17 Aug 2022 17:46:54 +0300 Oz Shlomo wrote:
> On 8/16/2022 9:19 PM, Jakub Kicinski wrote:
> > On Tue, 16 Aug 2022 12:23:35 +0300 Oz Shlomo wrote:  
> >> This series provides the platform to query per action stats for in_hw flows.  
> > 
> > I'd like to make sure we document the driver-facing APIs going forward,
> > please consider adding a doc.  
> 
> Is there an existing document that we can update?

Not that I know of.

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

* Re: [ RFC net-next 2/3] net: flow_offload: add action stats api
  2022-08-17 14:43     ` Oz Shlomo
@ 2022-09-28 15:19       ` Oz Shlomo
  0 siblings, 0 replies; 10+ messages in thread
From: Oz Shlomo @ 2022-09-28 15:19 UTC (permalink / raw)
  To: Edward Cree, netdev
  Cc: Jiri Pirko, Jamal Hadi Salim, Simon Horman, Baowen Zheng,
	Vlad Buslov, Ido Schimmel, Roi Dayan

Hן Edward,

On 8/17/2022 5:43 PM, Oz Shlomo wrote:
> Hi Edward,
> 
> On 8/16/2022 4:42 PM, Edward Cree wrote:
>> On 16/08/2022 10:23, 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,
>>> decides to drop or jump over other actions.
>>>
>>> Extend the flow_offload api to return stat record per action instance.
>>> Use the per action stats value, if available, when updating the action
>>> instance counters.
>>>
>>> Signed-off-by: Oz Shlomo <ozsh@nvidia.com>
>>
>> When I worked on this before I tried with a similar "array of action
>>   stats" API [1], but after some discussion it seemed cleaner to have
>>   a "get stats for one single action" callback [2] which then could
>>   be called in a loop for filter dumps but also called singly for
>>   action dumps (RTM_GETACTION).  I recommend this approach to your
>>   consideration.
>>
>> [1]: 
>> https://lore.kernel.org/all/9804a392-c9fd-8d03-7900-e01848044fea@solarflare.com/ 
>>
>> [2]: 
>> https://lore.kernel.org/all/a3f0a79a-7e2c-4cdc-8c97-dfebe959ab1f@solarflare.com/ 
>>
>>
> 
> The recent hw_actions infrastructure provides the platform for updating 
> stats per action.
> However, the platform does introduce performance penalties as it invokes 
> a driver api method call per action (compared to the current single api 
> call). It also requires the driver to lookup the specific action counter 
> - requiring more processing compared to the current flow cookie lookup.
> Further more, the current single stats per filter (rather than per 
> action) design only breaks when using branching actions (e.g. police), 
> which probably applies to a small subset of the rules.
> 
> This series proposes two apis:
> 1. High performance api for filter dump update (ovs triggers a dump per 
> rule per second) - extending the current api providing the driver an 
> option to update stats per action, if required.
> 2. Re-use the hw_actions api for tc action list update (see patch #3)
> 

I tried implementing the per action stats using the hw_action api.
The api proved itself well.
However, it is extremely inefficient to allocate a counter per action in
hardware. As such, the driver is required to lookup the action's counter
(hashtable lookup) and also update all the other action stats hanging on
this hw counter (requiring list iteration and locks).
This introduces quite a complex design with performance overheads.

Stats update is performance sensitive as ovs queries the filters' stats
every second.
Supporting tc action stats api will degrade the performance for existing
use cases.
Extending the existing flow_offload api will preserve the current
functionality (single flow stat which applies to all the actions) and
performance while providing the ability to specify per action stats for
use cases involving branching actions.
In the future we could add driver support for returning a per action
stats using the current hw_action api.
WDYT?

>>> diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
>>> index 7da3337c4356..7dc8a62796b5 100644
>>> --- a/net/sched/cls_flower.c
>>> +++ b/net/sched/cls_flower.c
>>> @@ -499,7 +499,9 @@ 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);
>>> +
>>> +    kfree(cls_flower.act_stats);
>>>   }
>>
>> Perhaps I'm being dumb, but I don't see this being allocated
>>   anywhere.  Is the driver supposed to be responsible for doing so?
>>   That seems inelegant.
> 
> You are right, the intention is for the driver to allocate the array and 
> for the calling method to free it.
> 
> While the proposed design is indeed inelegant, it is efficient compared 
> to the possible other alternatives:
> 1. Dynamically allocated stats array - this will introduce an alloc/free 
> calls per stats query (1 / filter/ second), even if per action stats is 
> not required.
> 2. Static action stats array - this has size issues, as this api is 
> shared for both tc and nft. Perhaps we can use a hard coded size and 
> return an error if the actual counter array size is larger.
> 
> 

I realized that we cannot assume a 1:1 mapping between tc action and its
corresponding offload action as tc pedit action can create an array of
flow offload actions.
I will fix this in v2.

>>
>> -ed

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

end of thread, other threads:[~2022-09-28 15:20 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-16  9:23 [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats Oz Shlomo
2022-08-16  9:23 ` [ RFC net-next 1/3] net: sched: Pass flow_stats instead of multiple stats args Oz Shlomo
2022-08-16  9:23 ` [ RFC net-next 2/3] net: flow_offload: add action stats api Oz Shlomo
2022-08-16 13:42   ` Edward Cree
2022-08-17 14:43     ` Oz Shlomo
2022-09-28 15:19       ` Oz Shlomo
2022-08-16  9:23 ` [ RFC net-next 3/3] net/sched: act_api: update hw stats for tc action list Oz Shlomo
2022-08-16 18:19 ` [ RFC net-next 0/3] net: flow_offload: add support for per action hw stats Jakub Kicinski
2022-08-17 14:46   ` Oz Shlomo
2022-08-17 16:02     ` Jakub Kicinski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).