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:25:52 +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> 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-eopbgr30048.outbound.protection.outlook.com ([40.107.3.48]:18704 "EHLO EUR03-AM5-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1732728AbeKMXYE (ORCPT ); Tue, 13 Nov 2018 18:24:04 -0500 In-Reply-To: <20181113104016.76d12436@redhat.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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_stru= ct *work) >> rtnl_unlock(); >> } >> =20 >> +/* 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 r= tnl lock. >> + * Note that this function does nothing if rtnl is already held. This i= s >> + * intended to be used by cls API rules update API when multiple condit= ions >> + * 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 =3D 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. 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. What is the race in these cases? Am I missing something? Vlad