Message-ID: <20200226135650.GA26061@nanopsycho>
Date:   Wed, 26 Feb 2020 14:56:50 +0100
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jamal Hadi Salim <jhs@...atatu.com>
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

Wed, Feb 26, 2020 at 01:52:20PM CET, jhs@...atatu.com wrote:
>On 2020-02-25 11:22 a.m., Jiri Pirko wrote:
>> Tue, Feb 25, 2020 at 05:01:05PM CET, jhs@...atatu.com wrote:
>> > +Cc Marian.
>> > 
>
>
>
>> > 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.
>> 
>> User does not care about any "hw index". That should be abstracted out
>> by the driver.
>> 
>
>My main motivation is proper accounting (which is important
>for the billing and debugging of course). Example:
>if i say "get stats" I should know it is the sum of both
>h/w + s/w stats or the rules are clear in regards to how
>to retrieve each and sum them or differentiate them.
>If your patch takes care of summing up things etc, then i agree.

The current state implemented in the code is summing up the stats. My
patchset has no relation to that.


>Or if the rules for accounting are consistent then we are fine
>as well.
>
>> > 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.
>> 
>> We don't and when/if we do, we need to put another counter to the
>> branch point.
>> 
>
>Ok, that would work.
>> 
>> > > 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.
>> 
>> Eh, that is not that simple. The existing users are used to the fact
>> that the actions are providing counters by themselves. Having and
>> explicit counter action like this would break that expectation.
>>
>> Also, I think it should be up to the driver implementation. Some HW
>> might only support stats per rule, not the actions. Driver should fit
>> into the existing abstraction, I think it is fine.
>> 
>
>Reasonable point.
>So "count" action is only useful for h/w?

There is no "count" action and should not be.


>
>> > > 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..
>> 
>> It's not "stats", it's an information about how long ago the act was
>> used.
>
>ah. Given tc has one of those per action, are you looking to introduce
>a new "last used" action?

No.


>
>cheers,
>jamal

Powered by blists - more mailing lists