From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Buslov Subject: Re: [PATCH net-next 16/17] net: sched: conditionally take rtnl lock on rules update path Date: Tue, 13 Nov 2018 13:58:05 +0000 Message-ID: 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="iso-8859-1" Content-Transfer-Encoding: quoted-printable 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: Stefano Brivio Return-path: Received: from mail-eopbgr80048.outbound.protection.outlook.com ([40.107.8.48]:48812 "EHLO EUR04-VI1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1732826AbeKMX41 (ORCPT ); Tue, 13 Nov 2018 18:56:27 -0500 In-Reply-To: <20181113144035.03e3e278@redhat.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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: >> > =20 >> >> @@ -179,9 +179,25 @@ static void tcf_proto_destroy_work(struct work_s= truct *work) >> >> rtnl_unlock(); >> >> } >> >> =20 >> >> +/* Helper function to lock rtnl mutex when specified condition is tr= ue and mutex >> >> + * hasn't been locked yet. Will set rtnl_held to 'true' before takin= g rtnl lock. >> >> + * Note that this function does nothing if rtnl is already held. Thi= s is >> >> + * intended to be used by cls API rules update API when multiple con= ditions >> >> + * could require rtnl lock and its state needs to be tracked to prev= ent trying >> >> + * to obtain lock multiple times. >> >> + */ >> >> + >> >> +static void tcf_require_rtnl(bool cond, bool *rtnl_held) >> >> +{ >> >> + if (!*rtnl_held && cond) { >> >> + *rtnl_held =3D true; >> >> + rtnl_lock(); >> >> + } >> >> +} =20 >> > >> > 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 indicat= e >> > 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. =20 >>=20 >> Hi Stefano, >>=20 >> Thank you for reviewing my code! >>=20 >> 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. Suggestions are welcome! > >> 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 =3D false; > - tcf_require_rtnl(true, &x); - tcf_require_rtnl(true, &x); > - if (!*x && true) - if (!*x && true) > - *x =3D true; > - rtnl_lock() - *x =3D true; > - rtnl_lock() > > but this cannot happen as you explained.