From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefano Brivio Subject: Re: [PATCH net-next 16/17] net: sched: conditionally take rtnl lock on rules update path Date: Tue, 13 Nov 2018 16:53:07 +0100 Message-ID: <20181113165307.45987362@redhat.com> References: <1542009346-23780-1-git-send-email-vladbu@mellanox.com> <1542009346-23780-17-git-send-email-vladbu@mellanox.com> <20181113104016.76d12436@redhat.com> <20181113144035.03e3e278@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: "netdev@vger.kernel.org" , "jhs@mojatatu.com" , "xiyou.wangcong@gmail.com" , "jiri@resnulli.us" , "davem@davemloft.net" , "ast@kernel.org" , "daniel@iogearbox.net" To: Vlad Buslov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:32988 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731439AbeKNBv4 (ORCPT ); Tue, 13 Nov 2018 20:51:56 -0500 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Tue, 13 Nov 2018 13:58:05 +0000 Vlad Buslov wrote: > On Tue 13 Nov 2018 at 13:40, Stefano Brivio wrote: > > On Tue, 13 Nov 2018 13:25:52 +0000 > > Vlad Buslov wrote: > > > >> On Tue 13 Nov 2018 at 09:40, Stefano Brivio wrote: > >> > Hi Vlad, > >> > > >> > On Mon, 12 Nov 2018 09:55:45 +0200 > >> > Vlad Buslov wrote: > >> > > >> >> @@ -179,9 +179,25 @@ static void tcf_proto_destroy_work(struct work_struct *work) > >> >> rtnl_unlock(); > >> >> } > >> >> > >> >> +/* Helper function to lock rtnl mutex when specified condition is true and mutex > >> >> + * hasn't been locked yet. Will set rtnl_held to 'true' before taking rtnl lock. > >> >> + * Note that this function does nothing if rtnl is already held. This is > >> >> + * intended to be used by cls API rules update API when multiple conditions > >> >> + * could require rtnl lock and its state needs to be tracked to prevent trying > >> >> + * to obtain lock multiple times. > >> >> + */ > >> >> + > >> >> +static void tcf_require_rtnl(bool cond, bool *rtnl_held) > >> >> +{ > >> >> + if (!*rtnl_held && cond) { > >> >> + *rtnl_held = true; > >> >> + rtnl_lock(); > >> >> + } > >> >> +} > >> > > >> > I guess calls to this function are supposed to be serialised. If that's > >> > the case (which is my tentative understanding so far), I would indicate > >> > that in the comment. > >> > > >> > If that's not the case, you would be introducing a race I guess. > >> > > >> > Same applies to tcf_block_release() from 17/17. > >> > >> Hi Stefano, > >> > >> Thank you for reviewing my code! > >> > >> I did not intend for this function to be serialized. First argument to > >> tcf_require_rtnl() is passed by value, and second argument is always a > >> pointer to local stack-allocated value of the caller. > > > > Yes, sorry, I haven't been terribly clear, that's what I meant by > > serialised: it won't be called concurrently with the same *rtnl_held. > > > > Perhaps the risk that somebody uses it that way is close to zero, so > > I'm not even too sure this is worth a comment, but if you can come up > > with a concise way of saying this, that would be nice. > > I considered my comment that function "Will set rtnl_held to 'true' > before taking rtnl lock" as a red flag for caller to not pass pointer to > a variable that can be accessed concurrently. I guess I can add > additional sentence to explicitly warn potential users. Or I can just > move rtnl_held assignment in both functions to be performed while > holding rtnl mutex. I implemented it the way I did as an overzealous > optimization, but realistically price of an assignment is negligible in > this case. But to make that effective, you would need to protect the read too, and that makes your optimisation not really overzealous I think. I'd rather go with an additional comment, if that doesn't become unreadable. -- Stefano