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

[Jamal Hadi Salim <jhs@mojatatu.com>]

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.