| Message ID | 1524159181-351878-1-git-send-email-mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, > -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, > > -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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> -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
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(-)