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: <4a909bf2-a7a1-67f9-2d62-d6858d3553b9@mojatatu.com>
Date:   Tue, 14 Dec 2021 07:01: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-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