[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] virtio: remove unnecessary thread fence while reading next d
From: |
Ilya Maximets |
Subject: |
Re: [PATCH] virtio: remove unnecessary thread fence while reading next descriptor |
Date: |
Mon, 25 Sep 2023 20:04:03 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 |
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.
Best regards, Ilya Maximets.