Re: TC stats / hw offload question

From: Edward Cree <ecree@solarflare.com>
To: Pablo Neira Ayuso <pablo@netfilter.org>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>,
	netdev <netdev@vger.kernel.org>, "Jiri Pirko" <jiri@resnulli.us>,
	Cong Wang <xiyou.wangcong@gmail.com>
Subject: Re: TC stats / hw offload question
Date: Mon, 29 Apr 2019 15:11:06 +0100	[thread overview]
Message-ID: <97133878-8e78-287b-9854-431b116b0788@solarflare.com> (raw)
In-Reply-To: <20190426184943.idewf2rqebvslcva@salvia>

On 26/04/2019 19:49, Pablo Neira Ayuso wrote:
> On Fri, Apr 26, 2019 at 01:13:41PM +0100, Edward Cree wrote:
>> Thus if (and only if) two TC actions have the same tcfa_index, they will
>>  share a single counter in the HW.
>> I gathered from a previous conversation with Jamal[1] that that was the
>>  correct behaviour:
>>> Note, your counters should also be shareable; example, count all
>>> the drops in one counter across multiple flows as in the following
>>> case where counter index 1 is used.
>>>
>>> tc flower match foo action drop index 1
>>> tc flower match bar action drop index 1
> The flow_action_entry structure needs a new 'counter_index' field to
> store this. The tc_setup_flow_action() function needs to be updated
> for this for the FLOW_ACTION_{ACCEPT,DROP,REDIRECT,MIRRED} cases to
> set this entry->counter_index field to tcfa_index, so the driver has
> access to this.
Hmm, I'm still not sure this solves everything.
Before, we could write
tc flower match foo \
    action mirred egress mirror eth1 index 1 \
    action mirred egress redirect eth2 index 2
and have two distinct HW counters (one of which might e.g. be shared
 with another rule).  But when reading those counters, under
 fl_hw_update_stats(), the driver only gets to return one set of flow
 stats for both actions.
Previously, the driver's TC_CLSFLOWER_STATS handler was updating the
 action stats directly, so was able to do something different for each
 action, but that's not possible in 5.1.  At stats gathering time, the
 driver doesn't even have access to anything that's per-action and
 thus could have a flow_stats member shoved in it.
AFAICT, the only reason this isn't a regression is that existing
 drivers didn't implement the old semantics correctly.
This is a bit of a mess; the best idea I've got is for the
 TC_CLSFLOWER_STATS call to include a tcfa_index.  Then the driver
 returns counter stats for that index, and tcf_exts_stats_update()
 only updates those actions whose index matches.  But then
 fl_hw_update_stats() would have to iterate over all the indices in
 f->exts.  What do you think?

-Ed