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. 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
