From: "Michael S. Tsirkin" <mst@redhat.com> To: Rusty Russell <rusty@rustcorp.com.au> Cc: linux-kernel@vger.kernel.org, Carsten Otte <cotte@de.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, linux390@de.ibm.com, Martin Schwidefsky <schwidefsky@de.ibm.com>, Heiko Carstens <heiko.carstens@de.ibm.com>, Shirley Ma <xma@us.ibm.com>, lguest@lists.ozlabs.org, virtualization@lists.linux-foundation.org, netdev@vger.kernel.org, linux-s390@vger.kernel.org, kvm@vger.kernel.org, Krishna Kumar <krkumar2@in.ibm.com>, Tom Lendacky <tahm@linux.vnet.ibm.com>, steved@us.ibm.com, habanero@linux.vnet.ibm.com Subject: Re: [PATCH 09/18] virtio: use avail_event index Date: Sun, 15 May 2011 16:55:41 +0300 [thread overview] Message-ID: <20110515135541.GF24932@redhat.com> (raw) In-Reply-To: <874o54h4rt.fsf@rustcorp.com.au> On Mon, May 09, 2011 at 02:03:26PM +0930, Rusty Russell wrote: > 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 :) You mean ASCII art in comments? > > @@ -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); Hmm, but does it say that somewhere? It seems that most drivers use locking to prevent posting more buffers while they drain the used ring, and also kick at least once in vq->num buffers, which I guess in the end would work out fine as vq num is never as large as 2^15. > > +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? If we kill the code above as you suggested, likely yes. Maybe an explicit flag is more obvious? > > @@ -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. Well if we don't support the feature in the ring we should not ack the feature, right?
WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> To: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> Cc: Krishna Kumar <krkumar2-xthvdsQ13ZrQT0dZR+AlfA@public.gmane.org>, Carsten Otte <cotte-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>, lguest-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Shirley Ma <xma-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-s390-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, habanero-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org, Heiko Carstens <heiko.carstens-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, steved-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org, Christian Borntraeger <borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>, Tom Lendacky <tahm-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>, Martin Schwidefsky <schwidefsky-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>, linux390-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org Subject: Re: [PATCH 09/18] virtio: use avail_event index Date: Sun, 15 May 2011 16:55:41 +0300 [thread overview] Message-ID: <20110515135541.GF24932@redhat.com> (raw) In-Reply-To: <874o54h4rt.fsf-8n+1lVoiYb80n/F98K4Iww@public.gmane.org> On Mon, May 09, 2011 at 02:03:26PM +0930, Rusty Russell wrote: > On Wed, 4 May 2011 23:51:47 +0300, "Michael S. Tsirkin" <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > > Use the new avail_event feature to reduce the number > > of exits from the guest. > > Figures here would be nice :) You mean ASCII art in comments? > > @@ -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); Hmm, but does it say that somewhere? It seems that most drivers use locking to prevent posting more buffers while they drain the used ring, and also kick at least once in vq->num buffers, which I guess in the end would work out fine as vq num is never as large as 2^15. > > +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? If we kill the code above as you suggested, likely yes. Maybe an explicit flag is more obvious? > > @@ -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. Well if we don't support the feature in the ring we should not ack the feature, right?
next prev parent reply other threads:[~2011-05-15 13:55 UTC|newest] Thread overview: 144+ messages / expand[flat|nested] mbox.gz Atom feed top 2011-05-04 20:50 [PATCH 00/18] virtio and vhost-net performance enhancements Michael S. Tsirkin 2011-05-04 20:50 ` Michael S. Tsirkin 2011-05-04 20:50 ` [PATCH 01/18] virtio: 64 bit features Michael S. Tsirkin 2011-05-04 20:50 ` Michael S. Tsirkin 2011-05-04 20:50 ` Michael S. Tsirkin 2011-05-04 20:50 ` [PATCH 02/18] virtio_test: update for " Michael S. Tsirkin 2011-05-04 20:50 ` Michael S. Tsirkin 2011-05-04 20:50 ` Michael S. Tsirkin 2011-05-04 20:50 ` [PATCH 03/18] vhost: fix " Michael S. Tsirkin 2011-05-04 20:50 ` Michael S. Tsirkin 2011-05-04 20:50 ` Michael S. Tsirkin 2011-05-04 20:51 ` [PATCH 04/18] virtio: don't delay avail index update Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 20:51 ` [PATCH 05/18] virtio: used event index interface Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 21:56 ` Tom Lendacky 2011-05-04 21:56 ` Tom Lendacky 2011-05-05 9:38 ` Michael S. Tsirkin 2011-05-05 9:38 ` Michael S. Tsirkin 2011-05-04 20:51 ` [PATCH 06/18] virtio_ring: avail " Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-09 4:13 ` Rusty Russell 2011-05-09 4:13 ` Rusty Russell 2011-05-09 4:13 ` Rusty Russell 2011-05-15 12:47 ` Michael S. Tsirkin 2011-05-15 12:47 ` Michael S. Tsirkin 2011-05-15 12:47 ` Michael S. Tsirkin 2011-05-16 6:23 ` Rusty Russell 2011-05-16 6:23 ` Rusty Russell 2011-05-16 6:23 ` Rusty Russell 2011-05-17 6:00 ` Michael S. Tsirkin 2011-05-17 6:00 ` Michael S. Tsirkin 2011-05-17 6:00 ` Michael S. Tsirkin 2011-05-18 0:08 ` Rusty Russell 2011-05-18 0:08 ` Rusty Russell 2011-05-18 0:08 ` Rusty Russell 2011-05-04 20:51 ` [PATCH 07/18] virtio ring: inline function to check for events Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-05 8:34 ` Stefan Hajnoczi 2011-05-05 8:56 ` Michael S. Tsirkin 2011-05-05 8:56 ` Michael S. Tsirkin 2011-05-05 8:34 ` Stefan Hajnoczi 2011-05-04 20:51 ` [PATCH 08/18] virtio_ring: support for used_event idx feature Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-09 4:17 ` Rusty Russell 2011-05-09 4:17 ` Rusty Russell 2011-05-09 4:17 ` Rusty Russell 2011-05-15 12:47 ` Michael S. Tsirkin 2011-05-15 12:47 ` Michael S. Tsirkin 2011-05-15 12:47 ` Michael S. Tsirkin 2011-05-04 20:51 ` [PATCH 09/18] virtio: use avail_event index Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 21:58 ` Tom Lendacky 2011-05-04 21:58 ` Tom Lendacky 2011-05-05 9:34 ` Michael S. Tsirkin 2011-05-05 9:34 ` Michael S. Tsirkin 2011-05-09 4:33 ` Rusty Russell 2011-05-09 4:33 ` Rusty Russell 2011-05-09 4:33 ` Rusty Russell 2011-05-15 13:55 ` Michael S. Tsirkin 2011-05-15 13:55 ` Michael S. Tsirkin [this message] 2011-05-15 13:55 ` Michael S. Tsirkin 2011-05-16 7:12 ` Rusty Russell 2011-05-16 7:12 ` Rusty Russell 2011-05-16 7:12 ` Rusty Russell 2011-05-17 6:10 ` Michael S. Tsirkin 2011-05-17 6:10 ` Michael S. Tsirkin 2011-05-18 0:19 ` Rusty Russell 2011-05-18 0:19 ` Rusty Russell 2011-05-18 0:19 ` Rusty Russell 2011-05-18 5:43 ` Michael S. Tsirkin 2011-05-18 5:43 ` Michael S. Tsirkin 2011-05-18 5:43 ` Michael S. Tsirkin 2011-05-19 7:27 ` Michael S. Tsirkin 2011-05-19 7:27 ` Michael S. Tsirkin 2011-05-19 7:27 ` Michael S. Tsirkin 2011-05-17 6:10 ` Michael S. Tsirkin 2011-05-17 16:23 ` Tom Lendacky 2011-05-17 16:23 ` Tom Lendacky 2011-05-04 20:51 ` [PATCH 10/18] vhost: utilize used_event index Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 20:51 ` Michael S. Tsirkin 2011-05-04 20:52 ` [PATCH 11/18] vhost: support avail_event idx Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:52 ` [PATCH 12/18] virtio_test: support used_event index Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:52 ` [PATCH 13/18] virtio_test: avail_event index support Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:52 ` [PATCH 14/18] virtio: add api for delayed callbacks Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-09 5:57 ` Rusty Russell 2011-05-09 5:57 ` Rusty Russell 2011-05-09 5:57 ` Rusty Russell 2011-05-15 12:48 ` Michael S. Tsirkin 2011-05-15 12:48 ` Michael S. Tsirkin 2011-05-15 12:48 ` Michael S. Tsirkin 2011-05-16 7:13 ` Rusty Russell 2011-05-16 7:13 ` Rusty Russell 2011-05-16 7:13 ` Rusty Russell 2011-05-19 7:24 ` Michael S. Tsirkin 2011-05-19 7:24 ` Michael S. Tsirkin 2011-05-20 7:43 ` Rusty Russell 2011-05-20 7:43 ` Rusty Russell 2011-05-20 7:43 ` Rusty Russell 2011-05-19 7:24 ` Michael S. Tsirkin 2011-05-04 20:52 ` [PATCH 15/18] virtio_net: delay TX callbacks Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:52 ` [PATCH 16/18] virtio_ring: Add capacity check API Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:52 ` Michael S. Tsirkin 2011-05-04 20:53 ` [PATCH 17/18] virtio_net: fix TX capacity checks using new API Michael S. Tsirkin 2011-05-04 20:53 ` Michael S. Tsirkin 2011-05-04 20:53 ` Michael S. Tsirkin 2011-05-04 20:53 ` [PATCH 18/18] virtio_net: limit xmit polling Michael S. Tsirkin 2011-05-04 20:53 ` Michael S. Tsirkin 2011-05-04 20:53 ` Michael S. Tsirkin 2011-05-05 15:07 ` [PATCH 0/3] virtio and vhost-net performance enhancements Michael S. Tsirkin 2011-05-05 15:07 ` Michael S. Tsirkin 2011-05-05 15:08 ` [PATCH 1/3] virtio: fix avail event support Michael S. Tsirkin 2011-05-05 15:08 ` Michael S. Tsirkin 2011-05-05 15:08 ` Michael S. Tsirkin 2011-05-05 15:08 ` [PATCH 2/3] virtio_ring: check used_event offset Michael S. Tsirkin 2011-05-05 15:08 ` Michael S. Tsirkin 2011-05-05 15:08 ` Michael S. Tsirkin 2011-05-05 15:08 ` [PATCH 3/3] virtio_ring: need_event api comment fix Michael S. Tsirkin 2011-05-05 15:08 ` Michael S. Tsirkin 2011-05-05 15:08 ` Michael S. Tsirkin 2011-05-09 5:59 ` Rusty Russell 2011-05-09 5:59 ` Rusty Russell 2011-05-09 5:59 ` Rusty Russell 2011-05-05 15:07 ` [PATCH 0/3] virtio and vhost-net performance enhancements Michael S. Tsirkin 2011-05-11 17:10 ` [PATCH 00/18] " Krishna Kumar2 2011-05-11 17:10 ` Krishna Kumar2
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20110515135541.GF24932@redhat.com \ --to=mst@redhat.com \ --cc=borntraeger@de.ibm.com \ --cc=cotte@de.ibm.com \ --cc=habanero@linux.vnet.ibm.com \ --cc=heiko.carstens@de.ibm.com \ --cc=krkumar2@in.ibm.com \ --cc=kvm@vger.kernel.org \ --cc=lguest@lists.ozlabs.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-s390@vger.kernel.org \ --cc=linux390@de.ibm.com \ --cc=netdev@vger.kernel.org \ --cc=rusty@rustcorp.com.au \ --cc=schwidefsky@de.ibm.com \ --cc=steved@us.ibm.com \ --cc=tahm@linux.vnet.ibm.com \ --cc=virtualization@lists.linux-foundation.org \ --cc=xma@us.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.