netdev - Re: [patch net-next 00/10] net: allow user specify TC filter HW stats type

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Feb 2020 07:52:20 -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

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.
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?

>>> 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?

cheers,
jamal