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