On Mon, May 24, 2010 at 08:52:40AM -0700, David Stevens wrote: > "Michael S. Tsirkin" <m...@redhat.com> wrote on 05/24/2010 03:17:10 AM: > > > On Thu, May 20, 2010 at 09:58:06AM -0700, David L Stevens wrote: > > > [for Michael Tsirkin's vhost development git tree] > > > > > > This patch fixes a race between guest and host when > > > adding used buffers wraps the ring. Without it, guests > > > can see partial packets before num_buffers is set in > > > the vnet header. > > > > > > Signed-off-by: David L Stevens <dlstev...@us.ibm.com> > > > > Could you please explain what the race is? > > Sure. The pre-patch code in the ring-wrap case > does this: > > add part1 bufs > update used index > add part2 bufs > update used index > > After we update the used index for part1, the part1 > buffers are available to the guest. If the guest is > consuming at that point, it can process the partial > packet before the rest of the packet is there. In that > case, num_buffers will be greater than the number of > buffers available to the guest and it'll drop the > packet with a framing error. I was seeing 2 or 3 framing > errors every 100 million packets or so pre-patch, none > post-patch. > Actually, the second sentence is incorrect in the > original description-- num_buffers is up to date when > the guest sees it, but the used index is not. > > +-DLS
so this happens always - what does wrap-around refer to? > > > > > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c > > > index 7f2568d..74790ab 100644 > > > --- a/drivers/vhost/vhost.c > > > +++ b/drivers/vhost/vhost.c > > > @@ -1065,14 +1065,6 @@ static int __vhost_add_used_n(struct > vhost_virtqueue *vq, > > > vq_err(vq, "Failed to write used"); > > > return -EFAULT; > > > } > > > - /* Make sure buffer is written before we update index. */ > > > - smp_wmb(); > > > - if (put_user(vq->last_used_idx + count, &vq->used->idx)) { > > > - vq_err(vq, "Failed to increment used idx"); > > > - return -EFAULT; > > > - } > > > - if (unlikely(vq->log_used)) > > > - vhost_log_used(vq, used); > > > vq->last_used_idx += count; > > > return 0; > > > } > > > @@ -1093,7 +1085,17 @@ int vhost_add_used_n(struct vhost_virtqueue > *vq, > > struct vring_used_elem *heads, > > > heads += n; > > > count -= n; > > > } > > > - return __vhost_add_used_n(vq, heads, count); > > > + r = __vhost_add_used_n(vq, heads, count); > > > + > > > + /* Make sure buffer is written before we update index. */ > > > + smp_wmb(); > > > + if (put_user(vq->last_used_idx, &vq->used->idx)) { > > > + vq_err(vq, "Failed to increment used idx"); > > > + return -EFAULT; > > > + } > > > + if (unlikely(vq->log_used)) > > > + vhost_log_used(vq, vq->used->ring + start); > > > + return r; > > > } I think a single vhost_log_used will not DTRT here: it only updates log for a single entry. So we'll need to split this to functions that 1. log used entries writes: called from __vhost_add_used_n 2. log used index write: called from vhost_add_used_n > > > > > > /* This actually signals the guest, using eventfd. */ > > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html