Message-ID: <b93272f2-f76c-10b5-1c2a-6d39e917ffd6@mojatatu.com>
Date:   Tue, 25 Feb 2020 11:01:05 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Edward Cree <ecree@...arflare.com>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        davem@...emloft.net, saeedm@...lanox.com, leon@...nel.org,
        michael.chan@...adcom.com, vishal@...lsio.com,
        jeffrey.t.kirsher@...el.com, idosch@...lanox.com,
        aelior@...vell.com, peppe.cavallaro@...com,
        alexandre.torgue@...com, xiyou.wangcong@...il.com,
        pablo@...filter.org, mlxsw@...lanox.com,
        Marian Pritsak <marianp@...lanox.com>
Subject: Re: [patch net-next 00/10] net: allow user specify TC filter HW stats
 type

+Cc Marian.

On 2020-02-24 11:25 a.m., Jiri Pirko wrote:
> Mon, Feb 24, 2020 at 04:45:57PM CET, jhs@...atatu.com wrote:
>> On 2020-02-24 8:11 a.m., Jiri Pirko wrote:
>>> Mon, Feb 24, 2020 at 12:38:20PM CET, ecree@...arflare.com wrote:
>>>> On 22/02/2020 06:38, Jiri Pirko wrote:
>>

>> There's also a requirement for the concept of "sharing" - think
>> "family plans" or "small bussiness plan".
>> Counters may be shared across multiple filter-action chains for example.
> 
> In hardware, we have a separate "counter" action with counter index.

Ok, so it is similar semantics.
In your case, you abstract it as a speacial action, but in most
abstractions(including P4) it looks like an indexed table.
 From a tc perspective you could abstract the equivalent to
your "counter action" as a gact "ok" or "pipe",etc depending
on your policy goal. The counter index becomes the gact index
if there is no conflict.
In most actions "index" attribute is really mapped to a
"counter" index. Exception would be actions with state
(like policer).

> You can reuse this index in multiple counter action instances.

That is great because it maps to tc semantics. When you create
an action of the same type, you can specify the index and it
is re-used. Example:

sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \
match ip dst 127.0.0.8/32 flowid 1:8 \
action vlan push id 8 protocol 802.1q index 8\
action mirred egress mirror dev eth0 index 111

sudo tc filter add dev lo parent ffff: protocol ip prio 8 u32 \
match ip dst 127.0.0.15/32 flowid 1:10 \
action vlan push id 15 protocol 802.1q index 15 \
action mirred egress mirror index 111 \
action drop index 111

So for the shared mirror action the counter is shared
by virtue of specifying index 111.

What tc _doesnt allow_ is to re-use the same
counter index across different types of actions (example
mirror index 111 is not the same instance as drop 111).
Thats why i was asking if you are exposing the hw index.

> However, in tc there is implicit separate counter for every action.
> 

Yes, and is proving to be a challenge for hw. In s/w it makes sense.
It seemed sensible at the time; existing hardware back then
(broadcom 5691 family and cant remember the other vendor, iirc)
hard coded the actions with counters. Mind you they would
only support one action per match.

Some rethinking is needed of this status quo.
So maybe having syntaticaly an index for s/w vs h/w may make
sense.
Knowing the indices is important. As an example, for telemetry
you may be very interesting in dumping only the counter stats
instead of the rule. Dumping gact has always made it easy in
my use cases because it doesnt have a lot of attributes. But it
could make sense to introduce a new semantic like "dump action stats .."

> The counter is limited resource. So we overcome this mismatch in mlxsw
> by having action "counter" always first for every rule inserted:
> rule->action_counter,the_actual_action,the_actual_action2,...the_actual_actionN
>

So i am guessing the hw cant support "branching" i.e based on in
some action state sometime you may execute action foo and other times
action bar. Those kind of scenarios would need multiple counters.
> and we report stats from action_counter for all the_actual_actionX.

This may not be accurate if you are branching - for example
a policer or quota enforcer which either accepts or drops or sends next
to a marker action etc .
IMO, this was fine in the old days when you had one action per match.
Best is to leave it to whoever creates the policy to decide what to
count. IOW, I think modelling it as a pipe or ok or drop or continue
and be placed anywhere in the policy graph instead of the begining.

>> Sharing then becomes a matter of specifying the same drop action
>> with the correct index across multiple filters.
>>
>> If you dont have enough hw counters - then perhaps a scheme to show
>> separate hardware counter index and software counter index (aka action
>> index) is needed.
> 
> I don't, that is the purpose of this patchset, to be able to avoid the
> "action_counter" from the example I wrote above.

IMO, it would make sense to reuse existing gact for example and
annotate s/w vs h/w indices as a starting point. It keeps the
existing approach intact.

> Note that I don't want to share, there is still separate "last_hit"
> record in hw I expose in "used X sec". Interestingly enough, in
> Spectrum-1 this is per rule, in Spectrum-2,3 this is per action block :)

I didnt understand this one..

cheers,
jamal

Powered by blists - more mailing lists