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

From: Jiri Pirko <jiri@resnulli.us>
To: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Edward Cree <ecree@solarflare.com>,
	Jakub Kicinski <kuba@kernel.org>,
	netdev@vger.kernel.org, davem@davemloft.net, saeedm@mellanox.com,
	leon@kernel.org, michael.chan@broadcom.com, vishal@chelsio.com,
	jeffrey.t.kirsher@intel.com, idosch@mellanox.com,
	aelior@marvell.com, peppe.cavallaro@st.com,
	alexandre.torgue@st.com, xiyou.wangcong@gmail.com,
	pablo@netfilter.org, mlxsw@mellanox.com
Subject: Re: [patch net-next 00/10] net: allow user specify TC filter HW stats type
Date: Mon, 24 Feb 2020 17:25:21 +0100	[thread overview]
Message-ID: <20200224162521.GE16270@nanopsycho> (raw)
In-Reply-To: <9cd1e555-6253-1856-f21d-43323eb77788@mojatatu.com>

Mon, Feb 24, 2020 at 04:45:57PM CET, jhs@mojatatu.com wrote:
>On 2020-02-24 8:11 a.m., Jiri Pirko wrote:
>> Mon, Feb 24, 2020 at 12:38:20PM CET, ecree@solarflare.com wrote:
>> > On 22/02/2020 06:38, Jiri Pirko wrote:
>
>[..]
>> > Potentially a user could only want stats on one action and disable them
>> >   on another.  For instance, if their action chain does delivery to the
>> >   'customer' and also mirrors the packets for monitoring, they might only
>> >   want stats on the first delivery (e.g. for billing) and not want to
>> >   waste a counter on the mirror.
>> 
>> Okay.
>> 
>
>+1  very important for telco billing use cases i am familiar with.
>
>Ancient ACL implementations that had only one filter and
>one action (drop/accept) didnt need more than one counter.
>But not anymore in my opinion.
>
>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.
You can reuse this index in multiple counter action instances.
However, in tc there is implicit separate counter for every action.

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

and we report stats from action_counter for all the_actual_actionX.

>
>> 
>> > 
>> > > Plus, if the fine grained setup is ever needed, the hw_stats could be in
>> > > future easilyt extended to be specified per-action too overriding
>> > > the per-filter setup only for specific action. What do you think?
>> > I think this is improper; the stats type should be defined per-action in
>> >   the uapi, even if userland initially only has UI to set it across the
>> >   entire filter.  (I imagine `tc action` would grow the corresponding UI
>> >   pretty quickly.)  Then on the driver side, you must determine whether
>> >   your hardware can support the user's request, and if not, return an
>> >   appropriate error code.
>> > 
>> > Let's try not to encourage people (users, future HW & driver developers)
>> >   to keep thinking of stats as per-filter entities.
>> > Okay, but in that case, it does not make sense to have "UI to set it
>> across the entire filter". The UI would have to set it per-action too.
>> Othewise it would make people think it is per-filter entity.
>> But I guess the tc cmdline is chatty already an it can take other
>> repetitive cmdline options.
>> 
>
>I normally visualize policy as a dag composed of filter(s) +
>actions. The UI and uAPI has to be able to express that.
>
>I am not sure if mlxsw works this way, but:
>Most hardware i have encountered tends to have a separate
>stats/counter table. The stats table is indexed.
>
>Going backwards and looking at your example in this stanza:
>---
>  in_hw in_hw_count 2
>  hw_stats immediate
>        action order 1: gact action drop
>         random type none pass val 0
>         index 1 ref 1 bind 1 installed 14 sec used 7 sec
>        Action statistics:
>----
>
>Guessing from "in_hw in_hw_count 2" - 2 is a hw stats table index?
>If you have enough counters in hardware, the "stats table index"
>essentially can be directly mapped to the action "index" attribute.

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

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 :)

>
>cheers,
>jamal
>
>> What do you think?
>> 
>> 
>> Thanks!
>> 
>