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