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

From: Jamal Hadi Salim <jhs@mojatatu.com>
To: Baowen Zheng <baowen.zheng@corigine.com>,
	Simon Horman <simon.horman@corigine.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: Cong Wang <xiyou.wangcong@gmail.com>,
	Dan Carpenter <dan.carpenter@oracle.com>,
	Ido Schimmel <idosch@nvidia.com>, Jiri Pirko <jiri@resnulli.us>,
	Oz Shlomo <ozsh@nvidia.com>, Roi Dayan <roid@nvidia.com>,
	Vlad Buslov <vladbu@nvidia.com>,
	Louis Peens <louis.peens@corigine.com>,
	oss-drivers <oss-drivers@corigine.com>
Subject: Re: [PATCH v6 net-next 08/12] flow_offload: add process to update action stats from hardware
Date: Tue, 14 Dec 2021 09:29:15 -0500	[thread overview]
Message-ID: <06a6d105-0661-81c1-350a-17d964931dc5@mojatatu.com> (raw)
In-Reply-To: <DM5PR1301MB2172C97E25E66520DEC901CFE7759@DM5PR1301MB2172.namprd13.prod.outlook.com>

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