From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dor Laor Subject: Re: [PATCH 2/3] virtio ring implementation Date: Tue, 25 Sep 2007 19:15:47 +0200 Message-ID: <46F94243.2000602@qumranet.com> References: <1190625194.27805.199.camel@localhost.localdomain> <1190625256.27805.201.camel@localhost.localdomain> <1190625307.27805.203.camel@localhost.localdomain> <46F7BF41.9060705@qumranet.com> <1190727156.27805.332.camel@localhost.localdomain> Reply-To: dor.laor-atKUWr5tajBWk0Htik3J/w@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0796708391==" Cc: kvm-devel , virtualization To: Rusty Russell Return-path: In-Reply-To: <1190727156.27805.332.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org This is a multi-part message in MIME format. --===============0796708391== Content-Type: multipart/alternative; boundary="------------010505020808080208030405" This is a multi-part message in MIME format. --------------010505020808080208030405 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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. --------------010505020808080208030405 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Rusty Russell wrote:
Re: [kvm-devel] [PATCH 2/3] virtio ring implementation

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.
 --------------010505020808080208030405-- --===============0796708391== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline ------------------------------------------------------------------------- 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/ --===============0796708391== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ kvm-devel mailing list kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org https://lists.sourceforge.net/lists/listinfo/kvm-devel --===============0796708391==--