Re: TC stats / hw offload question

From: Pablo Neira Ayuso <pablo@netfilter.org>
To: Edward Cree <ecree@solarflare.com>
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 21:14:50 +0200	[thread overview]
Message-ID: <20190429191450.kzhhe3zqnbt6f2ap@salvia> (raw)
In-Reply-To: <e30859ad-a4e9-b0fb-f37d-4e8dcf638fdb@solarflare.com>

On Mon, Apr 29, 2019 at 05:25:10PM +0100, Edward Cree wrote:
> On 29/04/2019 16:21, Pablo Neira Ayuso wrote:
> > On Mon, Apr 29, 2019 at 03:11:06PM +0100, Edward Cree wrote:
> >> 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?
> > You could extend struct flow_stats to pass an array of stats to the
> > driver, including one stats per action and the counter index. Then,
> > tcf_exts_stats_update() uses this array of stats to update per-action
> > stats.
> Yes, but that means allocating the flow_stats.stats array each time;

We use the stack to attach a reasonable size array, eg. 16 actions,
otherwise fall back to kmalloc(). I haven't seen any driver in the
tree supporting more than that so far.

>  I'd rather avoid memory allocation unless it's necessary.  As long as
>  we can move the preempt_disable() inside the loop that's currently in
>  tcf_exts_stats_update() (i.e. only disable pre-emption across each
>  individual call to tcf_action_stats_update()) I think we can.
> I think I prefer my approach (ask for one tcfa_index at a time); but
>  unmodified drivers that don't look at the passed index would return
>  zeroes for actions after the first, so we'll need some way to handle
>  those drivers separately (e.g. one tc_setup_cb_call with "answer
>  this one if you don't do indices" and a bunch more with specified
>  index values).  I think that requires much less change to the
>  existing drivers than putting an array back in the API, and keeps as
>  much of the work as possible in the core where it won't have to be
>  replicated in every driver.

That's all right. This chunk update will not be particularly large, so
we can change it anytime in the future.

> I'll put an RFC patch together soonish if no objections.

Sure, thanks.