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. Best regards, Ilya Maximets.