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:

quoted

On 2021-12-12 04:00, Baowen Zheng wrote:

[..]

quoted

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.

quoted

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.

quoted

quoted

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.

quoted

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