netdev - Re: TC stats / hw offload question

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] [day] [month] [year] [list]

Message-ID: <20190429191450.kzhhe3zqnbt6f2ap@salvia>
Date:   Mon, 29 Apr 2019 21:14:50 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Edward Cree <ecree@...arflare.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>,
        netdev <netdev@...r.kernel.org>, Jiri Pirko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: TC stats / hw offload question

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.