| Message ID | 20110504140319.14817.23145.sendpatchset@krkumar2.in.ibm.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, May 04, 2011 at 07:33:19PM +0530, Krishna Kumar wrote: > Introduce virtqueue_get_capacity() to help bail out of transmit > path early. Also remove notification when we run out of space (I > am not sure if this should be under a feature bit). > > Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com> > --- > drivers/virtio/virtio_ring.c | 13 ++++++++----- > include/linux/virtio.h | 5 +++++ > 2 files changed, 13 insertions(+), 5 deletions(-) > > diff -ruNp org/include/linux/virtio.h new/include/linux/virtio.h > --- org/include/linux/virtio.h 2011-05-04 18:57:06.000000000 +0530 > +++ new/include/linux/virtio.h 2011-05-04 18:57:09.000000000 +0530 > @@ -27,6 +27,9 @@ struct virtqueue { > > /** > * operations for virtqueue > + * virtqueue_get_capacity: Get vq capacity > + * vq: the struct virtqueue we're talking about. > + * Returns remaining capacity of queue > * virtqueue_add_buf: expose buffer to other end > * vq: the struct virtqueue we're talking about. > * sg: the description of the buffer(s). > @@ -62,6 +65,8 @@ struct virtqueue { > * All operations can be called in any context. > */ > > +int virtqueue_get_capacity(struct virtqueue *vq); > + > int virtqueue_add_buf_gfp(struct virtqueue *vq, > struct scatterlist sg[], > unsigned int out_num, This is same as Shirley sent? Maybe split and attribute ... > diff -ruNp org/drivers/virtio/virtio_ring.c new/drivers/virtio/virtio_ring.c > --- org/drivers/virtio/virtio_ring.c 2011-05-04 18:57:06.000000000 +0530 > +++ new/drivers/virtio/virtio_ring.c 2011-05-04 18:57:09.000000000 +0530 > @@ -156,6 +156,14 @@ static int vring_add_indirect(struct vri > return head; > } > > +int virtqueue_get_capacity(struct virtqueue *_vq) > +{ > + struct vring_virtqueue *vq = to_vvq(_vq); > + > + return vq->num_free; > +} > +EXPORT_SYMBOL_GPL(virtqueue_get_capacity); > + > int virtqueue_add_buf_gfp(struct virtqueue *_vq, > struct scatterlist sg[], > unsigned int out, > @@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtque > if (vq->num_free < out + in) { > pr_debug("Can't add buf len %i - avail = %i\n", > out + in, vq->num_free); > - /* FIXME: for historical reasons, we force a notify here if > - * there are outgoing parts to the buffer. Presumably the > - * host should service the ring ASAP. */ > - if (out) > - vq->notify(&vq->vq); > END_USE(vq); > return -ENOSPC; > } This will break qemu versions 0.13 and back. I'm adding some new virtio ring flags, we'll be able to reuse one of these to mean 'no need for work around', I think.
On Wed, May 04, 2011 at 07:33:19PM +0530, Krishna Kumar wrote: > @@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtque > if (vq->num_free < out + in) { > pr_debug("Can't add buf len %i - avail = %i\n", > out + in, vq->num_free); > - /* FIXME: for historical reasons, we force a notify here if > - * there are outgoing parts to the buffer. Presumably the > - * host should service the ring ASAP. */ > - if (out) > - vq->notify(&vq->vq); > END_USE(vq); > return -ENOSPC; > } I thought about it some more. We should typically not get into this state with the current driver as we check capacity upfront. So why would this change help performance? Shirley, any idea?
On Wed, May 04, 2011 at 05:50:19PM +0300, Michael S. Tsirkin wrote: > > @@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtque > > if (vq->num_free < out + in) { > > pr_debug("Can't add buf len %i - avail = %i\n", > > out + in, vq->num_free); > > - /* FIXME: for historical reasons, we force a notify here if > > - * there are outgoing parts to the buffer. Presumably the > > - * host should service the ring ASAP. */ > > - if (out) > > - vq->notify(&vq->vq); > > END_USE(vq); > > return -ENOSPC; > > } > > This will break qemu versions 0.13 and back. > I'm adding some new virtio ring flags, we'll be > able to reuse one of these to mean 'no need for > work around', I think. Not really, it wont. We shall almost never get here at all. But then, why would this help performance? > -- > MST -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
"Michael S. Tsirkin" <mst@redhat.com> wrote on 05/05/2011 01:30:23 AM: > > > @@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtque > > > if (vq->num_free < out + in) { > > > pr_debug("Can't add buf len %i - avail = %i\n", > > > out + in, vq->num_free); > > > - /* FIXME: for historical reasons, we force a notify here if > > > - * there are outgoing parts to the buffer. Presumably the > > > - * host should service the ring ASAP. */ > > > - if (out) > > > - vq->notify(&vq->vq); > > > END_USE(vq); > > > return -ENOSPC; > > > } > > > > This will break qemu versions 0.13 and back. > > I'm adding some new virtio ring flags, we'll be > > able to reuse one of these to mean 'no need for > > work around', I think. > > Not really, it wont. We shall almost never get here at all. > But then, why would this help performance? Yes, it is not needed. I will be testing it without this also. thanks, - KK -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 04, 2011 at 11:00:23PM +0300, Michael S. Tsirkin wrote: > On Wed, May 04, 2011 at 05:50:19PM +0300, Michael S. Tsirkin wrote: > > > @@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtque > > > if (vq->num_free < out + in) { > > > pr_debug("Can't add buf len %i - avail = %i\n", > > > out + in, vq->num_free); > > > - /* FIXME: for historical reasons, we force a notify here if > > > - * there are outgoing parts to the buffer. Presumably the > > > - * host should service the ring ASAP. */ > > > - if (out) > > > - vq->notify(&vq->vq); > > > END_USE(vq); > > > return -ENOSPC; > > > } > > > > This will break qemu versions 0.13 and back. > > I'm adding some new virtio ring flags, we'll be > > able to reuse one of these to mean 'no need for > > work around', I think. > > Not really, it wont. We shall almost never get here at all. > But then, why would this help performance? I think I understand this finally. By itself, this patch does not help performance and does not hurt it. But later patch makes us try to xmit and fail there instead of doing capacity checks. With *that* patch applied on top of this one, and with qemu 0.13 and older, performance will be hurt. We need to either - ignore these older hosts - add a feature bit (or use one of the new ones I added: for example with avail_event userspace never needs this behaviour as it can ask to get events when ring gets full) - keep doing capacity checks, which will make us almost never get here > > -- > > MST -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff -ruNp org/include/linux/virtio.h new/include/linux/virtio.h --- org/include/linux/virtio.h 2011-05-04 18:57:06.000000000 +0530 +++ new/include/linux/virtio.h 2011-05-04 18:57:09.000000000 +0530 @@ -27,6 +27,9 @@ struct virtqueue { /** * operations for virtqueue + * virtqueue_get_capacity: Get vq capacity + * vq: the struct virtqueue we're talking about. + * Returns remaining capacity of queue * virtqueue_add_buf: expose buffer to other end * vq: the struct virtqueue we're talking about. * sg: the description of the buffer(s). @@ -62,6 +65,8 @@ struct virtqueue { * All operations can be called in any context. */ +int virtqueue_get_capacity(struct virtqueue *vq); + int virtqueue_add_buf_gfp(struct virtqueue *vq, struct scatterlist sg[], unsigned int out_num, diff -ruNp org/drivers/virtio/virtio_ring.c new/drivers/virtio/virtio_ring.c --- org/drivers/virtio/virtio_ring.c 2011-05-04 18:57:06.000000000 +0530 +++ new/drivers/virtio/virtio_ring.c 2011-05-04 18:57:09.000000000 +0530 @@ -156,6 +156,14 @@ static int vring_add_indirect(struct vri return head; } +int virtqueue_get_capacity(struct virtqueue *_vq) +{ + struct vring_virtqueue *vq = to_vvq(_vq); + + return vq->num_free; +} +EXPORT_SYMBOL_GPL(virtqueue_get_capacity); + int virtqueue_add_buf_gfp(struct virtqueue *_vq, struct scatterlist sg[], unsigned int out, @@ -185,11 +193,6 @@ int virtqueue_add_buf_gfp(struct virtque if (vq->num_free < out + in) { pr_debug("Can't add buf len %i - avail = %i\n", out + in, vq->num_free); - /* FIXME: for historical reasons, we force a notify here if - * there are outgoing parts to the buffer. Presumably the - * host should service the ring ASAP. */ - if (out) - vq->notify(&vq->vq); END_USE(vq); return -ENOSPC; }
Introduce virtqueue_get_capacity() to help bail out of transmit path early. Also remove notification when we run out of space (I am not sure if this should be under a feature bit). Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com> --- drivers/virtio/virtio_ring.c | 13 ++++++++----- include/linux/virtio.h | 5 +++++ 2 files changed, 13 insertions(+), 5 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html