From mboxrd@z Thu Jan 1 00:00:00 1970 From: Davide Caratti Subject: Re: [PATCH net-next 2/2] net/sched: act_csum: don't use spinlock in the fast path Date: Fri, 22 Dec 2017 10:28:05 +0100 Message-ID: <1513934885.2913.13.camel@redhat.com> References: <16125b3f16fb9f8c068ec80ce3f6550d0465521c.1513104506.git.dcaratti@redhat.com> <20171213.162328.1374458006305010879.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: xiyou.wangcong@gmail.com, jiri@mellanox.com, netdev@vger.kernel.org To: David Miller Return-path: Received: from mx1.redhat.com ([209.132.183.28]:32804 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756149AbdLVJ2I (ORCPT ); Fri, 22 Dec 2017 04:28:08 -0500 In-Reply-To: <20171213.162328.1374458006305010879.davem@davemloft.net> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, 2017-12-13 at 16:23 -0500, David Miller wrote: > From: Davide Caratti > Date: Wed, 13 Dec 2017 10:48:38 +0100 > > > Then, in the data path, use READ_ONCE() to > > read those values, to avoid lock contention among multiple readers. > ... > > @@ -544,14 +543,12 @@ static int tcf_csum(struct sk_buff *skb, const struct tc_action *a, > > > > tcf_lastuse_update(&p->tcf_tm); > > bstats_cpu_update(this_cpu_ptr(p->common.cpu_bstats), skb); > > - spin_lock(&p->tcf_lock); > > - action = p->tcf_action; > > - update_flags = p->update_flags; > > - spin_unlock(&p->tcf_lock); > > > > + action = READ_ONCE(p->tcf_action); > > if (unlikely(action == TC_ACT_SHOT)) > > goto drop; > > > > + update_flags = READ_ONCE(p->update_flags); > > switch (tc_skb_protocol(skb)) { > > case cpu_to_be16(ETH_P_IP): > > if (!tcf_csum_ipv4(skb, update_flags)) hi David, thank you for replying! > That's not why the lock is here. > > We must read both action and flags atomically so that they are consistent > with eachother. > > We must never use action from one configuration change and flags from > yet another. I was (erroneously) assuming that such behavior was acceptable, since it's present almost in all other TC actions, even those where tcf_lock is used. But agree, it's better not to introduce a race in a place where it's not present. > Find a way to load both of these values with a single cpu load, then you > can legally remove the lock. act_tunnel_key seems a good example for this, I will send a v2 soon. -- davide