* [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg
@ 2018-04-19 17:35 Michael S. Tsirkin
2018-04-19 17:39 ` Paolo Bonzini
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2018-04-19 17:35 UTC (permalink / raw)
To: linux-kernel
Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Jason Wang,
virtualization, Paolo Bonzini
virtio is using barriers to order memory accesses, thus
dma_wmb/rmb is a good match.
Build-tested on x86: Before
[mst@tuck linux]$ size drivers/virtio/virtio_ring.o
text data bss dec hex filename
11392 820 0 12212 2fb4 drivers/virtio/virtio_ring.o
After
mst@tuck linux]$ size drivers/virtio/virtio_ring.o
text data bss dec hex filename
11284 820 0 12104 2f48 drivers/virtio/virtio_ring.o
Cc: Ohad Ben-Cohen <ohad@wizery.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: linux-remoteproc@vger.kernel.org
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
It's good in theory, but could one of RPMSG maintainers please review
and ack this patch? Or even better test it?
All these barriers are useless on Intel anyway ...
include/linux/virtio_ring.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index bbf3252..fab0213 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -35,7 +35,7 @@ static inline void virtio_rmb(bool weak_barriers)
if (weak_barriers)
virt_rmb();
else
- rmb();
+ dma_rmb();
}
static inline void virtio_wmb(bool weak_barriers)
@@ -43,7 +43,7 @@ static inline void virtio_wmb(bool weak_barriers)
if (weak_barriers)
virt_wmb();
else
- wmb();
+ dma_wmb();
}
static inline void virtio_store_mb(bool weak_barriers,
--
MST
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg
2018-04-19 17:35 [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg Michael S. Tsirkin
@ 2018-04-19 17:39 ` Paolo Bonzini
2018-04-19 17:46 ` Michael S. Tsirkin
2018-04-20 3:04 ` Jason Wang
2018-06-04 20:02 ` Bjorn Andersson
2 siblings, 1 reply; 9+ messages in thread
From: Paolo Bonzini @ 2018-04-19 17:39 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc, Jason Wang,
virtualization
On 19/04/2018 19:35, Michael S. Tsirkin wrote:
> virtio is using barriers to order memory accesses, thus
> dma_wmb/rmb is a good match.
>
> Build-tested on x86: Before
>
> [mst@tuck linux]$ size drivers/virtio/virtio_ring.o
> text data bss dec hex filename
> 11392 820 0 12212 2fb4 drivers/virtio/virtio_ring.o
>
> After
> mst@tuck linux]$ size drivers/virtio/virtio_ring.o
> text data bss dec hex filename
> 11284 820 0 12104 2f48 drivers/virtio/virtio_ring.o
>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: linux-remoteproc@vger.kernel.org
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> It's good in theory, but could one of RPMSG maintainers please review
> and ack this patch? Or even better test it?
>
> All these barriers are useless on Intel anyway ...
This should be okay, but I wonder if there should be a virtio_wmb(...)
or an "if (weak_barriers) wmb()" before the "writel" in vm_notify
(drivers/virtio/virtio_mmio.c).
Thanks,
Paolo
>
> include/linux/virtio_ring.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> index bbf3252..fab0213 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -35,7 +35,7 @@ static inline void virtio_rmb(bool weak_barriers)
> if (weak_barriers)
> virt_rmb();
> else
> - rmb();
> + dma_rmb();
> }
>
> static inline void virtio_wmb(bool weak_barriers)
> @@ -43,7 +43,7 @@ static inline void virtio_wmb(bool weak_barriers)
> if (weak_barriers)
> virt_wmb();
> else
> - wmb();
> + dma_wmb();
> }
>
> static inline void virtio_store_mb(bool weak_barriers,
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg
2018-04-19 17:39 ` Paolo Bonzini
@ 2018-04-19 17:46 ` Michael S. Tsirkin
2018-04-19 17:48 ` Paolo Bonzini
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2018-04-19 17:46 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc,
Jason Wang, virtualization
On Thu, Apr 19, 2018 at 07:39:21PM +0200, Paolo Bonzini wrote:
> On 19/04/2018 19:35, Michael S. Tsirkin wrote:
> > virtio is using barriers to order memory accesses, thus
> > dma_wmb/rmb is a good match.
> >
> > Build-tested on x86: Before
> >
> > [mst@tuck linux]$ size drivers/virtio/virtio_ring.o
> > text data bss dec hex filename
> > 11392 820 0 12212 2fb4 drivers/virtio/virtio_ring.o
> >
> > After
> > mst@tuck linux]$ size drivers/virtio/virtio_ring.o
> > text data bss dec hex filename
> > 11284 820 0 12104 2f48 drivers/virtio/virtio_ring.o
> >
> > Cc: Ohad Ben-Cohen <ohad@wizery.com>
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: linux-remoteproc@vger.kernel.org
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > It's good in theory, but could one of RPMSG maintainers please review
> > and ack this patch? Or even better test it?
> >
> > All these barriers are useless on Intel anyway ...
>
> This should be okay, but I wonder if there should be a virtio_wmb(...)
> or an "if (weak_barriers) wmb()" before the "writel" in vm_notify
> (drivers/virtio/virtio_mmio.c).
>
> Thanks,
>
> Paolo
That one uses weak barriers AFAIK.
IIUC you mean rproc_virtio_notify.
I suspect it works because specific kick callbacks have a barrier internally.
> >
> > include/linux/virtio_ring.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> > index bbf3252..fab0213 100644
> > --- a/include/linux/virtio_ring.h
> > +++ b/include/linux/virtio_ring.h
> > @@ -35,7 +35,7 @@ static inline void virtio_rmb(bool weak_barriers)
> > if (weak_barriers)
> > virt_rmb();
> > else
> > - rmb();
> > + dma_rmb();
> > }
> >
> > static inline void virtio_wmb(bool weak_barriers)
> > @@ -43,7 +43,7 @@ static inline void virtio_wmb(bool weak_barriers)
> > if (weak_barriers)
> > virt_wmb();
> > else
> > - wmb();
> > + dma_wmb();
> > }
> >
> > static inline void virtio_store_mb(bool weak_barriers,
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg
2018-04-19 17:46 ` Michael S. Tsirkin
@ 2018-04-19 17:48 ` Paolo Bonzini
2018-04-19 18:10 ` Michael S. Tsirkin
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Paolo Bonzini @ 2018-04-19 17:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel, Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc,
Jason Wang, virtualization
On 19/04/2018 19:46, Michael S. Tsirkin wrote:
>> This should be okay, but I wonder if there should be a virtio_wmb(...)
>> or an "if (weak_barriers) wmb()" before the "writel" in vm_notify
>> (drivers/virtio/virtio_mmio.c).
>>
>> Thanks,
>>
>> Paolo
> That one uses weak barriers AFAIK.
>
> IIUC you mean rproc_virtio_notify.
>
> I suspect it works because specific kick callbacks have a barrier internally.
Yes, that one. At least keystone_rproc_kick doesn't seem to have a barrier.
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg
2018-04-19 17:48 ` Paolo Bonzini
@ 2018-04-19 18:10 ` Michael S. Tsirkin
2018-05-30 14:10 ` Michael S. Tsirkin
2018-06-04 20:16 ` Bjorn Andersson
2 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2018-04-19 18:10 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc,
Jason Wang, virtualization
On Thu, Apr 19, 2018 at 07:48:24PM +0200, Paolo Bonzini wrote:
> On 19/04/2018 19:46, Michael S. Tsirkin wrote:
> >> This should be okay, but I wonder if there should be a virtio_wmb(...)
> >> or an "if (weak_barriers) wmb()" before the "writel" in vm_notify
> >> (drivers/virtio/virtio_mmio.c).
> >>
> >> Thanks,
> >>
> >> Paolo
> > That one uses weak barriers AFAIK.
> >
> > IIUC you mean rproc_virtio_notify.
> >
> > I suspect it works because specific kick callbacks have a barrier internally.
>
> Yes, that one. At least keystone_rproc_kick doesn't seem to have a barrier.
>
> Paolo
Maybe it's wrong or not used with virtio then.
This patch won't change anything with respect to that though.
--
MST
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg
2018-04-19 17:35 [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg Michael S. Tsirkin
2018-04-19 17:39 ` Paolo Bonzini
@ 2018-04-20 3:04 ` Jason Wang
2018-06-04 20:02 ` Bjorn Andersson
2 siblings, 0 replies; 9+ messages in thread
From: Jason Wang @ 2018-04-20 3:04 UTC (permalink / raw)
To: Michael S. Tsirkin, linux-kernel
Cc: Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc,
virtualization, Paolo Bonzini
On 2018年04月20日 01:35, Michael S. Tsirkin wrote:
> virtio is using barriers to order memory accesses, thus
> dma_wmb/rmb is a good match.
>
> Build-tested on x86: Before
>
> [mst@tuck linux]$ size drivers/virtio/virtio_ring.o
> text data bss dec hex filename
> 11392 820 0 12212 2fb4 drivers/virtio/virtio_ring.o
>
> After
> mst@tuck linux]$ size drivers/virtio/virtio_ring.o
> text data bss dec hex filename
> 11284 820 0 12104 2f48 drivers/virtio/virtio_ring.o
>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: linux-remoteproc@vger.kernel.org
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> It's good in theory, but could one of RPMSG maintainers please review
> and ack this patch? Or even better test it?
>
> All these barriers are useless on Intel anyway ...
>
> include/linux/virtio_ring.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> index bbf3252..fab0213 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -35,7 +35,7 @@ static inline void virtio_rmb(bool weak_barriers)
> if (weak_barriers)
> virt_rmb();
> else
> - rmb();
> + dma_rmb();
> }
>
> static inline void virtio_wmb(bool weak_barriers)
> @@ -43,7 +43,7 @@ static inline void virtio_wmb(bool weak_barriers)
> if (weak_barriers)
> virt_wmb();
> else
> - wmb();
> + dma_wmb();
> }
>
> static inline void virtio_store_mb(bool weak_barriers,
Acked-by: Jason Wang <jasowang@redhat.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg
2018-04-19 17:48 ` Paolo Bonzini
2018-04-19 18:10 ` Michael S. Tsirkin
@ 2018-05-30 14:10 ` Michael S. Tsirkin
2018-06-04 20:16 ` Bjorn Andersson
2 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2018-05-30 14:10 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linux-kernel, Ohad Ben-Cohen, Bjorn Andersson, linux-remoteproc,
Jason Wang, virtualization
On Thu, Apr 19, 2018 at 07:48:24PM +0200, Paolo Bonzini wrote:
> On 19/04/2018 19:46, Michael S. Tsirkin wrote:
> >> This should be okay, but I wonder if there should be a virtio_wmb(...)
> >> or an "if (weak_barriers) wmb()" before the "writel" in vm_notify
> >> (drivers/virtio/virtio_mmio.c).
> >>
> >> Thanks,
> >>
> >> Paolo
> > That one uses weak barriers AFAIK.
> >
> > IIUC you mean rproc_virtio_notify.
> >
> > I suspect it works because specific kick callbacks have a barrier internally.
>
> Yes, that one. At least keystone_rproc_kick doesn't seem to have a barrier.
>
> Paolo
Any feedback from rproc maintainers?
--
MST
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg
2018-04-19 17:35 [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg Michael S. Tsirkin
2018-04-19 17:39 ` Paolo Bonzini
2018-04-20 3:04 ` Jason Wang
@ 2018-06-04 20:02 ` Bjorn Andersson
2 siblings, 0 replies; 9+ messages in thread
From: Bjorn Andersson @ 2018-06-04 20:02 UTC (permalink / raw)
To: Michael S. Tsirkin, Suman Anna, Loic Pallardy
Cc: linux-kernel, Ohad Ben-Cohen, linux-remoteproc, Jason Wang,
virtualization, Paolo Bonzini
On Thu 19 Apr 10:35 PDT 2018, Michael S. Tsirkin wrote:
> virtio is using barriers to order memory accesses, thus
> dma_wmb/rmb is a good match.
>
> Build-tested on x86: Before
>
> [mst@tuck linux]$ size drivers/virtio/virtio_ring.o
> text data bss dec hex filename
> 11392 820 0 12212 2fb4 drivers/virtio/virtio_ring.o
>
> After
> mst@tuck linux]$ size drivers/virtio/virtio_ring.o
> text data bss dec hex filename
> 11284 820 0 12104 2f48 drivers/virtio/virtio_ring.o
>
> Cc: Ohad Ben-Cohen <ohad@wizery.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: linux-remoteproc@vger.kernel.org
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
>
> It's good in theory, but could one of RPMSG maintainers please review
> and ack this patch? Or even better test it?
>
> All these barriers are useless on Intel anyway ...
>
> include/linux/virtio_ring.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> index bbf3252..fab0213 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -35,7 +35,7 @@ static inline void virtio_rmb(bool weak_barriers)
> if (weak_barriers)
> virt_rmb();
> else
> - rmb();
> + dma_rmb();
> }
>
> static inline void virtio_wmb(bool weak_barriers)
> @@ -43,7 +43,7 @@ static inline void virtio_wmb(bool weak_barriers)
> if (weak_barriers)
> virt_wmb();
> else
> - wmb();
> + dma_wmb();
> }
>
> static inline void virtio_store_mb(bool weak_barriers,
> --
> MST
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg
2018-04-19 17:48 ` Paolo Bonzini
2018-04-19 18:10 ` Michael S. Tsirkin
2018-05-30 14:10 ` Michael S. Tsirkin
@ 2018-06-04 20:16 ` Bjorn Andersson
2 siblings, 0 replies; 9+ messages in thread
From: Bjorn Andersson @ 2018-06-04 20:16 UTC (permalink / raw)
To: Paolo Bonzini, Suman Anna
Cc: Michael S. Tsirkin, linux-kernel, Ohad Ben-Cohen,
linux-remoteproc, Jason Wang, virtualization
On Thu 19 Apr 10:48 PDT 2018, Paolo Bonzini wrote:
> On 19/04/2018 19:46, Michael S. Tsirkin wrote:
> >> This should be okay, but I wonder if there should be a virtio_wmb(...)
> >> or an "if (weak_barriers) wmb()" before the "writel" in vm_notify
> >> (drivers/virtio/virtio_mmio.c).
> >>
> >> Thanks,
> >>
> >> Paolo
> > That one uses weak barriers AFAIK.
> >
> > IIUC you mean rproc_virtio_notify.
> >
> > I suspect it works because specific kick callbacks have a barrier internally.
>
> Yes, that one. At least keystone_rproc_kick doesn't seem to have a barrier.
>
Afaict you're correct. My expectation is that the kick ensures write
ordering internally and if I read this correct keystone_rproc_kick()
results in a writel_relaxed() in the gpio driver.
@Suman, can you please have a look at this.
Thanks Paolo,
Bjorn
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-06-04 20:16 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-19 17:35 [PATCH] virtio_ring: switch to dma_XX barriers for rpmsg Michael S. Tsirkin
2018-04-19 17:39 ` Paolo Bonzini
2018-04-19 17:46 ` Michael S. Tsirkin
2018-04-19 17:48 ` Paolo Bonzini
2018-04-19 18:10 ` Michael S. Tsirkin
2018-05-30 14:10 ` Michael S. Tsirkin
2018-06-04 20:16 ` Bjorn Andersson
2018-04-20 3:04 ` Jason Wang
2018-06-04 20:02 ` Bjorn Andersson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).