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

[Jamal Hadi Salim <jhs@mojatatu.com>]

On 2021-12-12 04:00, Baowen Zheng wrote:
> On December 12, 2021 3:52 AM, Jamal Hadi Salim wrote:
>> On 2021-12-09 04:28, Simon Horman wrote:
>>> include/net/act_api.h |  1 +
>>>    include/net/pkt_cls.h | 18 ++++++++++--------
>>>    net/sched/act_api.c   | 34 ++++++++++++++++++++++++++++++++++
>>>    3 files changed, 45 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/net/act_api.h b/include/net/act_api.h index
>>> 7e4e79b50216..ce094e79f722 100644
>>> --- a/include/net/act_api.h
>>> +++ b/include/net/act_api.h
>>> @@ -253,6 +253,7 @@ void tcf_action_update_stats(struct tc_action *a,
>> u64 bytes, u64 packets,
>>>    			     u64 drops, bool hw);
>>>    int tcf_action_copy_stats(struct sk_buff *, struct tc_action *,
>>> int);
>>>
>>> +int tcf_action_update_hw_stats(struct tc_action *action);
>>>    int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
>>>    			     struct tcf_chain **handle,
>>>    			     struct netlink_ext_ack *newchain); diff --git
>>> a/include/net/pkt_cls.h b/include/net/pkt_cls.h index
>>> 13f0e4a3a136..1942fe72b3e3 100644
>>> --- a/include/net/pkt_cls.h
>>> +++ b/include/net/pkt_cls.h
>>> @@ -269,18 +269,20 @@ tcf_exts_stats_update(const struct tcf_exts *exts,
>>>    #ifdef CONFIG_NET_CLS_ACT
>>>    	int i;
>>>
>>> -	preempt_disable();
>>> -
>>>    	for (i = 0; i < exts->nr_actions; i++) {
>>>    		struct tc_action *a = exts->actions[i];
>>>
>>> -		tcf_action_stats_update(a, bytes, packets, drops,
>>> -					lastuse, true);
>>> -		a->used_hw_stats = used_hw_stats;
>>> -		a->used_hw_stats_valid = used_hw_stats_valid;
>>> -	}
>>> +		/* if stats from hw, just skip */
>>> +		if (tcf_action_update_hw_stats(a)) {
>>> +			preempt_disable();
>>> +			tcf_action_stats_update(a, bytes, packets, drops,
>>> +						lastuse, true);
>>> +			preempt_enable();
>>>
>>> -	preempt_enable();
>>> +			a->used_hw_stats = used_hw_stats;
>>> +			a->used_hw_stats_valid = used_hw_stats_valid;
>>> +		}
>>> +	}
>>>    #endif
>>>    }
>>
>> Sorry - didnt quiet follow this one even after reading to the end.
>> I may have missed the obvious in the equivalence:
>> In the old code we did the preempt first then collect. The changed version only
>> does it if tcf_action_update_hw_stats() is true.
> Hi Jamal, for this change, this is because for the function of tcf_action_update_hw_stats, it will try to retrieve hw stats fron hardware. But in he process of retrieving stats information, the driver may have
> Lock or other sleeping function. So we should not call tcf_action_update_hw_stats function in context of preempt_disable.

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.

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?

> 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?

cheers,
jamal





>> cheers,
>> jamal