Re: [kvm-devel] [PATCH 2/3] virtio ring implementation
Rusty Russell wrote: > On Tue, 2007-09-25 at 19:15 +0200, Dor Laor wrote: > >> 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. >> > > Good point. On the one hand, reporting IRQ_NONE occasionally is not > fatal. On the other, it'd be nice to get this right. > > >> 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 did it optimistically because otherwise we need two barriers (and > still have a window). > > >> I'll try to think of something better than a hypercall for switching >> irq on/off. >> > > Erk. That would be v. bad... > Actually double barrier will satisfy non-shared irq lines and will report irq handling right. The more complex scenario is shared irqs (pci...), since several devices are raising the irq line (level triggered) in the same time, while the tap add receive packets it always leaves a potential race for irq handled return value. So seems like we better not have shared irq and if only if shared irq is a must we should do the naughty naughty things. If returning IRQ_NONE once in a while is not too bad (including the matching windows implementation) then there is no issue. Dor. - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH 2/3] virtio ring implementation
On Tue, 2007-09-25 at 19:15 +0200, Dor Laor wrote: > 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. Good point. On the one hand, reporting IRQ_NONE occasionally is not fatal. On the other, it'd be nice to get this right. > 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 did it optimistically because otherwise we need two barriers (and still have a window). > I'll try to think of something better than a hypercall for switching > irq on/off. Erk. That would be v. bad... Thanks, Rusty. - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH 2/3] virtio ring implementation
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.
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
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.
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH 2/3] virtio ring implementation
On Mon, 2007-09-24 at 15:44 +0200, Dor Laor wrote: > 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). > > It can be solved by having a host irq counter (in the shared ring) and > a guest irq counter and return > mb(); return (host_counter!=guest_counter)? IRQ_HANDLED:IRQ_NONE; Or we could make the callback return irqreturn_t and have an explicit "disable" hook to disable interrupts. Using the return value of the callback to disable the queue has always made be a little uncomfortable, but it's slightly more efficient than a separate hook. Thanks, Rusty. - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH 2/3] virtio ring implementation
Rusty Russell wrote:
These helper routines supply most of the virtqueue_ops for hypervisors
which want to use a ring for virtio. Unlike the previous lguest
implementation:
1) The rings are variable sized (2^n-1 elements).
2) They have an unfortunate limit of 65535 bytes per sg element.
3) The page numbers are always 64 bit (PAE anyone?)
4) They no longer place used[] on a separate page, just a separate
cacheline.
5) We do a modulo on a variable. We could be tricky if we cared.
6) Interrupts and notifies are suppressed using flags within the rings.
Users need only implement the new_vq and free_vq hooks (KVM wants the
guest to allocate the rings, lguest does it sanely).
Signed-off-by: Rusty Russell <[EMAIL PROTECTED]>
[snip]
+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).
It can be solved by having a host irq counter (in the shared ring) and a
guest irq counter and return
mb(); return (host_counter!=guest_counter)? IRQ_HANDLED:IRQ_NONE;
Dor.
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
