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 14:40:35 +0100 Message-ID: <20181113144035.03e3e278@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> 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]:56036 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1732728AbeKMXiy (ORCPT ); Tue, 13 Nov 2018 18:38:54 -0500 In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 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. > Same applies to tcf_block_release() - its arguments are Qdisc and block > which support concurrency-safe reference counting, and pointer to local > variable rtnl_held, which is not accessible to concurrent users. Same there. > What is the race in these cases? Am I missing something? No, no race then. My only concern was: thread A: thread B: - x = false; - tcf_require_rtnl(true, &x); - tcf_require_rtnl(true, &x); - if (!*x && true) - if (!*x && true) - *x = true; - rtnl_lock() - *x = true; - rtnl_lock() but this cannot happen as you explained. -- Stefano