[RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action

[RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action

[Edward Cree]

Introduce a new offload command TC_CLSFLOWER_STATS_BYINDEX, similar to
 the existing TC_CLSFLOWER_STATS but specifying an action_index (the
 tcfa_index of the action), which is called for each stats-having action
 on the rule.  Drivers should implement either, but not both, of these
 commands.

Signed-off-by: Edward Cree <ecree@solarflare.com>
---
 include/net/pkt_cls.h  |  2 ++
 net/sched/cls_flower.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index d5e7a1af346f..0e33c52c23a8 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -762,6 +762,7 @@ enum tc_fl_command {
 	TC_CLSFLOWER_REPLACE,
 	TC_CLSFLOWER_DESTROY,
 	TC_CLSFLOWER_STATS,
+	TC_CLSFLOWER_STATS_BYINDEX,
 	TC_CLSFLOWER_TMPLT_CREATE,
 	TC_CLSFLOWER_TMPLT_DESTROY,
 };
@@ -773,6 +774,7 @@ struct tc_cls_flower_offload {
 	struct flow_rule *rule;
 	struct flow_stats stats;
 	u32 classid;
+	u32 action_index; /* for TC_CLSFLOWER_STATS_BYINDEX */
 };
 
 static inline struct flow_rule *
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c
index f6685fc53119..be339cd6a86e 100644
--- a/net/sched/cls_flower.c
+++ b/net/sched/cls_flower.c
@@ -474,6 +474,10 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
 {
 	struct tc_cls_flower_offload cls_flower = {};
 	struct tcf_block *block = tp->chain->block;
+#ifdef CONFIG_NET_CLS_ACT
+	struct tc_action *a;
+	int i;
+#endif
 
 	if (!rtnl_held)
 		rtnl_lock();
@@ -489,6 +493,32 @@ static void fl_hw_update_stats(struct tcf_proto *tp, struct cls_fl_filter *f,
 			      cls_flower.stats.pkts,
 			      cls_flower.stats.lastused);
 
+#ifdef CONFIG_NET_CLS_ACT
+	for (i = 0; i < f->exts.nr_actions; i++) {
+		a = f->exts.actions[i];
+
+		if (!a->ops->stats_update)
+			continue;
+		memset(&cls_flower, 0, sizeof(cls_flower));
+		tc_cls_common_offload_init(&cls_flower.common, tp, f->flags, NULL);
+		cls_flower.command = TC_CLSFLOWER_STATS_BYINDEX;
+		cls_flower.cookie = (unsigned long) f;
+		cls_flower.classid = f->res.classid;
+		cls_flower.action_index = a->tcfa_index;
+
+		tc_setup_cb_call(block, TC_SETUP_CLSFLOWER, &cls_flower, false);
+
+		/* Some ->stats_update() use percpu variables and must thus be
+		 * called with preemption disabled.
+		 */
+		preempt_disable();
+		a->ops->stats_update(a, cls_flower.stats.bytes,
+				     cls_flower.stats.pkts,
+				     cls_flower.stats.lastused, true);
+		preempt_enable();
+	}
+#endif
+
 	if (!rtnl_held)
 		rtnl_unlock();
 }

Re: [RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action

[Jakub Kicinski]

On Fri, 3 May 2019 16:06:55 +0100, Edward Cree wrote:
> Introduce a new offload command TC_CLSFLOWER_STATS_BYINDEX, similar to
>  the existing TC_CLSFLOWER_STATS but specifying an action_index (the
>  tcfa_index of the action), which is called for each stats-having action
>  on the rule.  Drivers should implement either, but not both, of these
>  commands.
> 
> Signed-off-by: Edward Cree <ecree@solarflare.com>
> ---
>  include/net/pkt_cls.h  |  2 ++
>  net/sched/cls_flower.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+)

It feels a little strange to me to call the new stats updates from
cls_flower, if we really want to support action sharing correctly.

Can RTM_GETACTION not be used to dump actions without dumping the
classifiers?  If we dump from the classifiers wouldn't that lead to
stale stats being returned?

Re: [RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action

[Jamal Hadi Salim]

On 2019-05-04 2:27 a.m., Jakub Kicinski wrote:
> On Fri, 3 May 2019 16:06:55 +0100, Edward Cree wrote:
>> Introduce a new offload command TC_CLSFLOWER_STATS_BYINDEX, similar to
>>   the existing TC_CLSFLOWER_STATS but specifying an action_index (the
>>   tcfa_index of the action), which is called for each stats-having action
>>   on the rule.  Drivers should implement either, but not both, of these
>>   commands.

[..]
> 
> It feels a little strange to me to call the new stats updates from
> cls_flower, if we really want to support action sharing correctly.
> 
> Can RTM_GETACTION not be used to dump actions without dumping the
> classifiers?  If we dump from the classifiers wouldn't that lead to
> stale stats being returned?


Not sure about the staleness factor, but:
For efficiency reasons we certainly need the RTM_GETACTION approach
(as you stated above we dont need to dump all that classifier info if
all we want are stats). This becomes a big deal if you have a lot
of stats/rules.
But we also need to support the reference from the classifier rules,
if for no other reason, to support tc semantics.

If we are going to support RTM_GETACTION, then it is important to
note one caveat: tc uses the index as an identifier for the action;
meaning attributes + stats. You specify the index when i want to change
an attribute or optionally when you create an action,
or do a get attributes and stats.
Example, to change an existing skbedit to set attribute
skbmark of 10 instead of whatever existing value was there:

tc actions replace action skbedit mark 10 index 2

and to get the attributes + stats for a specific
action instance:

tc actions get action skbedit index 2

or dump for specific action type as such:

tc actions ls action police

Most H/W i have seen has a global indexed stats table which is
shared by different action types (droppers, accept, mirror etc).
The specific actions may also have their own tables which also
then refer to the 32 bit index used in the stats table[1].
So for this to work well, the action will need at minimal to have
two indices one that is used in hardware stats table
and another that is kernel mapped to identify the attributes. Of
course we'll need to have a skip_sw flag etc.

cheers,
jamal

[1] except for the policers/meters which tend have their own state
tables and often counters.

Re: [RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action

[Edward Cree]

On 06/05/2019 13:41, Jamal Hadi Salim wrote:
> On 2019-05-04 2:27 a.m., Jakub Kicinski wrote:
>> On Fri, 3 May 2019 16:06:55 +0100, Edward Cree wrote:
>>> Introduce a new offload command TC_CLSFLOWER_STATS_BYINDEX, similar to
>>>   the existing TC_CLSFLOWER_STATS but specifying an action_index (the
>>>   tcfa_index of the action), which is called for each stats-having action
>>>   on the rule.  Drivers should implement either, but not both, of these
>>>   commands.
>
> [..]
>>
>> It feels a little strange to me to call the new stats updates from
>> cls_flower, if we really want to support action sharing correctly.
>>
>> Can RTM_GETACTION not be used to dump actions without dumping the
>> classifiers?  If we dump from the classifiers wouldn't that lead to
>> stale stats being returned?
>
> Not sure about the staleness factor, but:
> For efficiency reasons we certainly need the RTM_GETACTION approach
> (as you stated above we dont need to dump all that classifier info if
> all we want are stats). This becomes a big deal if you have a lot
> of stats/rules.
I don't know much of anything about RTM_GETACTION, but it doesn't appear
 to be part of the current "tc offload" world, which AIUI is very much
 centred around cls_flower.  I'm just trying to make counters in
 cls_flower offload do 'the right thing' (whatever that may be), anything
 else is out of scope.

> Most H/W i have seen has a global indexed stats table which is
> shared by different action types (droppers, accept, mirror etc).
> The specific actions may also have their own tables which also
> then refer to the 32 bit index used in the stats table[1].
> So for this to work well, the action will need at minimal to have
> two indices one that is used in hardware stats table
> and another that is kernel mapped to identify the attributes. Of
> course we'll need to have a skip_sw flag etc.
I'm not sure I'm parsing this correctly, but are you saying that the
 index namespace is per-action type?  I.e. a mirred and a drop action
 could have the same index yet expect to have separate counters?  My
 approach here has assumed that in such a case they would share their
 counters.

Re: [RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action

[Jakub Kicinski]

On Tue, 7 May 2019 13:27:15 +0100, Edward Cree wrote:
> On 06/05/2019 13:41, Jamal Hadi Salim wrote:
> > On 2019-05-04 2:27 a.m., Jakub Kicinski wrote:  
> >> On Fri, 3 May 2019 16:06:55 +0100, Edward Cree wrote:  
> >>> Introduce a new offload command TC_CLSFLOWER_STATS_BYINDEX, similar to
> >>>   the existing TC_CLSFLOWER_STATS but specifying an action_index (the
> >>>   tcfa_index of the action), which is called for each stats-having action
> >>>   on the rule.  Drivers should implement either, but not both, of these
> >>>   commands.  
> >>
> >> It feels a little strange to me to call the new stats updates from
> >> cls_flower, if we really want to support action sharing correctly.
> >>
> >> Can RTM_GETACTION not be used to dump actions without dumping the
> >> classifiers?  If we dump from the classifiers wouldn't that lead to
> >> stale stats being returned?  
> >
> > Not sure about the staleness factor, but:
> > For efficiency reasons we certainly need the RTM_GETACTION approach
> > (as you stated above we dont need to dump all that classifier info if
> > all we want are stats). This becomes a big deal if you have a lot
> > of stats/rules.  
> 
> I don't know much of anything about RTM_GETACTION, but it doesn't appear
>  to be part of the current "tc offload" world, which AIUI is very much
>  centred around cls_flower.  I'm just trying to make counters in
>  cls_flower offload do 'the right thing' (whatever that may be), anything
>  else is out of scope.

I understand, and you are correct that the action sharing is not part
of the current "tc offload" world.  My point is that you are making it
part of the offload world, so it'd be best if it could be done well
from the start.

People trying to make the code do the right thing just in their narrow
use case brings us the periodic rewrites of the TC offload
infrastructure, and every time we do it some use cases get broken :/

Re: [RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action

[Jamal Hadi Salim]

On 2019-05-07 8:27 a.m., Edward Cree wrote:
> On 06/05/2019 13:41, Jamal Hadi Salim wrote:
>> On 2019-05-04 2:27 a.m., Jakub Kicinski wrote:
>>> On Fri, 3 May 2019 16:06:55 +0100, Edward Cree wrote:

[..]


> I don't know much of anything about RTM_GETACTION, but it doesn't appear
>   to be part of the current "tc offload" world, which AIUI is very much
>   centred around cls_flower.  I'm just trying to make counters in
>   cls_flower offload do 'the right thing' (whatever that may be), anything
>   else is out of scope.
> 

The lazy thing most people have done is essentially assume that
there is a stat per filter rule. From tc semantics that is an implicit
"pipe" action (which then carries stats).
And most times they implement a single action per match. I wouldnt call
it the 'the right thing'...

>> Most H/W i have seen has a global indexed stats table which is
>> shared by different action types (droppers, accept, mirror etc).
>> The specific actions may also have their own tables which also
>> then refer to the 32 bit index used in the stats table[1].
>> So for this to work well, the action will need at minimal to have
>> two indices one that is used in hardware stats table
>> and another that is kernel mapped to identify the attributes. Of
>> course we'll need to have a skip_sw flag etc.
> I'm not sure I'm parsing this correctly, but are you saying that the
>   index namespace is per-action type?  I.e. a mirred and a drop action
>   could have the same index yet expect to have separate counters?  My
>   approach here has assumed that in such a case they would share their
>   counters.

Yes, the index at tc semantics level is per-action type.
So "mirred index 1" and "drop index 1" are not the same stats counter.
Filters on the other hand can share per-action type stats by virtue of
sharing the same index per-action. e.g

flower match foo action drop index 1
flower match bar action drop index 1

will share the same stats.

cheers,
jamal

Re: [RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action

[Edward Cree]

On 08/05/2019 15:02, Jamal Hadi Salim wrote:
> The lazy thing most people have done is essentially assume that
> there is a stat per filter rule...
> I wouldnt call it the 'the right thing'
Yup, that's why I'm trying to not do that ;-)

> Yes, the index at tc semantics level is per-action type.
> So "mirred index 1" and "drop index 1" are not the same stats counter.
Ok, then that kills the design I used here that relied entirely on the
 index to specify counters.
I guess instead I'll have to go with the approach Pablo suggested,
 passing an array of struct flow_stats in the callback, thus using
 the index into that array (which corresponds to the index in
 f->exts->actions) to identify different counters.
Which means I will have to change all the existing drivers, which will
 largely revert (from the drivers' perspective) the change when Pablo
 took f->exts away from them — they will go back to calling something
 that looks a lot like tcf_exts_stats_update().
However, that'll mean the API has in-tree users, so it might be
 considered mergeable(?)

-Ed

Re: [RFC PATCH net-next 2/3] flow_offload: restore ability to collect separate stats per action

[Jamal Hadi Salim]

On 2019-05-08 1:07 p.m., Edward Cree wrote:
> On 08/05/2019 15:02, Jamal Hadi Salim wrote:
>> The lazy thing most people have done is essentially assume that
>> there is a stat per filter rule...
>> I wouldnt call it the 'the right thing'
> Yup, that's why I'm trying to not do that ;-)

Thank you ;->

> 
>> Yes, the index at tc semantics level is per-action type.
>> So "mirred index 1" and "drop index 1" are not the same stats counter.
> Ok, then that kills the design I used here that relied entirely on the
>   index to specify counters.
> I guess instead I'll have to go with the approach Pablo suggested,
>   passing an array of struct flow_stats in the callback, thus using
>   the index into that array (which corresponds to the index in
>   f->exts->actions) to identify different counters.
> Which means I will have to change all the existing drivers, which will
>   largely revert (from the drivers' perspective) the change when Pablo
>   took f->exts away from them — they will go back to calling something
>   that looks a lot like tcf_exts_stats_update().
> However, that'll mean the API has in-tree users, so it might be
>   considered mergeable(?)

I would say yes, but post the patches and lets have the stakeholders
chime in.
Would it be simpler to just restore the f->exts?


cheers,
jamal