Re: [PATCH 2/3] virtio ring implementation

[Dor Laor <dor.laor-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2468 bytes --]

Rusty Russell wrote:
>
> On Mon, 2007-09-24 at 15:44 +0200, Dor Laor wrote:
> > Rusty Russell wrote:
> > > +irqreturn_t vring_interrupt(int irq, void *_vq)
> > > +{
> > > +       struct vring_virtqueue *vq = to_vvq(_vq);
> > > +
> > > +       pr_debug("virtqueue interrupt for %p\n", vq);
> > > +
> > > +       if (unlikely(vq->broken))
> > > +               return IRQ_HANDLED;
> > > +
> > > +       if (more_used(vq)) {
> > > +               pr_debug("virtqueue callback for %p (%p)\n",
> > > +                        vq, vq->vq.callback);
> > > +               if (!vq->vq.callback)
> > > +                       return IRQ_NONE;
> > > +               if (!vq->vq.callback(&vq->vq))
> > > +                       vq->vring.avail->flags |=
> > > VRING_AVAIL_F_NO_INTERRUPT;
> > > +       } else
> > > +               pr_debug("virtqueue %p no more used\n", vq);
> > > +
> > > +       return IRQ_HANDLED;
> > > +}
> > > +
> > >
> > Seems like there is a problem with shared irq line, the interrupt
> > handler always returns IRQ_HANDLED (except for the trivial case
> > were the callback is null).
>
> To reply for a second time, this time with thought.  Sorry.
>
> Yes, this code is wrong.  It should be:
>
> irqreturn_t vring_interrupt(int irq, void *_vq)
> {
>         struct vring_virtqueue *vq = to_vvq(_vq);
>
>         if (!more_used(vq)) {
>                 pr_debug("virtqueue interrupt with no work for %p\n", vq);
>                 return IRQ_NONE;
>         }
>
>         if (unlikely(vq->broken))
>                 return IRQ_HANDLED;
>
>         pr_debug("virtqueue callback for %p (%p)\n", vq, vq->vq.callback);
>         if (vq->vq.callback && !vq->vq.callback(&vq->vq))
>                 vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
>
>         return IRQ_HANDLED;
> }
>
> Cheers,
> Rusty.
>
At the moment it's not good enough, there is a potential race were the 
guest optimistically turn off
the VRING_AVAIL_F_NO_INTERRUPT in the vring_restart and afterwards finds 
there are more_used so
it consume them in the poll function.
If in the middle, packets arrive the host will see the flag is off and 
send irq.
In that case the above irq handler will report IRQ_NONE.

It's also not trivial to cancel the optimistic approach in the restart 
function since then there might be another race
that will result in missing irq reaching the guest.
I'll try to think of something better than a hypercall for switching irq 
on/off.
Cheers mate,
Dor.

[-- Attachment #1.2: Type: text/html, Size: 4910 bytes --]

[-- Attachment #2: Type: text/plain, Size: 228 bytes --]

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

[-- Attachment #3: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel