netdev - Re: [PATCH net-next 2/3] net/sched: act_connmark: transition to percpu stats and rcu

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] [day] [month] [year] [list]

Date:   Tue, 14 Feb 2023 11:31:20 -0300
From:   Pedro Tammela <pctammela@...atatu.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     jhs@...atatu.com, xiyou.wangcong@...il.com, jiri@...nulli.us,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org
Subject: Re: [PATCH net-next 2/3] net/sched: act_connmark: transition to
 percpu stats and rcu

On 14/02/2023 06:03, Paolo Abeni wrote:
> On Fri, 2023-02-10 at 17:27 -0300, Pedro Tammela wrote:
>> The tc action act_connmark was using shared stats and taking the per
>> action lock in the datapath. Improve it by using percpu stats and rcu.
>>
>> perf before:
>> - 13.55% tcf_connmark_act
>>     - 81.18% _raw_spin_lock
>>         80.46% native_queued_spin_lock_slowpath
>>
>> perf after:
>> - 3.12% tcf_connmark_act
>>
>> tdc results:
>> 1..15
>> ok 1 2002 - Add valid connmark action with defaults
>> ok 2 56a5 - Add valid connmark action with control pass
>> ok 3 7c66 - Add valid connmark action with control drop
>> ok 4 a913 - Add valid connmark action with control pipe
>> ok 5 bdd8 - Add valid connmark action with control reclassify
>> ok 6 b8be - Add valid connmark action with control continue
>> ok 7 d8a6 - Add valid connmark action with control jump
>> ok 8 aae8 - Add valid connmark action with zone argument
>> ok 9 2f0b - Add valid connmark action with invalid zone argument
>> ok 10 9305 - Add connmark action with unsupported argument
>> ok 11 71ca - Add valid connmark action and replace it
>> ok 12 5f8f - Add valid connmark action with cookie
>> ok 13 c506 - Replace connmark with invalid goto chain control
>> ok 14 6571 - Delete connmark action with valid index
>> ok 15 3426 - Delete connmark action with invalid index
>>
>> Reviewed-by: Jamal Hadi Salim <jhs@...atatu.com>
>> Signed-off-by: Pedro Tammela <pctammela@...atatu.com>
>> ---
>>   include/net/tc_act/tc_connmark.h |   9 ++-
>>   net/sched/act_connmark.c         | 109 ++++++++++++++++++++-----------
>>   2 files changed, 77 insertions(+), 41 deletions(-)
>>
>> diff --git a/include/net/tc_act/tc_connmark.h b/include/net/tc_act/tc_connmark.h
>> index 1f4cb477b..e8dd77a96 100644
>> --- a/include/net/tc_act/tc_connmark.h
>> +++ b/include/net/tc_act/tc_connmark.h
>> @@ -4,10 +4,15 @@
>>   
>>   #include <net/act_api.h>
>>   
>> -struct tcf_connmark_info {
>> -	struct tc_action common;
>> +struct tcf_connmark_parms {
>>   	struct net *net;
>>   	u16 zone;
>> +	struct rcu_head rcu;
>> +};
>> +
>> +struct tcf_connmark_info {
>> +	struct tc_action common;
>> +	struct tcf_connmark_parms __rcu *parms;
>>   };
>>   
>>   #define to_connmark(a) ((struct tcf_connmark_info *)a)
>> diff --git a/net/sched/act_connmark.c b/net/sched/act_connmark.c
>> index 7e63ff7e3..541e1c556 100644
>> --- a/net/sched/act_connmark.c
>> +++ b/net/sched/act_connmark.c
>> @@ -36,13 +36,15 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
>>   	struct nf_conntrack_tuple tuple;
>>   	enum ip_conntrack_info ctinfo;
>>   	struct tcf_connmark_info *ca = to_connmark(a);
>> +	struct tcf_connmark_parms *parms;
>>   	struct nf_conntrack_zone zone;
>>   	struct nf_conn *c;
>>   	int proto;
>>   
>> -	spin_lock(&ca->tcf_lock);
>>   	tcf_lastuse_update(&ca->tcf_tm);
>> -	bstats_update(&ca->tcf_bstats, skb);
>> +	tcf_action_update_bstats(&ca->common, skb);
>> +
>> +	parms = rcu_dereference_bh(ca->parms);
>>   
>>   	switch (skb_protocol(skb, true)) {
>>   	case htons(ETH_P_IP):
>> @@ -64,31 +66,31 @@ TC_INDIRECT_SCOPE int tcf_connmark_act(struct sk_buff *skb,
>>   	c = nf_ct_get(skb, &ctinfo);
>>   	if (c) {
>>   		skb->mark = READ_ONCE(c->mark);
>> -		/* using overlimits stats to count how many packets marked */
>> -		ca->tcf_qstats.overlimits++;
>> -		goto out;
>> +		goto count;
>>   	}
>>   
>> -	if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb),
>> -			       proto, ca->net, &tuple))
>> +	if (!nf_ct_get_tuplepr(skb, skb_network_offset(skb), proto, parms->net,
>> +			       &tuple))
>>   		goto out;
>>   
>> -	zone.id = ca->zone;
>> +	zone.id = parms->zone;
>>   	zone.dir = NF_CT_DEFAULT_ZONE_DIR;
>>   
>> -	thash = nf_conntrack_find_get(ca->net, &zone, &tuple);
>> +	thash = nf_conntrack_find_get(parms->net, &zone, &tuple);
>>   	if (!thash)
>>   		goto out;
>>   
>>   	c = nf_ct_tuplehash_to_ctrack(thash);
>> -	/* using overlimits stats to count how many packets marked */
>> -	ca->tcf_qstats.overlimits++;
>>   	skb->mark = READ_ONCE(c->mark);
>>   	nf_ct_put(c);
>>   
>> -out:
>> +count:
>> +	/* using overlimits stats to count how many packets marked */
>> +	spin_lock(&ca->tcf_lock);
>> +	ca->tcf_qstats.overlimits++;
>>   	spin_unlock(&ca->tcf_lock);
> 
> I think above you could use tcf_action_inc_overlimit_qstats() and avoid
> acquiring the spin lock in most cases.
> 
> Side note: it looks like pedit could use a similar change, too - sorry
> for missing that point before.
> 

Oh, indeed! I will respin and add the pedit change in this series as well.
Many thanks.