* [PATCH 3/3] virtio: avail_event index support
From: Michael S. Tsirkin @ 2011-05-04 21:02 UTC (permalink / raw)
To: qemu-devel
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
habanero, Heiko Carstens, virtualization, steved,
Christian Borntraeger, Tom Lendacky, Martin Schwidefsky, linux390
In-Reply-To: <cover.1304542880.git.mst@redhat.com>
Reduce the number of exits utilizing the
avail_event feature.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
hw/vhost_net.c | 6 ++++++
hw/virtio.c | 26 ++++++++++++++++++++++++--
hw/virtio.h | 7 ++++++-
3 files changed, 36 insertions(+), 3 deletions(-)
diff --git a/hw/vhost_net.c b/hw/vhost_net.c
index 18e4653..aeb5a84 100644
--- a/hw/vhost_net.c
+++ b/hw/vhost_net.c
@@ -53,6 +53,9 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
if (!(net->dev.features & (1ULL << VIRTIO_RING_F_USED_EVENT_IDX))) {
features &= ~(1ULL << VIRTIO_RING_F_USED_EVENT_IDX);
}
+ if (!(net->dev.features & (1ULL << VIRTIO_RING_F_AVAIL_EVENT_IDX))) {
+ features &= ~(1ULL << VIRTIO_RING_F_AVAIL_EVENT_IDX);
+ }
if (!(net->dev.features & (1 << VIRTIO_NET_F_MRG_RXBUF))) {
features &= ~(1 << VIRTIO_NET_F_MRG_RXBUF);
}
@@ -71,6 +74,9 @@ void vhost_net_ack_features(struct vhost_net *net, uint64_t features)
if (features & (1ULL << VIRTIO_RING_F_USED_EVENT_IDX)) {
net->dev.acked_features |= (1ULL << VIRTIO_RING_F_USED_EVENT_IDX);
}
+ if (features & (1ULL << VIRTIO_RING_F_AVAIL_EVENT_IDX)) {
+ net->dev.acked_features |= (1ULL << VIRTIO_RING_F_AVAIL_EVENT_IDX);
+ }
if (features & (1 << VIRTIO_NET_F_MRG_RXBUF)) {
net->dev.acked_features |= (1 << VIRTIO_NET_F_MRG_RXBUF);
}
diff --git a/hw/virtio.c b/hw/virtio.c
index e459093..6ae5542 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -78,6 +78,9 @@ struct VirtQueue
/* Last used index value we have signalled on */
bool signalled_used_valid;
+ /* Notification enabled? */
+ bool notification;
+
int inuse;
uint16_t vector;
@@ -195,12 +198,26 @@ static inline void vring_used_flags_unset_bit(VirtQueue *vq, int mask)
stw_phys(pa, lduw_phys(pa) & ~mask);
}
+static inline void vring_avail_event(VirtQueue *vq, uint16_t val)
+{
+ target_phys_addr_t pa;
+ if (!vq->notification) {
+ return;
+ }
+ pa = vq->vring.used + offsetof(VRingUsed, ring[vq->vring.num]);
+ stw_phys(pa, val);
+}
+
void virtio_queue_set_notification(VirtQueue *vq, int enable)
{
- if (enable)
+ vq->notification = enable;
+ if (vq->vdev->guest_features & (1ULL << VIRTIO_RING_F_AVAIL_EVENT_IDX)) {
+ vring_avail_event(vq, vq->last_avail_idx);
+ } else if (enable) {
vring_used_flags_unset_bit(vq, VRING_USED_F_NO_NOTIFY);
- else
+ } else {
vring_used_flags_set_bit(vq, VRING_USED_F_NO_NOTIFY);
+ }
}
int virtio_queue_ready(VirtQueue *vq)
@@ -412,6 +429,9 @@ int virtqueue_pop(VirtQueue *vq, VirtQueueElement *elem)
max = vq->vring.num;
i = head = virtqueue_get_head(vq, vq->last_avail_idx++);
+ if (vq->vdev->guest_features & (1ULL << VIRTIO_RING_F_AVAIL_EVENT_IDX)) {
+ vring_avail_event(vq, vq->last_avail_idx);
+ }
if (vring_desc_flags(desc_pa, i) & VRING_DESC_F_INDIRECT) {
if (vring_desc_len(desc_pa, i) % sizeof(VRingDesc)) {
@@ -497,6 +517,7 @@ void virtio_reset(void *opaque)
vdev->vq[i].vector = VIRTIO_NO_VECTOR;
vdev->vq[i].signalled_used = 0;
vdev->vq[i].signalled_used_valid = false;
+ vdev->vq[i].notification = true;
}
}
@@ -784,6 +805,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f)
vdev->vq[i].pa = qemu_get_be64(f);
qemu_get_be16s(f, &vdev->vq[i].last_avail_idx);
vdev->vq[i].signalled_used_valid = false;
+ vdev->vq[i].notification = true;
if (vdev->vq[i].pa) {
uint16_t nheads;
diff --git a/hw/virtio.h b/hw/virtio.h
index d765946..8c0923a 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -54,6 +54,9 @@
/* Enables feature bits 32 to 63 (only really required for virtio_pci). */
#define VIRTIO_F_FEATURES_HI 31
+/* The Host publishes the avail index for which it expects a kick
+ * at the end of the used ring. Guest should ignore the used->flags field. */
+#define VIRTIO_RING_F_AVAIL_EVENT_IDX 32
/* from Linux's linux/virtio_ring.h */
@@ -218,7 +221,9 @@ void virtio_serial_exit(VirtIODevice *vdev);
DEFINE_PROP_BIT64("indirect_desc", _state, _field, \
VIRTIO_RING_F_INDIRECT_DESC, true), \
DEFINE_PROP_BIT64("used_event", _state, _field, \
- VIRTIO_RING_F_USED_EVENT_IDX, true)
+ VIRTIO_RING_F_USED_EVENT_IDX, true), \
+ DEFINE_PROP_BIT64("avail_event", _state, _field, \
+ VIRTIO_RING_F_AVAIL_EVENT_IDX, true)
target_phys_addr_t virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
target_phys_addr_t virtio_queue_get_avail_addr(VirtIODevice *vdev, int n);
--
1.7.5.53.gc233e
^ permalink raw reply related
* [PDFv2] virtio-spec: 64 bit features, used/avail event
From: Michael S. Tsirkin @ 2011-05-04 21:17 UTC (permalink / raw)
To: linux-kernel, qemu-devel, kvm
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, linux-s390,
netdev, habanero, Heiko Carstens, virtualization, steved,
Christian Borntraeger, Tom Lendacky, Martin Schwidefsky, linux390
In-Reply-To: <20110504203256.GA20819@redhat.com>
People asked for a pdf for a new spec, so here it is:
http://userweb.kernel.org/~mst/virtio-spec-event-idx-v2.pdf
Guest and host implementation can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net-next-event-idx-v1
git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu-kvm.git virtio-net-event-idx-v1
Description reposted below:
I'm working on a patchset (to follow shortly)
that modified the notificatin hand-off in virtio to be basically
like Xen: each side published an index, the other side only triggers
an event when it crosses that index value
(Xen event indexes start at 1, ours start at 0 for
backward-compatiblity, but that's minor).
Especially for testing, it is very convenient to have
separate feature bits for this change in used and available
ring; since we've run out of bits in the 32 bit field,
I added another 32 bit and bit 31 enables that.
I started with using both flags and indexes in parallel,
but switched to doing either-or: this means we do
not need to tweak memory access ordering as index access just
replaces flags access.
A note on naming: the index replacing avail->flags is named
used_event, the index replacing used->flags is named
avail_event to stress the fact that these actually
point into the other side of the ring:
event is triggered when avail->idx == used->avail_event + 1
and when used->idx == avail->used_event + 1, respectively.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
--
MST
^ permalink raw reply
* Re: [PATCH 0/3] virtio-net: 64 bit features, event index
From: Michael S. Tsirkin @ 2011-05-04 21:20 UTC (permalink / raw)
To: qemu-devel
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
habanero, Heiko Carstens, virtualization, steved,
Christian Borntraeger, Tom Lendacky, Martin Schwidefsky, linux390
In-Reply-To: <cover.1304542880.git.mst@redhat.com>
On Thu, May 05, 2011 at 12:01:48AM +0300, Michael S. Tsirkin wrote:
> OK, here's a patch that implements the virtio spec update that I
> sent earlier. It supercedes the PUBLISH_USED_IDX patches
> I sent out earlier.
Sorry, forgot to mention this is on top of qemu-kvm d16e0f0 .
Copy is here:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/qemu-kvm.git virtio-net-event-idx-v1
Relevant kernel bits are here:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net-next-event-idx-v1
--
MST
^ permalink raw reply
* Re: [PATCH 05/18] virtio: used event index interface
From: Tom Lendacky @ 2011-05-04 21:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Martin Schwidefsky, linux390
In-Reply-To: <aacced20b8109a615ee24c1bde6d9f5702850111.1304541919.git.mst@redhat.com>
On Wednesday, May 04, 2011 03:51:09 PM Michael S. Tsirkin wrote:
> Define a new feature bit for the guest to utilize a used_event index
> (like Xen) instead if a flag bit to enable/disable interrupts.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/linux/virtio_ring.h | 9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> index e4d144b..f5c1b75 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -29,6 +29,10 @@
> /* We support indirect buffer descriptors */
> #define VIRTIO_RING_F_INDIRECT_DESC 28
>
> +/* The Guest publishes the used index for which it expects an interrupt
> + * at the end of the avail ring. Host should ignore the avail->flags
> field. */ +#define VIRTIO_RING_F_USED_EVENT_IDX 29
> +
> /* Virtio ring descriptors: 16 bytes. These can chain together via
> "next". */ struct vring_desc {
> /* Address (guest-physical). */
> @@ -83,6 +87,7 @@ struct vring {
> * __u16 avail_flags;
> * __u16 avail_idx;
> * __u16 available[num];
> + * __u16 used_event_idx;
> *
> * // Padding to the next align boundary.
> * char pad[];
> @@ -93,6 +98,10 @@ struct vring {
> * struct vring_used_elem used[num];
> * };
> */
> +/* We publish the used event index at the end of the available ring.
> + * It is at the end for backwards compatibility. */
> +#define vring_used_event(vr) ((vr)->avail->ring[(vr)->num])
> +
> static inline void vring_init(struct vring *vr, unsigned int num, void *p,
> unsigned long align)
> {
You should update the vring_size procedure to account for the extra field at
the end of the available ring by change the "(2 + num)" to "(3 + num)":
return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (3 + num)
Tom
^ permalink raw reply
* Re: [PATCH 09/18] virtio: use avail_event index
From: Tom Lendacky @ 2011-05-04 21:58 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Martin Schwidefsky, linux390
In-Reply-To: <8bba6a0a8eee17e741c5155b04ff1b1c9f34bf94.1304541919.git.mst@redhat.com>
On Wednesday, May 04, 2011 03:51:47 PM Michael S. Tsirkin wrote:
> Use the new avail_event feature to reduce the number
> of exits from the guest.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/virtio/virtio_ring.c | 39
> ++++++++++++++++++++++++++++++++++++++- 1 files changed, 38 insertions(+),
> 1 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 3a3ed75..262dfe6 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -82,6 +82,15 @@ struct vring_virtqueue
> /* Host supports indirect buffers */
> bool indirect;
>
> + /* Host publishes avail event idx */
> + bool event;
> +
> + /* Is kicked_avail below valid? */
> + bool kicked_avail_valid;
> +
> + /* avail idx value we already kicked. */
> + u16 kicked_avail;
> +
> /* Number of free buffers */
> unsigned int num_free;
> /* Head of free buffer list. */
> @@ -228,6 +237,12 @@ add_head:
> * new available array entries. */
> virtio_wmb();
> vq->vring.avail->idx++;
> + /* If the driver never bothers to kick in a very long while,
> + * avail index might wrap around. If that happens, invalidate
> + * kicked_avail index we stored. TODO: make sure all drivers
> + * kick at least once in 2^16 and remove this. */
> + if (unlikely(vq->vring.avail->idx == vq->kicked_avail))
> + vq->kicked_avail_valid = true;
vq->kicked_avail_valid should be set to false here.
Tom
>
> pr_debug("Added buffer head %i to %p\n", head, vq);
> END_USE(vq);
> @@ -236,6 +251,23 @@ add_head:
> }
> EXPORT_SYMBOL_GPL(virtqueue_add_buf_gfp);
>
> +
> +static bool vring_notify(struct vring_virtqueue *vq)
> +{
> + u16 old, new;
> + bool v;
> + if (!vq->event)
> + return !(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
> +
> + v = vq->kicked_avail_valid;
> + old = vq->kicked_avail;
> + new = vq->kicked_avail = vq->vring.avail->idx;
> + vq->kicked_avail_valid = true;
> + if (unlikely(!v))
> + return true;
> + return vring_need_event(vring_avail_event(&vq->vring), new, old);
> +}
> +
> void virtqueue_kick(struct virtqueue *_vq)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> @@ -244,7 +276,7 @@ void virtqueue_kick(struct virtqueue *_vq)
> /* Need to update avail index before checking if we should notify */
> virtio_mb();
>
> - if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
> + if (vring_notify(vq))
> /* Prod other side to tell it about changes. */
> vq->notify(&vq->vq);
>
> @@ -437,6 +469,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> vq->vq.name = name;
> vq->notify = notify;
> vq->broken = false;
> + vq->kicked_avail_valid = false;
> + vq->kicked_avail = 0;
> vq->last_used_idx = 0;
> list_add_tail(&vq->vq.list, &vdev->vqs);
> #ifdef DEBUG
> @@ -444,6 +478,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> #endif
>
> vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
> + vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_AVAIL_EVENT_IDX);
>
> /* No callback? Tell other side not to bother us. */
> if (!callback)
> @@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device
> *vdev) break;
> case VIRTIO_RING_F_USED_EVENT_IDX:
> break;
> + case VIRTIO_RING_F_AVAIL_EVENT_IDX:
> + break;
> default:
> /* We don't understand this bit. */
> clear_bit(i, vdev->features);
^ permalink raw reply
* Re: [PATCH 07/18] virtio ring: inline function to check for events
From: Stefan Hajnoczi @ 2011-05-05 8:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <cc4596a5ecdbf8dd37b5567a36667e3841f18ca3.1304541919.git.mst@redhat.com>
On Wed, May 4, 2011 at 9:51 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> With the new used_event and avail_event and features, both
> host and guest need similar logic to check whether events are
> enabled, so it helps to put the common code in the header.
>
> Note that Xen has similar logic for notification hold-off
> in include/xen/interface/io/ring.h with req_event and req_prod
> corresponding to event_idx + 1 and new_idx respectively.
> +1 comes from the fact that req_event and req_prod in Xen start at 1,
> while event index in virtio starts at 0.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/linux/virtio_ring.h | 14 ++++++++++++++
> 1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> index f791772..2a3b0ea 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -124,6 +124,20 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
> + sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num;
> }
>
> +/* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
> +/* Assuming a given event_idx value from the other size, if
s/other size/other side/ ?
Stefan
^ permalink raw reply
* Re: [PATCH 07/18] virtio ring: inline function to check for events
From: Michael S. Tsirkin @ 2011-05-05 8:56 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <BANLkTi=eH-iK7L2LBAm0eqidapqXqhSqxw@mail.gmail.com>
On Thu, May 05, 2011 at 09:34:46AM +0100, Stefan Hajnoczi wrote:
> On Wed, May 4, 2011 at 9:51 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > With the new used_event and avail_event and features, both
> > host and guest need similar logic to check whether events are
> > enabled, so it helps to put the common code in the header.
> >
> > Note that Xen has similar logic for notification hold-off
> > in include/xen/interface/io/ring.h with req_event and req_prod
> > corresponding to event_idx + 1 and new_idx respectively.
> > +1 comes from the fact that req_event and req_prod in Xen start at 1,
> > while event index in virtio starts at 0.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > include/linux/virtio_ring.h | 14 ++++++++++++++
> > 1 files changed, 14 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> > index f791772..2a3b0ea 100644
> > --- a/include/linux/virtio_ring.h
> > +++ b/include/linux/virtio_ring.h
> > @@ -124,6 +124,20 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
> > + sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num;
> > }
> >
> > +/* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
> > +/* Assuming a given event_idx value from the other size, if
>
> s/other size/other side/ ?
>
> Stefan
Exactly. Good catch, thanks.
--
MST
^ permalink raw reply
* Re: [PATCH 09/18] virtio: use avail_event index
From: Michael S. Tsirkin @ 2011-05-05 9:34 UTC (permalink / raw)
To: Tom Lendacky
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Martin Schwidefsky, linux390
In-Reply-To: <201105041658.19917.tahm@linux.vnet.ibm.com>
On Wed, May 04, 2011 at 04:58:18PM -0500, Tom Lendacky wrote:
>
> On Wednesday, May 04, 2011 03:51:47 PM Michael S. Tsirkin wrote:
> > Use the new avail_event feature to reduce the number
> > of exits from the guest.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/virtio/virtio_ring.c | 39
> > ++++++++++++++++++++++++++++++++++++++- 1 files changed, 38 insertions(+),
> > 1 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 3a3ed75..262dfe6 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -82,6 +82,15 @@ struct vring_virtqueue
> > /* Host supports indirect buffers */
> > bool indirect;
> >
> > + /* Host publishes avail event idx */
> > + bool event;
> > +
> > + /* Is kicked_avail below valid? */
> > + bool kicked_avail_valid;
> > +
> > + /* avail idx value we already kicked. */
> > + u16 kicked_avail;
> > +
> > /* Number of free buffers */
> > unsigned int num_free;
> > /* Head of free buffer list. */
> > @@ -228,6 +237,12 @@ add_head:
> > * new available array entries. */
> > virtio_wmb();
> > vq->vring.avail->idx++;
> > + /* If the driver never bothers to kick in a very long while,
> > + * avail index might wrap around. If that happens, invalidate
> > + * kicked_avail index we stored. TODO: make sure all drivers
> > + * kick at least once in 2^16 and remove this. */
> > + if (unlikely(vq->vring.avail->idx == vq->kicked_avail))
> > + vq->kicked_avail_valid = true;
>
> vq->kicked_avail_valid should be set to false here.
>
> Tom
Right, good catch.
> >
> > pr_debug("Added buffer head %i to %p\n", head, vq);
> > END_USE(vq);
> > @@ -236,6 +251,23 @@ add_head:
> > }
> > EXPORT_SYMBOL_GPL(virtqueue_add_buf_gfp);
> >
> > +
> > +static bool vring_notify(struct vring_virtqueue *vq)
> > +{
> > + u16 old, new;
> > + bool v;
> > + if (!vq->event)
> > + return !(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
> > +
> > + v = vq->kicked_avail_valid;
> > + old = vq->kicked_avail;
> > + new = vq->kicked_avail = vq->vring.avail->idx;
> > + vq->kicked_avail_valid = true;
> > + if (unlikely(!v))
> > + return true;
> > + return vring_need_event(vring_avail_event(&vq->vring), new, old);
> > +}
> > +
> > void virtqueue_kick(struct virtqueue *_vq)
> > {
> > struct vring_virtqueue *vq = to_vvq(_vq);
> > @@ -244,7 +276,7 @@ void virtqueue_kick(struct virtqueue *_vq)
> > /* Need to update avail index before checking if we should notify */
> > virtio_mb();
> >
> > - if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY))
> > + if (vring_notify(vq))
> > /* Prod other side to tell it about changes. */
> > vq->notify(&vq->vq);
> >
> > @@ -437,6 +469,8 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> > vq->vq.name = name;
> > vq->notify = notify;
> > vq->broken = false;
> > + vq->kicked_avail_valid = false;
> > + vq->kicked_avail = 0;
> > vq->last_used_idx = 0;
> > list_add_tail(&vq->vq.list, &vdev->vqs);
> > #ifdef DEBUG
> > @@ -444,6 +478,7 @@ struct virtqueue *vring_new_virtqueue(unsigned int num,
> > #endif
> >
> > vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
> > + vq->event = virtio_has_feature(vdev, VIRTIO_RING_F_AVAIL_EVENT_IDX);
> >
> > /* No callback? Tell other side not to bother us. */
> > if (!callback)
> > @@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device
> > *vdev) break;
> > case VIRTIO_RING_F_USED_EVENT_IDX:
> > break;
> > + case VIRTIO_RING_F_AVAIL_EVENT_IDX:
> > + break;
> > default:
> > /* We don't understand this bit. */
> > clear_bit(i, vdev->features);
^ permalink raw reply
* Re: [PATCH 05/18] virtio: used event index interface
From: Michael S. Tsirkin @ 2011-05-05 9:38 UTC (permalink / raw)
To: Tom Lendacky
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Martin Schwidefsky, linux390
In-Reply-To: <201105041656.11009.tahm@linux.vnet.ibm.com>
On Wed, May 04, 2011 at 04:56:09PM -0500, Tom Lendacky wrote:
> On Wednesday, May 04, 2011 03:51:09 PM Michael S. Tsirkin wrote:
> > Define a new feature bit for the guest to utilize a used_event index
> > (like Xen) instead if a flag bit to enable/disable interrupts.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > include/linux/virtio_ring.h | 9 +++++++++
> > 1 files changed, 9 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> > index e4d144b..f5c1b75 100644
> > --- a/include/linux/virtio_ring.h
> > +++ b/include/linux/virtio_ring.h
> > @@ -29,6 +29,10 @@
> > /* We support indirect buffer descriptors */
> > #define VIRTIO_RING_F_INDIRECT_DESC 28
> >
> > +/* The Guest publishes the used index for which it expects an interrupt
> > + * at the end of the avail ring. Host should ignore the avail->flags
> > field. */ +#define VIRTIO_RING_F_USED_EVENT_IDX 29
> > +
> > /* Virtio ring descriptors: 16 bytes. These can chain together via
> > "next". */ struct vring_desc {
> > /* Address (guest-physical). */
> > @@ -83,6 +87,7 @@ struct vring {
> > * __u16 avail_flags;
> > * __u16 avail_idx;
> > * __u16 available[num];
> > + * __u16 used_event_idx;
> > *
> > * // Padding to the next align boundary.
> > * char pad[];
> > @@ -93,6 +98,10 @@ struct vring {
> > * struct vring_used_elem used[num];
> > * };
> > */
> > +/* We publish the used event index at the end of the available ring.
> > + * It is at the end for backwards compatibility. */
> > +#define vring_used_event(vr) ((vr)->avail->ring[(vr)->num])
> > +
> > static inline void vring_init(struct vring *vr, unsigned int num, void *p,
> > unsigned long align)
> > {
>
> You should update the vring_size procedure to account for the extra field at
> the end of the available ring by change the "(2 + num)" to "(3 + num)":
> return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (3 + num)
>
> Tom
In practice it gives the same result because of the alignment, but sure.
Thanks!
--
MST
^ permalink raw reply
* [PATCH 0/3] virtio and vhost-net performance enhancements
From: Michael S. Tsirkin @ 2011-05-05 15:07 UTC (permalink / raw)
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <cover.1304541918.git.mst@redhat.com>
Here are a couple of minor fixes suggested on list.
Applies on top of the previous patchset.
As before result pushed here:
git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git vhost-net-next-event-idx-v1
Michael S. Tsirkin (3):
virtio: fix avail event support
virtio_ring: check used_event offset
virtio_ring: need_event api comment fix
drivers/virtio/virtio_ring.c | 2 +-
include/linux/virtio_ring.h | 10 ++++++++--
2 files changed, 9 insertions(+), 3 deletions(-)
--
1.7.5.53.gc233e
^ permalink raw reply
* [PATCH 1/3] virtio: fix avail event support
From: Michael S. Tsirkin @ 2011-05-05 15:08 UTC (permalink / raw)
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <cover.1304605816.git.mst@redhat.com>
make valid flag false, not true, on overrun
Reported-by: Tom Lendacky <tahm@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/virtio/virtio_ring.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 57bf9d5..0ea0781 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -242,7 +242,7 @@ add_head:
* kicked_avail index we stored. TODO: make sure all drivers
* kick at least once in 2^16 and remove this. */
if (unlikely(vq->vring.avail->idx == vq->kicked_avail))
- vq->kicked_avail_valid = true;
+ vq->kicked_avail_valid = false;
pr_debug("Added buffer head %i to %p\n", head, vq);
END_USE(vq);
--
1.7.5.53.gc233e
^ permalink raw reply related
* [PATCH 2/3] virtio_ring: check used_event offset
From: Michael S. Tsirkin @ 2011-05-05 15:08 UTC (permalink / raw)
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <cover.1304605816.git.mst@redhat.com>
Nothing's wrong with vring_size as is, but it's nice
to check that the new field in the avail ring
won't overlow into the used ring.
Reported-by: Tom Lendacky <tahm@linux.vnet.ibm.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_ring.h | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 2a3b0ea..089cbf2 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -119,7 +119,13 @@ static inline void vring_init(struct vring *vr, unsigned int num, void *p,
static inline unsigned vring_size(unsigned int num, unsigned long align)
{
- return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
+#ifdef __KERNEL__
+ /* Older versions did not have used_event field at the end of the
+ * avail ring. Used ring offset must be compatible with such devices. */
+ size_t s = sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num);
+ BUG_ON(ALIGN(s, align) != ALIGN(s + sizeof(__u16), align));
+#endif
+ return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (3 + num)
+ align - 1) & ~(align - 1))
+ sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num;
}
--
1.7.5.53.gc233e
^ permalink raw reply related
* [PATCH 3/3] virtio_ring: need_event api comment fix
From: Michael S. Tsirkin @ 2011-05-05 15:08 UTC (permalink / raw)
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <cover.1304605816.git.mst@redhat.com>
fix typo in a comment: size -> side
Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/virtio_ring.h | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
index 089cbf2..0a45c6e 100644
--- a/include/linux/virtio_ring.h
+++ b/include/linux/virtio_ring.h
@@ -131,7 +131,7 @@ static inline unsigned vring_size(unsigned int num, unsigned long align)
}
/* The following is used with USED_EVENT_IDX and AVAIL_EVENT_IDX */
-/* Assuming a given event_idx value from the other size, if
+/* Assuming a given event_idx value from the other side, if
* we have just incremented index from old to new_idx,
* should we trigger an event? */
static inline int vring_need_event(__u16 event_idx, __u16 new_idx, __u16 old)
--
1.7.5.53.gc233e
^ permalink raw reply related
* RE: various vmbus review comments
From: KY Srinivasan @ 2011-05-06 13:10 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110504163228.GA19754@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, May 04, 2011 12:32 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
>
> On Wed, May 04, 2011 at 04:20:11PM +0000, KY Srinivasan wrote:
> >
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:greg@kroah.com]
> > > Sent: Tuesday, May 03, 2011 4:47 PM
> > > To: KY Srinivasan
> > > Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> > > - module reference counting. Are you sure you got it all right
> > > for the individual modules that attach to the bus? I don't
> > > see any reference counting happening, is that correct?
> >
> > For this round, I want to concentrate on just the vmbus driver. So,
> > module reference counting is I don't think an issue for the vmbus driver
> > given that the driver is not unlodable. Once I am done with the vmbus driver
> > I will address the module reference counting issues for other drivers.
>
> No, I am referring to the module reference counting of the bus drivers
> that register with the vmbus core. You aren't doing that at all, and
> you probably need to make sure that this isn't needed. That is
> concentrating on the vmbus driver.
I audited the block and the net drivers. As part of their exit routine,
they invoke vmbus_child_driver_unregister() after properly cleaning
up all the devices they are managing. Do you still see an issue with
regards to module reference counting.
>
> > I will also address your comment on static initialization hv_driver instances
> > as part of other driver cleanup.
>
> No, please do this now as it will show how to properly interact with the
> vmbus core code in the correct manner. Hopefully that will be correct,
> but I have a feeling that it will show you some places in the API that
> need to be changed...
As opposed to run-time initialization of fields such as probe, etc; I have
initialized them statically. For instance, in the blkvsc driver:
/* The one and only one */
static struct storvsc_driver blkvsc_drv = {
.base.probe = blkvsc_probe,
.base.remove = blkvsc_remove,
.base.shutdown = blkvsc_shutdown,
};
Is this what you had in mind.
Regards,
K. Y
^ permalink raw reply
* Re: various vmbus review comments
From: Greg KH @ 2011-05-06 14:59 UTC (permalink / raw)
To: KY Srinivasan
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481E0694@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Fri, May 06, 2011 at 01:10:38PM +0000, KY Srinivasan wrote:
> > No, I am referring to the module reference counting of the bus drivers
> > that register with the vmbus core. You aren't doing that at all, and
> > you probably need to make sure that this isn't needed. That is
> > concentrating on the vmbus driver.
>
> I audited the block and the net drivers. As part of their exit routine,
> they invoke vmbus_child_driver_unregister() after properly cleaning
> up all the devices they are managing. Do you still see an issue with
> regards to module reference counting.
I will look again, the next time I review the vmbus code.
> > > I will also address your comment on static initialization hv_driver instances
> > > as part of other driver cleanup.
> >
> > No, please do this now as it will show how to properly interact with the
> > vmbus core code in the correct manner. Hopefully that will be correct,
> > but I have a feeling that it will show you some places in the API that
> > need to be changed...
>
> As opposed to run-time initialization of fields such as probe, etc; I have
> initialized them statically. For instance, in the blkvsc driver:
>
> /* The one and only one */
> static struct storvsc_driver blkvsc_drv = {
> .base.probe = blkvsc_probe,
> .base.remove = blkvsc_remove,
> .base.shutdown = blkvsc_shutdown,
> };
>
> Is this what you had in mind.
Close. The format is correct.
But what's with that ".base." crud? That shows that something is wrong
as no USB or PCI or any other bus driver has to mess with a ".base"
subpointer in a driver structure.
See, I told you that when you converted to use this format the problems
would pop out at you :)
Please fix that.
thanks,
greg k-h
^ permalink raw reply
* RE: various vmbus review comments
From: KY Srinivasan @ 2011-05-06 17:34 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110506145944.GA11871@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Friday, May 06, 2011 11:00 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
>
> On Fri, May 06, 2011 at 01:10:38PM +0000, KY Srinivasan wrote:
> > > No, I am referring to the module reference counting of the bus drivers
> > > that register with the vmbus core. You aren't doing that at all, and
> > > you probably need to make sure that this isn't needed. That is
> > > concentrating on the vmbus driver.
> >
> > I audited the block and the net drivers. As part of their exit routine,
> > they invoke vmbus_child_driver_unregister() after properly cleaning
> > up all the devices they are managing. Do you still see an issue with
> > regards to module reference counting.
>
> I will look again, the next time I review the vmbus code.
Thank you.
>
> > > > I will also address your comment on static initialization hv_driver instances
> > > > as part of other driver cleanup.
> > >
> > > No, please do this now as it will show how to properly interact with the
> > > vmbus core code in the correct manner. Hopefully that will be correct,
> > > but I have a feeling that it will show you some places in the API that
> > > need to be changed...
> >
> > As opposed to run-time initialization of fields such as probe, etc; I have
> > initialized them statically. For instance, in the blkvsc driver:
> >
> > /* The one and only one */
> > static struct storvsc_driver blkvsc_drv = {
> > .base.probe = blkvsc_probe,
> > .base.remove = blkvsc_remove,
> > .base.shutdown = blkvsc_shutdown,
> > };
> >
> > Is this what you had in mind.
>
> Close. The format is correct.
>
> But what's with that ".base." crud? That shows that something is wrong
> as no USB or PCI or any other bus driver has to mess with a ".base"
> subpointer in a driver structure.
>
> See, I told you that when you converted to use this format the problems
> would pop out at you :)
>
> Please fix that.
I will. I am incrementally cleaning up the drivers, I will cleanup the ".base"
crud as part of additional cleanup that I will do.
Regards,
K. Y
^ permalink raw reply
* RE: various vmbus review comments
From: KY Srinivasan @ 2011-05-09 1:46 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110503204641.GA11132@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, May 03, 2011 4:47 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: various vmbus review comments
>
> I just took a quick look at the vmbus code, and have the following
> comments:
> - why is count_hv_channel() even a function?
Done; I got rid of this function
> - your .h files need to be consolidated and renamed. You will
> need a single hyperv.h file for include/linux/ that will
> contain some of what the vmbus*.h files have in it, but not
> all. Please merge things together to have:
> - include/linux/hyperv.h
> What is needed to build the drivers that attach to
> the bus
> - drivers/staging/hv/hyperv.h
> The local .h file will have the vmbus*.h remaining
> stuff that is only needed by the hv_vmbus.ko module
> to be build.
Done; as I had informed you in an earlier mail, in addition to the two header files
you have mentioned, I have also created driver specific header files for block and
network drivers.
> - the instances of hv_driver structures need to be static and
> not programatically defined, like all other USB and PCI
> drivers are handled.
Done. You had expressed some concern that this would expose some issue
with the core vmbus driver (which is what I want to concentrate on this
go around). I have done this for both the block driver and the mouse driver
and I am pretty sure I can do the same with the network driver. I have not
currently done this for the network driver, since the number of patches I have
to submit is already very large.
> - module reference counting. Are you sure you got it all right
> for the individual modules that attach to the bus? I don't
> see any reference counting happening, is that correct?
I have already exchanged an email with you on this. To summarize, it
does not look like there is a problem
> - fix the sparse warnings.
Mostly done; most of the errors are in the base kernel coming out of
the macro page_to_pfn()
> - fix the use of volatile in the ring buffer code. It should
> not be needed and if you are relying on it, then the code is
> wrong.
You are right; all accesses were already serialized with a spin lock and the
Volatile attribute was unnecessary.
> - fix the namespace on the ringbuffer code to show that it
> really is only for the hyperv bus code internally.
Done.
>
> That should be enough for at least one more set of patches for you all
> to work on :)
Greg,
I have had so much fun cleaning up these drivers that I lost track of the patch count.
I have addressed all the issues you have raised in addition to some other cleanup
that I was doing since about a week. As I look at the patch-set, I have little over
200 patches. If it is ok with you, I would like to send them as a single set. Let me know
what you prefer. I need to circulate these patches internally before I can send them out.
I should be able to send these out early next week.
Regards,
K. Y
^ permalink raw reply
* Re: various vmbus review comments
From: Greg KH @ 2011-05-09 3:04 UTC (permalink / raw)
To: KY Srinivasan
Cc: devel@linuxdriverproject.org, gregkh@suse.de,
linux-kernel@vger.kernel.org, virtualization@lists.osdl.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481E9404@TK5EX14MBXC128.redmond.corp.microsoft.com>
On Mon, May 09, 2011 at 01:46:56AM +0000, KY Srinivasan wrote:
> > - the instances of hv_driver structures need to be static and
> > not programatically defined, like all other USB and PCI
> > drivers are handled.
>
> Done. You had expressed some concern that this would expose some issue
> with the core vmbus driver (which is what I want to concentrate on this
> go around). I have done this for both the block driver and the mouse driver
> and I am pretty sure I can do the same with the network driver. I have not
> currently done this for the network driver, since the number of patches I have
> to submit is already very large.
Ok, but it shouldn't be a major change to the code, right?
> > - module reference counting. Are you sure you got it all right
> > for the individual modules that attach to the bus? I don't
> > see any reference counting happening, is that correct?
>
> I have already exchanged an email with you on this. To summarize, it
> does not look like there is a problem
>
> > - fix the sparse warnings.
> Mostly done; most of the errors are in the base kernel coming out of
> the macro page_to_pfn()
>
> > - fix the use of volatile in the ring buffer code. It should
> > not be needed and if you are relying on it, then the code is
> > wrong.
>
> You are right; all accesses were already serialized with a spin lock and the
> Volatile attribute was unnecessary.
>
> > - fix the namespace on the ringbuffer code to show that it
> > really is only for the hyperv bus code internally.
>
> Done.
>
> >
> > That should be enough for at least one more set of patches for you all
> > to work on :)
>
> Greg,
>
> I have had so much fun cleaning up these drivers that I lost track of the patch count.
> I have addressed all the issues you have raised in addition to some other cleanup
> that I was doing since about a week. As I look at the patch-set, I have little over
> 200 patches. If it is ok with you, I would like to send them as a single set. Let me know
> what you prefer. I need to circulate these patches internally before I can send them out.
> I should be able to send these out early next week.
A single set is fine, if that's what you want to do, I can handle that
amount as long as they all build all along the way and don't break
anything.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 06/18] virtio_ring: avail event index interface
From: Rusty Russell @ 2011-05-09 4:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <b00ef5d572ec9bc3b884c496025512abc1f14349.1304541919.git.mst@redhat.com>
On Wed, 4 May 2011 23:51:19 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Define a new feature bit for the host to
> declare that it uses an avail_event index
> (like Xen) instead of a feature bit
> to enable/disable interrupts.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> include/linux/virtio_ring.h | 11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/virtio_ring.h b/include/linux/virtio_ring.h
> index f5c1b75..f791772 100644
> --- a/include/linux/virtio_ring.h
> +++ b/include/linux/virtio_ring.h
> @@ -32,6 +32,9 @@
> /* The Guest publishes the used index for which it expects an interrupt
> * at the end of the avail ring. Host should ignore the avail->flags field. */
> #define VIRTIO_RING_F_USED_EVENT_IDX 29
> +/* The Host publishes the avail index for which it expects a kick
> + * at the end of the used ring. Guest should ignore the used->flags field. */
> +#define VIRTIO_RING_F_AVAIL_EVENT_IDX 32
Are you really sure we want to separate the two? Seems a little simpler
to have one bit to mean "we're publishing our threshold". For someone
implementing this from scratch, it's a little simpler.
Or are there cases where the old style makes more sense?
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 08/18] virtio_ring: support for used_event idx feature
From: Rusty Russell @ 2011-05-09 4:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <dd16b9f3bad847b0eadc7f94368e27982c1a7560.1304541919.git.mst@redhat.com>
On Wed, 4 May 2011 23:51:38 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Add support for the used_event idx feature: when enabling
> interrupts, publish the current avail index value to
> the host so that we get interrupts on the next update.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/virtio/virtio_ring.c | 14 ++++++++++++++
> 1 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 507d6eb..3a3ed75 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -320,6 +320,14 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> ret = vq->data[i];
> detach_buf(vq, i);
> vq->last_used_idx++;
> + /* If we expect an interrupt for the next entry, tell host
> + * by writing event index and flush out the write before
> + * the read in the next get_buf call. */
> + if (!(vq->vring.avail->flags & VRING_AVAIL_F_NO_INTERRUPT)) {
> + vring_used_event(&vq->vring) = vq->last_used_idx;
> + virtio_mb();
> + }
> +
Hmm, so you're still using the avail->flags; it's just if thresholding
is enabled the host will ignore it?
It's a little subtle, but it keeps this patch small. Perhaps we'll want
to make it more explicit later.
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 09/18] virtio: use avail_event index
From: Rusty Russell @ 2011-05-09 4:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <8bba6a0a8eee17e741c5155b04ff1b1c9f34bf94.1304541919.git.mst@redhat.com>
On Wed, 4 May 2011 23:51:47 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Use the new avail_event feature to reduce the number
> of exits from the guest.
Figures here would be nice :)
> @@ -228,6 +237,12 @@ add_head:
> * new available array entries. */
> virtio_wmb();
> vq->vring.avail->idx++;
> + /* If the driver never bothers to kick in a very long while,
> + * avail index might wrap around. If that happens, invalidate
> + * kicked_avail index we stored. TODO: make sure all drivers
> + * kick at least once in 2^16 and remove this. */
> + if (unlikely(vq->vring.avail->idx == vq->kicked_avail))
> + vq->kicked_avail_valid = true;
If they don't, they're already buggy. Simply do:
WARN_ON(vq->vring.avail->idx == vq->kicked_avail);
> +static bool vring_notify(struct vring_virtqueue *vq)
> +{
> + u16 old, new;
> + bool v;
> + if (!vq->event)
> + return !(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY);
> +
> + v = vq->kicked_avail_valid;
> + old = vq->kicked_avail;
> + new = vq->kicked_avail = vq->vring.avail->idx;
> + vq->kicked_avail_valid = true;
> + if (unlikely(!v))
> + return true;
This is the only place you actually used kicked_avail_valid. Is it
possible to initialize it in such a way that you can remove this?
> @@ -482,6 +517,8 @@ void vring_transport_features(struct virtio_device *vdev)
> break;
> case VIRTIO_RING_F_USED_EVENT_IDX:
> break;
> + case VIRTIO_RING_F_AVAIL_EVENT_IDX:
> + break;
> default:
> /* We don't understand this bit. */
> clear_bit(i, vdev->features);
Does this belong in a prior patch?
Thanks,
Rusty.
^ permalink raw reply
* Re: [PATCH 14/18] virtio: add api for delayed callbacks
From: Rusty Russell @ 2011-05-09 5:57 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <64d47c628b3fdc0ac156aed4be182933d8bcc0db.1304541919.git.mst@redhat.com>
On Wed, 4 May 2011 23:52:33 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> Add an API that tells the other side that callbacks
> should be delayed until a lot of work has been done.
> Implement using the new used_event feature.
Since you're going to add a capacity query anyway, why not add the
threshold argument here?
Then the caller can choose how much space it needs. Maybe net and block
will want different things...
Cheers,
Rusty.
^ permalink raw reply
* Re: [PATCH 3/3] virtio_ring: need_event api comment fix
From: Rusty Russell @ 2011-05-09 5:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Krishna Kumar, Carsten Otte, lguest, Shirley Ma, kvm, linux-s390,
netdev, habanero, Heiko Carstens, linux-kernel, virtualization,
steved, Christian Borntraeger, Tom Lendacky, Martin Schwidefsky,
linux390
In-Reply-To: <c8b6d8a8df4927826d0024fceeb68726c50d0b1a.1304605817.git.mst@redhat.com>
On Thu, 5 May 2011 18:08:17 +0300, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> fix typo in a comment: size -> side
>
> Reported-by: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
I could smerge these together for you, but I *really* want benchmarks in
these commit messages.
Thanks,
Rusty.
PS. Was away last week, hence the delay on this...
^ permalink raw reply
* RE: various vmbus review comments
From: KY Srinivasan @ 2011-05-09 12:35 UTC (permalink / raw)
To: Greg KH
Cc: gregkh@suse.de, linux-kernel@vger.kernel.org,
devel@linuxdriverproject.org, virtualization@lists.osdl.org
In-Reply-To: <20110509030452.GA17182@kroah.com>
> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Sunday, May 08, 2011 11:05 PM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: various vmbus review comments
>
> On Mon, May 09, 2011 at 01:46:56AM +0000, KY Srinivasan wrote:
> > > - the instances of hv_driver structures need to be static and
> > > not programatically defined, like all other USB and PCI
> > > drivers are handled.
> >
> > Done. You had expressed some concern that this would expose some issue
> > with the core vmbus driver (which is what I want to concentrate on this
> > go around). I have done this for both the block driver and the mouse driver
> > and I am pretty sure I can do the same with the network driver. I have not
> > currently done this for the network driver, since the number of patches I have
> > to submit is already very large.
>
> Ok, but it shouldn't be a major change to the code, right?
Right, it is not. I will submit code for the net driver after I am done with this patch-set.
>
> > > - module reference counting. Are you sure you got it all right
> > > for the individual modules that attach to the bus? I don't
> > > see any reference counting happening, is that correct?
> >
> > I have already exchanged an email with you on this. To summarize, it
> > does not look like there is a problem
> >
> > > - fix the sparse warnings.
> > Mostly done; most of the errors are in the base kernel coming out of
> > the macro page_to_pfn()
> >
> > > - fix the use of volatile in the ring buffer code. It should
> > > not be needed and if you are relying on it, then the code is
> > > wrong.
> >
> > You are right; all accesses were already serialized with a spin lock and the
> > Volatile attribute was unnecessary.
> >
> > > - fix the namespace on the ringbuffer code to show that it
> > > really is only for the hyperv bus code internally.
> >
> > Done.
> >
> > >
> > > That should be enough for at least one more set of patches for you all
> > > to work on :)
> >
> > Greg,
> >
> > I have had so much fun cleaning up these drivers that I lost track of the patch
> count.
> > I have addressed all the issues you have raised in addition to some other
> cleanup
> > that I was doing since about a week. As I look at the patch-set, I have little over
> > 200 patches. If it is ok with you, I would like to send them as a single set. Let me
> know
> > what you prefer. I need to circulate these patches internally before I can send
> them out.
> > I should be able to send these out early next week.
>
> A single set is fine, if that's what you want to do, I can handle that
> amount as long as they all build all along the way and don't break
> anything.
Thanks Greg.
Regards,
K. Y
^ permalink raw reply
* Re: various vmbus review comments
From: Christoph Hellwig @ 2011-05-09 14:33 UTC (permalink / raw)
To: KY Srinivasan
Cc: virtualization@lists.osdl.org, gregkh@suse.de,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org
In-Reply-To: <6E21E5352C11B742B20C142EB499E0481E0694@TK5EX14MBXC124.redmond.corp.microsoft.com>
On Fri, May 06, 2011 at 01:10:38PM +0000, KY Srinivasan wrote:
> I audited the block and the net drivers. As part of their exit routine,
> they invoke vmbus_child_driver_unregister() after properly cleaning
> up all the devices they are managing. Do you still see an issue with
> regards to module reference counting.
Which is not the correct thing to do as explained in my last round
of reviews. Take a look at the PCI code - the functional driver only
does a foo_untegister_driver (which maps almost directly to
driver_unregister), which then causes the device core to unbind the
devices. The function driver must never call device_unregister
directly as the device continues to exist even if no driver is bound to
it.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox