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

Date:   Mon, 29 Apr 2019 17:25:10 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
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 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;
 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.
I'll put an RFC patch together soonish if no objections.

-Ed