netdev - Re: [PATCH v6 net-next 08/12] flow_offload: add process to update action stats from hardware

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]

Message-ID: <06a6d105-0661-81c1-350a-17d964931dc5@mojatatu.com>
Date:   Tue, 14 Dec 2021 09:29:15 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Baowen Zheng <baowen.zheng@...igine.com>,
        Simon Horman <simon.horman@...igine.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:     Cong Wang <xiyou.wangcong@...il.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Ido Schimmel <idosch@...dia.com>,
        Jiri Pirko <jiri@...nulli.us>, Oz Shlomo <ozsh@...dia.com>,
        Roi Dayan <roid@...dia.com>, Vlad Buslov <vladbu@...dia.com>,
        Louis Peens <louis.peens@...igine.com>,
        oss-drivers <oss-drivers@...igine.com>
Subject: Re: [PATCH v6 net-next 08/12] flow_offload: add process to update
 action stats from hardware

On 2021-12-14 08:43, Baowen Zheng wrote:
> On December 14, 2021 8:01 PM, Jamal Hadi Salim wrote:
>> On 2021-12-12 04:00, Baowen Zheng wrote:

[..]
>>
>> Still confused probably because this is one of those functions that are badly
>> named. Initially i thought that it was useful to call this function for both
>> offloaded vs non-offloaded stats. But it seems it is only useful for hw
>> offloaded stats? If so, please consider a patch for renaming this appropriately
>> for readability.
> Yes, it is only for hw offload stats and is used to sync the stats information from the
> Offloaded filter to the actions the filter referred to.
> 
> We will consider to add a patch to rename this function for readability.
>>
>> Regardless, two things:
>>
>> 1) In the old code the last two lines
>> +			a->used_hw_stats = used_hw_stats;
>> +			a->used_hw_stats_valid = used_hw_stats_valid;
>> inside the preempt check and with this they are outside.
>>
>> This is fine if the only reason we have this function is for h/w offload.
>>
>> 2) You introduced tcf_action_update_hw_stats() which also does preempt
>> disable/enable and seems to repeat some of the things you are doing as well
>> in this function?
> As I mentioned above, the function of tcf_exts_stats_update is used to sync the stats
> information from the offloaded filter to the actions the filter referred to.
> Then the new added function tcf_action_update_hw_stats() is used to sync the stats
> Information from the hw device that offloads this action.  So if the action is offloaded
> to hw as a single action, then it will not sync the stats from the hw filter.
>>
>>> Actually, since there is no vendor to support update single action stats from
>> hardware, so it is not obvious, we will post our implement support after these
>> patches set.
>>> Do you think if it make sense?
>>
>> Since you plan to have more patches:
>> If it doesnt affect your current goals then i would suggest you leave it to later.
>> The question is, with what you already have in this patchset, do we get
>> something functional and standalone?
>>
> What we will post later to support update single action stats from hardware is code for driver side,
> It will mainly implement the flow_offload_act_command of stats an action from hw in driver.
> 
> So i think it will proper to post the whole framework code in act_api and cls_api in this series.
> Then when we post the driver patch, we will not need to change the act/cls implement.
> 
> WDYT?

ok.

cheers,
jamal