On Tue 13 Nov 2018 at 13:40, Stefano Brivio <sbri...@redhat.com> wrote:
> 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.
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 = 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.
