* [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_size
@ 2015-09-02 15:23 Pierre Morel
2015-09-02 15:50 ` Cornelia Huck
0 siblings, 1 reply; 6+ messages in thread
From: Pierre Morel @ 2015-09-02 15:23 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, pmorel, stefanha, gkurz
Being working on dataplane I notice something strange:
virtio_queue_get_avail_size() used a 64bit size index
for the calculation of the available ring size.
It is quite strange but it did work with the old calculation
of the avail ring, at most with performance penalty,
and I wonder where I missed something.
This patch let use a 16bit size as defined in virtio_ring.h
Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
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 788b556..5c856eb 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1460,7 +1460,7 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
{
return offsetof(VRingAvail, ring) +
- sizeof(uint64_t) * vdev->vq[n].vring.num;
+ sizeof(uint16_t) * vdev->vq[n].vring.num;
}
hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
--
2.3.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_size
2015-09-02 15:23 Pierre Morel
@ 2015-09-02 15:50 ` Cornelia Huck
2015-09-02 17:57 ` Greg Kurz
0 siblings, 1 reply; 6+ messages in thread
From: Cornelia Huck @ 2015-09-02 15:50 UTC (permalink / raw)
To: Pierre Morel; +Cc: mst, qemu-devel, stefanha, gkurz
On Wed, 2 Sep 2015 17:23:49 +0200
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> Being working on dataplane I notice something strange:
>
> virtio_queue_get_avail_size() used a 64bit size index
> for the calculation of the available ring size.
>
> It is quite strange but it did work with the old calculation
> of the avail ring, at most with performance penalty,
> and I wonder where I missed something.
>
> This patch let use a 16bit size as defined in virtio_ring.h
>
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> ---
> 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 788b556..5c856eb 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1460,7 +1460,7 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
> hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
> {
> return offsetof(VRingAvail, ring) +
> - sizeof(uint64_t) * vdev->vq[n].vring.num;
> + sizeof(uint16_t) * vdev->vq[n].vring.num;
> }
>
> hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
I'm wondering about the semantics of the _size() functions. Naively I
would expect (size of buffer) * (number of buffers). I think at least
vhost expects the {used,avail} indices in there as well? The
s390-virtio code seems not to expect the indices to be contained in the
size, though...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_size
2015-09-02 15:50 ` Cornelia Huck
@ 2015-09-02 17:57 ` Greg Kurz
2015-09-03 8:23 ` Greg Kurz
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2015-09-02 17:57 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Pierre Morel, qemu-devel, stefanha, mst
On Wed, 2 Sep 2015 17:50:55 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> On Wed, 2 Sep 2015 17:23:49 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>
> > Being working on dataplane I notice something strange:
> >
> > virtio_queue_get_avail_size() used a 64bit size index
> > for the calculation of the available ring size.
> >
> > It is quite strange but it did work with the old calculation
> > of the avail ring, at most with performance penalty,
> > and I wonder where I missed something.
> >
> > This patch let use a 16bit size as defined in virtio_ring.h
> >
> > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> > ---
> > 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 788b556..5c856eb 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1460,7 +1460,7 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
> > hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
> > {
> > return offsetof(VRingAvail, ring) +
> > - sizeof(uint64_t) * vdev->vq[n].vring.num;
> > + sizeof(uint16_t) * vdev->vq[n].vring.num;
> > }
> >
> > hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
>
> I'm wondering about the semantics of the _size() functions. Naively I
> would expect (size of buffer) * (number of buffers). I think at least
Looking at where these functions are called, it really looks like they are
expected to return the size of the memory region to be mapped. Since we have:
typedef struct VRingAvail
{
uint16_t flags;
uint16_t idx;
uint16_t ring[0];
} VRingAvail;
Pierre's patch looks valid. But while we're here, why not introducing
something like:
#define member_size(type, member) sizeof(((type *)0)->member)
It would consolidate the _size functions and the types they are referring to:
- sizeof(uint64_t) * vdev->vq[n].vring.num;
+ member_size(VRingAvail, vring[0]) * vdev->vq[n].vring.num;
> vhost expects the {used,avail} indices in there as well? The
> s390-virtio code seems not to expect the indices to be contained in the
> size, though...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_size
2015-09-02 17:57 ` Greg Kurz
@ 2015-09-03 8:23 ` Greg Kurz
2015-09-03 9:10 ` Cornelia Huck
0 siblings, 1 reply; 6+ messages in thread
From: Greg Kurz @ 2015-09-03 8:23 UTC (permalink / raw)
To: Cornelia Huck; +Cc: Pierre Morel, qemu-devel, stefanha, mst
On Wed, 2 Sep 2015 19:57:25 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Wed, 2 Sep 2015 17:50:55 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
>
> > On Wed, 2 Sep 2015 17:23:49 +0200
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >
> > > Being working on dataplane I notice something strange:
> > >
> > > virtio_queue_get_avail_size() used a 64bit size index
> > > for the calculation of the available ring size.
> > >
> > > It is quite strange but it did work with the old calculation
> > > of the avail ring, at most with performance penalty,
> > > and I wonder where I missed something.
> > >
> > > This patch let use a 16bit size as defined in virtio_ring.h
> > >
> > > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> > > ---
> > > 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 788b556..5c856eb 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1460,7 +1460,7 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
> > > hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
> > > {
> > > return offsetof(VRingAvail, ring) +
> > > - sizeof(uint64_t) * vdev->vq[n].vring.num;
> > > + sizeof(uint16_t) * vdev->vq[n].vring.num;
> > > }
> > >
> > > hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
> >
> > I'm wondering about the semantics of the _size() functions. Naively I
> > would expect (size of buffer) * (number of buffers). I think at least
>
> Looking at where these functions are called, it really looks like they are
> expected to return the size of the memory region to be mapped. Since we have:
>
Acutally no... they are also used to compute the address of used_event_idx
and avail_event_idx.
> typedef struct VRingAvail
> {
> uint16_t flags;
> uint16_t idx;
> uint16_t ring[0];
> } VRingAvail;
>
> Pierre's patch looks valid. But while we're here, why not introducing
> something like:
>
> #define member_size(type, member) sizeof(((type *)0)->member)
>
> It would consolidate the _size functions and the types they are referring to:
>
> - sizeof(uint64_t) * vdev->vq[n].vring.num;
> + member_size(VRingAvail, vring[0]) * vdev->vq[n].vring.num;
>
> > vhost expects the {used,avail} indices in there as well? The
> > s390-virtio code seems not to expect the indices to be contained in the
> > size, though...
>
>
Sorry I missed the real question... should these _size functions return
the actual size + sizeof(uint16_t) ?
Indeed, I could verify the the s390-virtio code uses the _size functions
to compute the address of used_event_idx and avail_event_idx...
The vhost code only uses the _size functions to map memory... and
doesn't add sizeof(uint16_t)... which looks like a bug.
--
Greg
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_size
2015-09-03 8:23 ` Greg Kurz
@ 2015-09-03 9:10 ` Cornelia Huck
0 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2015-09-03 9:10 UTC (permalink / raw)
To: Greg Kurz; +Cc: Pierre Morel, qemu-devel, stefanha, mst
On Thu, 3 Sep 2015 10:23:44 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Wed, 2 Sep 2015 19:57:25 +0200
> Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
>
> > On Wed, 2 Sep 2015 17:50:55 +0200
> > Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> >
> > > On Wed, 2 Sep 2015 17:23:49 +0200
> > > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> > >
> > > > Being working on dataplane I notice something strange:
> > > >
> > > > virtio_queue_get_avail_size() used a 64bit size index
> > > > for the calculation of the available ring size.
> > > >
> > > > It is quite strange but it did work with the old calculation
> > > > of the avail ring, at most with performance penalty,
> > > > and I wonder where I missed something.
> > > >
> > > > This patch let use a 16bit size as defined in virtio_ring.h
> > > >
> > > > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> > > > ---
> > > > 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 788b556..5c856eb 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -1460,7 +1460,7 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
> > > > hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
> > > > {
> > > > return offsetof(VRingAvail, ring) +
> > > > - sizeof(uint64_t) * vdev->vq[n].vring.num;
> > > > + sizeof(uint16_t) * vdev->vq[n].vring.num;
> > > > }
> > > >
> > > > hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
> > >
> > > I'm wondering about the semantics of the _size() functions. Naively I
> > > would expect (size of buffer) * (number of buffers). I think at least
> >
> > Looking at where these functions are called, it really looks like they are
> > expected to return the size of the memory region to be mapped. Since we have:
> >
>
> Acutally no... they are also used to compute the address of used_event_idx
> and avail_event_idx.
>
> > typedef struct VRingAvail
> > {
> > uint16_t flags;
> > uint16_t idx;
> > uint16_t ring[0];
> > } VRingAvail;
> >
> > Pierre's patch looks valid. But while we're here, why not introducing
> > something like:
> >
> > #define member_size(type, member) sizeof(((type *)0)->member)
> >
> > It would consolidate the _size functions and the types they are referring to:
> >
> > - sizeof(uint64_t) * vdev->vq[n].vring.num;
> > + member_size(VRingAvail, vring[0]) * vdev->vq[n].vring.num;
> >
> > > vhost expects the {used,avail} indices in there as well? The
> > > s390-virtio code seems not to expect the indices to be contained in the
> > > size, though...
> >
> >
>
> Sorry I missed the real question... should these _size functions return
> the actual size + sizeof(uint16_t) ?
>
> Indeed, I could verify the the s390-virtio code uses the _size functions
> to compute the address of used_event_idx and avail_event_idx...
> The vhost code only uses the _size functions to map memory... and
> doesn't add sizeof(uint16_t)... which looks like a bug.
Yes, this probably worked by chance because (a) the avail size is too
big anyway and (b) the used size added the offset value... and probably
nobody cares much about s390-virtio reset, but that might explain some
headscratchers we were seeing very occasionally.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_size
@ 2015-09-10 11:37 Pierre Morel
0 siblings, 0 replies; 6+ messages in thread
From: Pierre Morel @ 2015-09-10 11:37 UTC (permalink / raw)
To: qemu-devel; +Cc: cornelia.huck, mst, pmorel, stefanha, gkurz
Being working on dataplane I notice something strange:
virtio_queue_get_avail_size() used a 64bit size index
for the calculation of the available ring size.
It is quite strange but it did work with the old calculation
of the avail ring, at most with performance penalty,
and I wonder where I missed something.
This patch let use a 16bit size as defined in virtio_ring.h
Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
---
PING...
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 788b556..5c856eb 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1460,7 +1460,7 @@ hwaddr virtio_queue_get_desc_size(VirtIODevice *vdev, int n)
hwaddr virtio_queue_get_avail_size(VirtIODevice *vdev, int n)
{
return offsetof(VRingAvail, ring) +
- sizeof(uint64_t) * vdev->vq[n].vring.num;
+ sizeof(uint16_t) * vdev->vq[n].vring.num;
}
hwaddr virtio_queue_get_used_size(VirtIODevice *vdev, int n)
--
2.3.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-09-10 11:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-09-10 11:37 [Qemu-devel] [PATCH] virtio: right size for virtio_queue_get_avail_size Pierre Morel
-- strict thread matches above, loose matches on Subject: below --
2015-09-02 15:23 Pierre Morel
2015-09-02 15:50 ` Cornelia Huck
2015-09-02 17:57 ` Greg Kurz
2015-09-03 8:23 ` Greg Kurz
2015-09-03 9:10 ` Cornelia Huck
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).