Re: [PATCH] virtio: remove unnecessary thread fence while reading next descriptor

[Ilya Maximets]

On 9/25/23 20:04, Ilya Maximets wrote:
> On 9/25/23 16:32, Stefan Hajnoczi wrote:
>> On Fri, 25 Aug 2023 at 13:02, Ilya Maximets <i.maximets@ovn.org> wrote:
>>>
>>> It was supposed to be a compiler barrier and it was a compiler barrier
>>> initially called 'wmb' (??) when virtio core support was introduced.
>>> Later all the instances of 'wmb' were switched to smp_wmb to fix memory
>>> ordering issues on non-x86 platforms.  However, this one doesn't need
>>> to be an actual barrier.  It's enough for it to stay a compiler barrier
>>> as its only purpose is to ensure that the value is not read twice.
>>>
>>> There is no counterpart read barrier in the drivers, AFAICT.  And even
>>> if we needed an actual barrier, it shouldn't have been a write barrier.
>>>
>>> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
>>> ---
>>>  hw/virtio/virtio.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 309038fd46..6eb8586858 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -1051,7 +1051,7 @@ static int 
>>> virtqueue_split_read_next_desc(VirtIODevice *vdev, VRingDesc *desc,
>>>      /* Check they're not leading us off end of descriptors. */
>>>      *next = desc->next;
>>
>> I don't see a caller that uses *next. Can the argument be eliminated?
> 
> Yes, it can.  The 'next' was converted from a local variable to
> an output parameter in commit:
>   412e0e81b174 ("virtio: handle virtqueue_read_next_desc() errors")
> 
> And that didn't actually make sense even then, because all the
> actual uses of the 'i/next' as an output were removed a few months
> prior in commit:
>   aa570d6fb6bd ("virtio: combine the read of a descriptor")
> 
> I can post a separate patch for this.
> 
>>
>>>      /* Make sure compiler knows to grab that: we don't want it changing! */
>>> -    smp_wmb();
>>> +    barrier();
>>
>> What is the purpose of this barrier? desc is not guest memory and
>> nothing modifies desc's fields while this function is executing. I
>> think the barrier can be removed.
> 
> True.  In fact, that was the first thing I did, but then the comment
> derailed me into thinking that it somehow can be updated concurrently,
> so I went with a safer option.  :/
> It is indeed a local variable and the barrier is not needed today.
> It had a little more sense before the previously mentioned commit:
>   aa570d6fb6bd ("virtio: combine the read of a descriptor")
> because we were reading guest memory before the barrier and used the
> result after.
> 
> I'll remove it.

Converted this into a cleanup patch set.  Posted here:
  https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg06780.html

Best regards, Ilya Maximets.