[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20181113165307.45987362@redhat.com>
Date: Tue, 13 Nov 2018 16:53:07 +0100
From: Stefano Brivio <sbrivio@...hat.com>
To: Vlad Buslov <vladbu@...lanox.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"jhs@...atatu.com" <jhs@...atatu.com>,
"xiyou.wangcong@...il.com" <xiyou.wangcong@...il.com>,
"jiri@...nulli.us" <jiri@...nulli.us>,
"davem@...emloft.net" <davem@...emloft.net>,
"ast@...nel.org" <ast@...nel.org>,
"daniel@...earbox.net" <daniel@...earbox.net>
Subject: Re: [PATCH net-next 16/17] net: sched: conditionally take rtnl lock
on rules update path
On Tue, 13 Nov 2018 13:58:05 +0000
Vlad Buslov <vladbu@...lanox.com> wrote:
> On Tue 13 Nov 2018 at 13:40, Stefano Brivio <sbrivio@...hat.com> wrote:
> > On Tue, 13 Nov 2018 13:25:52 +0000
> > Vlad Buslov <vladbu@...lanox.com> wrote:
> >
> >> On Tue 13 Nov 2018 at 09:40, Stefano Brivio <sbrivio@...hat.com> wrote:
> >> > Hi Vlad,
> >> >
> >> > On Mon, 12 Nov 2018 09:55:45 +0200
> >> > Vlad Buslov <vladbu@...lanox.com> 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
Powered by blists - more mailing lists