On Tue, 13 Nov 2018 13:25:52 +0000
Vlad Buslov <vla...@mellanox.com> wrote:
> On Tue 13 Nov 2018 at 09:40, Stefano Brivio <sbri...@redhat.com> wrote:
> > Hi Vlad,
> >
> > On Mon, 12 Nov 2018 09:55:45 +0200
> > Vlad Buslov <vla...@mellanox.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.
> 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
