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

[Michael S. Tsirkin]

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