lkml.org 
[lkml]   [2011]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5 of 5] virtio: expose added descriptors immediately
    On Sun, Nov 13, 2011 at 11:03:13PM +0200, Michael S. Tsirkin wrote:
    > On Thu, Nov 03, 2011 at 06:12:53PM +1030, Rusty Russell wrote:
    > > A virtio driver does virtqueue_add_buf() multiple times before finally
    > > calling virtqueue_kick(); previously we only exposed the added buffers
    > > in the virtqueue_kick() call. This means we don't need a memory
    > > barrier in virtqueue_add_buf(), but it reduces concurrency as the
    > > device (ie. host) can't see the buffers until the kick.
    > >
    > > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
    >
    > In the past I played with a patch like this, but I didn't see a
    > performance gain either way. Do you see any gain?
    >
    > I'm a bit concerned that with this patch, a buggy driver that
    > adds more than 2^16 descriptors without a kick
    > would seem to work sometimes. Let's add WARN_ON(vq->num_added > (1 << 16))?

    Thinking about it more - it might be tricky for drivers
    to ensure this. add used to fail when vq is full, but now
    driver might do get between add and notify:
    lock
    add_buf * N
    prep
    unlock
    lock
    get_buf * N
    unlock
    lock
    add_buf
    prep
    unlock
    notify

    and since add was followed by get, this doesn't fail.

    So the right thing to do I think is to either ignore indexes and assume
    a kick is needed, something like:
    if vq->num_added >= (1 << 15))
    needs_kick = true
    (note: maybe it's 1<<16, and maybe >, but 1<<15 is plenty anyway)

    Or alternatively, fail add when num_added is too large.

    > > ---
    > > drivers/virtio/virtio_ring.c | 37 ++++++++++++++++++++++---------------
    > > 1 file changed, 22 insertions(+), 15 deletions(-)
    > >
    > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
    > > --- a/drivers/virtio/virtio_ring.c
    > > +++ b/drivers/virtio/virtio_ring.c
    > > @@ -227,9 +227,15 @@ add_head:
    > >
    > > /* Put entry in available array (but don't update avail->idx until they
    > > * do sync). */
    > > - avail = ((vq->vring.avail->idx + vq->num_added++) & (vq->vring.num-1));
    > > + avail = (vq->vring.avail->idx & (vq->vring.num-1));
    > > vq->vring.avail->ring[avail] = head;
    > >
    > > + /* Descriptors and available array need to be set before we expose the
    > > + * new available array entries. */
    > > + virtio_wmb();
    > > + vq->vring.avail->idx++;
    > > + vq->num_added++;
    > > +
    > > pr_debug("Added buffer head %i to %p\n", head, vq);
    > > END_USE(vq);
    > >
    > > @@ -248,13 +254,10 @@ bool virtqueue_kick_prepare(struct virtq
    > > * new available array entries. */
    > > virtio_wmb();
    > >
    > > - old = vq->vring.avail->idx;
    > > - new = vq->vring.avail->idx = old + vq->num_added;
    > > + old = vq->vring.avail->idx - vq->num_added;
    > > + new = vq->vring.avail->idx;
    > > vq->num_added = 0;
    > >
    > > - /* Need to update avail index before checking if we should notify */
    > > - virtio_mb();
    > > -
    > > if (vq->event) {
    > > needs_kick = vring_need_event(vring_avail_event(&vq->vring),
    > > new, old);
    > >
    > >
    > > _______________________________________________
    > > Virtualization mailing list
    > > Virtualization@lists.linux-foundation.org
    > > https://lists.linuxfoundation.org/mailman/listinfo/virtualization


    \
     
     \ /
      Last update: 2011-11-14 07:57    [W:7.603 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site