Re: [PATCHv7 3/3] vhost_net: a kernel-level virtio server

[Gregory Haskins]

Eric Dumazet wrote:
> Gregory Haskins a écrit :
>> Gregory Haskins wrote:
>>> Eric Dumazet wrote:
>>>> Michael S. Tsirkin a écrit :

>>>> using rcu_dereference() and mutex_lock() at the same time seems wrong, I suspect
>>>> that your use of RCU is not correct.
>>>>
>>>> 1) rcu_dereference() should be done inside a read_rcu_lock() section, and
>>>>    we are not allowed to sleep in such a section.
>>>>    (Quoting Documentation/RCU/whatisRCU.txt :
>>>>      It is illegal to block while in an RCU read-side critical section, )
>>>>
>>>> 2) mutex_lock() can sleep (ie block)
>>>>
>>> Michael,
>>>   I warned you that this needed better documentation ;)
>>>
>>> Eric,
>>>   I think I flagged this once before, but Michael convinced me that it
>>> was indeed "ok", if but perhaps a bit unconventional.  I will try to
>>> find the thread.
>>>
>>> Kind Regards,
>>> -Greg
>>>
>> Here it is:
>>
>> http://lkml.org/lkml/2009/8/12/173
>>
> 
> Yes, this doesnt convince me at all, and could be a precedent for a wrong RCU use.
> People wanting to use RCU do a grep on kernel sources to find how to correctly
> use RCU.
> 
> Michael, please use existing locking/barrier mechanisms, and not pretend to use RCU.

Yes, I would tend to agree with you.  In fact, I think I suggested that
a normal barrier should be used instead of abusing rcu_dereference().

But as far as his code is concerned, I think it technically works
properly, and that was my main point.  Also note that the usage
rcu_dereference+mutex_lock() are not necessarily broken, per se:  it
could be an srcu-based critical section created by the caller, for
instance.  It would be perfectly legal to sleep on the mutex if that
were the case.

To me, the bigger issue is that the rcu_dereference() without any
apparent hint of a corresponding RSCS is simply confusing as a reviewer.
 smp_rmb() (or whatever is proper in this case) is probably more
appropriate.

Kind Regards,
-Greg

Attachment: signature.asc
Description: OpenPGP digital signature