| Message ID | 20230825170136.1953236-1-i.maximets@ovn.org |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: remove unnecessary thread fence while reading next descriptor | expand |
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);
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 > >
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.
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.
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.
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.
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);
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(-)