On 9/27/23 17:41, Michael S. Tsirkin wrote:
> On Wed, Sep 27, 2023 at 04:06:41PM +0200, Ilya Maximets wrote:
>> 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.
>
> Ugh, these archives are useless. use lore please.
>
OK. The lore link is the following:
https://lore.kernel.org/qemu-devel/20230927140016.2317404-1-i.maxim...@ovn.org/
Best regards, Ilya Maximets.
