virtio: remove unnecessary thread fence while reading next descriptor

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

Posted by Ilya Maximets 1 year, 3 months ago

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;
     /* Make sure compiler knows to grab that: we don't want it changing! */
-    smp_wmb();
+    barrier();
 
     if (*next >= max) {
         virtio_error(vdev, "Desc next is %u", *next);
-- 
2.40.1

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

Posted by Stefan Hajnoczi 1 year, 2 months ago

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?

>      /* 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.

>
>      if (*next >= max) {
>          virtio_error(vdev, "Desc next is %u", *next);
> --
> 2.40.1
>
>

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

Posted by Ilya Maximets 1 year, 2 months ago

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.

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

Posted by Ilya Maximets 1 year, 2 months ago

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.

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

Posted by Michael S. Tsirkin 1 year, 2 months ago

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

Ugh, these archives are useless. use lore please. 

-- 
MST

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

Posted by Ilya Maximets 1 year, 2 months ago

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.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.
> 
> 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.maximets@ovn.org/

Best regards, Ilya Maximets.

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

Posted by Ilya Maximets 1 year, 2 months ago

On 8/25/23 19:01, Ilya Maximets 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>
> ---

Kind reminder.

Best regards, Ilya Maximets.

>  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;
>      /* Make sure compiler knows to grab that: we don't want it changing! */
> -    smp_wmb();
> +    barrier();
>  
>      if (*next >= max) {
>          virtio_error(vdev, "Desc next is %u", *next);