[PATCH resent] virtio_net: Fix stalled inbound traffic on early packets

[PATCH resent] virtio_net: Fix stalled inbound traffic on early packets

[Christian Borntraeger]

Hello Rusty,

while implementing and testing virtio on s390 I found a problem in
virtio_net: The current virtio_net driver has a startup race, which
prevents any incoming traffic:

If try_fill_recv submits buffers to the host system data might be
filled in and an interrupt is sent, before napi_enable finishes.
In that case the interrupt will kick skb_recv_done which will then
call netif_rx_schedule. netif_rx_schedule checks, if NAPI_STATE_SCHED
is set - which is not as we did not run napi_enable. No poll routine
is scheduled. Furthermore, skb_recv_done returns false, we disables
interrupts for this device.

One solution is the enable napi before inbound buffer are available.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 drivers/net/virtio_net.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: kvm/drivers/net/virtio_net.c
===================================================================
--- kvm.orig/drivers/net/virtio_net.c
+++ kvm/drivers/net/virtio_net.c
@@ -285,13 +285,15 @@ static int virtnet_open(struct net_devic
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 
+	napi_enable(&vi->napi);
 	try_fill_recv(vi);
 
 	/* If we didn't even get one input buffer, we're useless. */
-	if (vi->num == 0)
+	if (vi->num == 0) {
+		napi_disable(&vi->napi);
 		return -ENOMEM;
+	}
 
-	napi_enable(&vi->napi);
 	return 0;
 }
 

Re: [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Dor Laor]

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

Christian Borntraeger wrote:
>
> Hello Rusty,
>
> while implementing and testing virtio on s390 I found a problem in
> virtio_net: The current virtio_net driver has a startup race, which
> prevents any incoming traffic:
>
> If try_fill_recv submits buffers to the host system data might be
> filled in and an interrupt is sent, before napi_enable finishes.
> In that case the interrupt will kick skb_recv_done which will then
> call netif_rx_schedule. netif_rx_schedule checks, if NAPI_STATE_SCHED
> is set - which is not as we did not run napi_enable. No poll routine
> is scheduled. Furthermore, skb_recv_done returns false, we disables
> interrupts for this device.
>
> One solution is the enable napi before inbound buffer are available.
>
But then you might get recv interrupt without a buffer.
The way other physical NICs doing it is by dis/en/abling interrupt using 
registers (look at e1000).
I suggest we can export add_status and use the original code but
before enabling napi add a call to add_status(dev, VIRTIO_CONFIG_DEV_OPEN).
The host won't trigger an irq until it sees the above.

BTW: Rusty is on vacation and that's probably the reason he didn't respond.
Regards,
Dor.
>
> Signed-off-by: Christian Borntraeger <borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/net/virtio_net.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> Index: kvm/drivers/net/virtio_net.c
> ===================================================================
> --- kvm.orig/drivers/net/virtio_net.c
> +++ kvm/drivers/net/virtio_net.c
> @@ -285,13 +285,15 @@ static int virtnet_open(struct net_devic
>  {
>         struct virtnet_info *vi = netdev_priv(dev);
>
> +       napi_enable(&vi->napi);
>         try_fill_recv(vi);
>
>         /* If we didn't even get one input buffer, we're useless. */
> -       if (vi->num == 0)
> +       if (vi->num == 0) {
> +               napi_disable(&vi->napi);
>                 return -ENOMEM;
> +       }
>
> -       napi_enable(&vi->napi);
>         return 0;
>  }
>
>
> -------------------------------------------------------------------------
> SF.Net email is sponsored by:
> Check out the new SourceForge.net Marketplace.
> It's the best place to buy or sell services for
> just about anything Open Source.
> http://sourceforge.net/services/buy/index.php
> _______________________________________________
> kvm-devel mailing list
> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>


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

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

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

[-- 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

Re: [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Dor Laor]

This time I send in text so netdev list won't reject it; sorry.
Dor Laor wrote:
> Christian Borntraeger wrote:
>>
>> Hello Rusty,
>>
>> while implementing and testing virtio on s390 I found a problem in
>> virtio_net: The current virtio_net driver has a startup race, which
>> prevents any incoming traffic:
>>
>> If try_fill_recv submits buffers to the host system data might be
>> filled in and an interrupt is sent, before napi_enable finishes.
>> In that case the interrupt will kick skb_recv_done which will then
>> call netif_rx_schedule. netif_rx_schedule checks, if NAPI_STATE_SCHED
>> is set - which is not as we did not run napi_enable. No poll routine
>> is scheduled. Furthermore, skb_recv_done returns false, we disables
>> interrupts for this device.
>>
>> One solution is the enable napi before inbound buffer are available.
>>
> But then you might get recv interrupt without a buffer.
> The way other physical NICs doing it is by dis/en/abling interrupt 
> using registers (look at e1000).
> I suggest we can export add_status and use the original code but
> before enabling napi add a call to add_status(dev, 
> VIRTIO_CONFIG_DEV_OPEN).
> The host won't trigger an irq until it sees the above.
>
> BTW: Rusty is on vacation and that's probably the reason he didn't 
> respond.
> Regards,
> Dor.
>>
>> Signed-off-by: Christian Borntraeger <borntraeger-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/net/virtio_net.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> Index: kvm/drivers/net/virtio_net.c
>> ===================================================================
>> --- kvm.orig/drivers/net/virtio_net.c
>> +++ kvm/drivers/net/virtio_net.c
>> @@ -285,13 +285,15 @@ static int virtnet_open(struct net_devic
>>  {
>>         struct virtnet_info *vi = netdev_priv(dev);
>>
>> +       napi_enable(&vi->napi);
>>         try_fill_recv(vi);
>>
>>         /* If we didn't even get one input buffer, we're useless. */
>> -       if (vi->num == 0)
>> +       if (vi->num == 0) {
>> +               napi_disable(&vi->napi);
>>                 return -ENOMEM;
>> +       }
>>
>> -       napi_enable(&vi->napi);
>>         return 0;
>>  }
>>
>>
>> -------------------------------------------------------------------------
>> SF.Net email is sponsored by:
>> Check out the new SourceForge.net Marketplace.
>> It's the best place to buy or sell services for
>> just about anything Open Source.
>> http://sourceforge.net/services/buy/index.php
>> _______________________________________________
>> kvm-devel mailing list
>> kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
>> https://lists.sourceforge.net/lists/listinfo/kvm-devel
>>
>


-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

Re: [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Christian Borntraeger]

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

Dor Laor wrote:
> Christian Borntraeger wrote:
>>
>> Hello Rusty,
>>
>> while implementing and testing virtio on s390 I found a problem in
>> virtio_net: The current virtio_net driver has a startup race, which
>> prevents any incoming traffic:
>>
>> If try_fill_recv submits buffers to the host system data might be
>> filled in and an interrupt is sent, before napi_enable finishes.
>> In that case the interrupt will kick skb_recv_done which will then
>> call netif_rx_schedule. netif_rx_schedule checks, if NAPI_STATE_SCHED
>> is set - which is not as we did not run napi_enable. No poll routine
>> is scheduled. Furthermore, skb_recv_done returns false, we disables
>> interrupts for this device.
>>
>> One solution is the enable napi before inbound buffer are available.
>>
> But then you might get recv interrupt without a buffer.

If I look at the current implementation (lguest) no interrupt is sent if
there is not buffer available, no?
On the other hand, if the host does send an interrupt when no buffer is
available, this is also no problem. Looking at virtnet_poll, it seems it
can cope with an empty ring. 

> The way other physical NICs doing it is by dis/en/abling interrupt 
> using registers (look at e1000).
> I suggest we can export add_status and use the original code but
> before enabling napi add a call to add_status(dev, 
> VIRTIO_CONFIG_DEV_OPEN).
> The host won't trigger an irq until it sees the above.

That would also work. We already have VRING_AVAIL_F_NO_INTERRUPT in
virtio_ring.c - maybe we can use that. Its hidden in callback and
restart handling, what about adding an explicit startup?
>
> BTW: Rusty is on vacation and that's probably the reason he didn't 
> respond.
> Regards,
> Dor.

Ok, didnt know that. At the moment I can live with my private patch
while we work on a final solution. Meanwhile I will try to debug virtio
on SMP guests - I still see some strange races on our test system. (But
block and net is now working on s390 and can cope with medium load. )

Christian



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

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

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

[-- 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

Re: [kvm-devel] [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Rusty Russell]

On Wednesday 12 December 2007 00:16:12 Christian Borntraeger wrote:
> That would also work. We already have VRING_AVAIL_F_NO_INTERRUPT in
> virtio_ring.c - maybe we can use that. Its hidden in callback and
> restart handling, what about adding an explicit startup?

Yes, I debated whether to make this a separate hook or not; the current method 
reduces the number of function calls without having two ways of disabling 
callbacks.

In this case, simply starting devices with callbacks disabled and 
renaming 'restart' to 'enable' (or something) and calling it at the beginning 
is probably sufficient?

Thanks for all this debugging!  Will apply next week when I'm back...

> Ok, didnt know that. At the moment I can live with my private patch
> while we work on a final solution. Meanwhile I will try to debug virtio
> on SMP guests - I still see some strange races on our test system. (But
> block and net is now working on s390 and can cope with medium load. )

Obviously SMP is not something I tested under lguest, so can believe there are 
nasty lurking bugs.  Hope they're not too deep...

Thanks!
Rusty.

Re: [kvm-devel] [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Christian Borntraeger]

Am Mittwoch, 12. Dezember 2007 schrieb Rusty Russell:
> On Wednesday 12 December 2007 00:16:12 Christian Borntraeger wrote:
> > That would also work. We already have VRING_AVAIL_F_NO_INTERRUPT in
> > virtio_ring.c - maybe we can use that. Its hidden in callback and
> > restart handling, what about adding an explicit startup?
> 
> Yes, I debated whether to make this a separate hook or not; the current
> method  reduces the number of function calls without having two ways of
> disabling  callbacks.
> 
> In this case, simply starting devices with callbacks disabled and 
> renaming 'restart' to 'enable' (or something) and calling it at the
> beginning is probably sufficient?

So you suggest something like the following patch? It seems to work but 
there is still a theoretical race at startup. Therefore, I tend to agree 
with Dor that a separate hook seems prefereable, so I am not fully sure if 
the patch is the final solution:

ps: Its ok to answer that after your vacation. 

---
 drivers/block/virtio_blk.c   |    3 ++-
 drivers/net/virtio_net.c     |    5 ++++-
 drivers/virtio/virtio_ring.c |    9 ++++-----
 include/linux/virtio.h       |    4 ++--
 4 files changed, 12 insertions(+), 9 deletions(-)

Index: kvm/drivers/virtio/virtio_ring.c
===================================================================
--- kvm.orig/drivers/virtio/virtio_ring.c
+++ kvm/drivers/virtio/virtio_ring.c
@@ -220,7 +220,7 @@ static void *vring_get_buf(struct virtqu
 	return ret;
 }
 
-static bool vring_restart(struct virtqueue *_vq)
+static bool vring_enable(struct virtqueue *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 
@@ -264,7 +264,7 @@ static struct virtqueue_ops vring_vq_ops
 	.add_buf = vring_add_buf,
 	.get_buf = vring_get_buf,
 	.kick = vring_kick,
-	.restart = vring_restart,
+	.enable = vring_enable,
 	.shutdown = vring_shutdown,
 };
 
@@ -299,9 +299,8 @@ struct virtqueue *vring_new_virtqueue(un
 	vq->in_use = false;
 #endif
 
-	/* No callback?  Tell other side not to bother us. */
-	if (!callback)
-		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
+	/* disable interrupts until we enable them */
+	vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
 
 	/* Put everything in free lists. */
 	vq->num_free = num;
Index: kvm/include/linux/virtio.h
===================================================================
--- kvm.orig/include/linux/virtio.h
+++ kvm/include/linux/virtio.h
@@ -41,7 +41,7 @@ struct virtqueue
  *	vq: the struct virtqueue we're talking about.
  *	len: the length written into the buffer
  *	Returns NULL or the "data" token handed to add_buf.
- * @restart: restart callbacks after callback returned false.
+ * @enable: restart callbacks after callback returned false.
  *	vq: the struct virtqueue we're talking about.
  *	This returns "false" (and doesn't re-enable) if there are pending
  *	buffers in the queue, to avoid a race.
@@ -65,7 +65,7 @@ struct virtqueue_ops {
 
 	void *(*get_buf)(struct virtqueue *vq, unsigned int *len);
 
-	bool (*restart)(struct virtqueue *vq);
+	bool (*enable)(struct virtqueue *vq);
 
 	void (*shutdown)(struct virtqueue *vq);
 };
Index: kvm/drivers/net/virtio_net.c
===================================================================
--- kvm.orig/drivers/net/virtio_net.c
+++ kvm/drivers/net/virtio_net.c
@@ -201,7 +201,7 @@ again:
 	/* Out of packets? */
 	if (received < budget) {
 		netif_rx_complete(vi->dev, napi);
-		if (unlikely(!vi->rvq->vq_ops->restart(vi->rvq))
+		if (unlikely(!vi->rvq->vq_ops->enable(vi->rvq))
 		    && netif_rx_reschedule(vi->dev, napi))
 			goto again;
 	}
@@ -292,6 +292,9 @@ static int virtnet_open(struct net_devic
 		return -ENOMEM;
 
 	napi_enable(&vi->napi);
+
+	vi->rvq->vq_ops->enable(vi->rvq);
+	vi->svq->vq_ops->enable(vi->svq);
 	return 0;
 }
 
Index: kvm/drivers/block/virtio_blk.c
===================================================================
--- kvm.orig/drivers/block/virtio_blk.c
+++ kvm/drivers/block/virtio_blk.c
@@ -183,7 +183,8 @@ static int virtblk_probe(struct virtio_d
 		err = PTR_ERR(vblk->vq);
 		goto out_free_vblk;
 	}
-
+	/* enable interrupts */
+	vblk->vq->vq_ops->enable(vblk->vq);
 	vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
 	if (!vblk->pool) {
 		err = -ENOMEM;

Re: [kvm-devel] [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Dor Laor]

Christian Borntraeger wrote:
>
> Am Mittwoch, 12. Dezember 2007 schrieb Rusty Russell:
> > On Wednesday 12 December 2007 00:16:12 Christian Borntraeger wrote:
> > > That would also work. We already have VRING_AVAIL_F_NO_INTERRUPT in
> > > virtio_ring.c - maybe we can use that. Its hidden in callback and
> > > restart handling, what about adding an explicit startup?
> >
> > Yes, I debated whether to make this a separate hook or not; the current
> > method  reduces the number of function calls without having two ways of
> > disabling  callbacks.
> >
> > In this case, simply starting devices with callbacks disabled and
> > renaming 'restart' to 'enable' (or something) and calling it at the
> > beginning is probably sufficient?
>
> So you suggest something like the following patch? It seems to work but
> there is still a theoretical race at startup. Therefore, I tend to agree
> with Dor that a separate hook seems prefereable, so I am not fully sure if
> the patch is the final solution:
>
I think the change below handles the race. Otherwise please detail the 
use case.
>
> ps: Its ok to answer that after your vacation.
>
> ---
>  drivers/block/virtio_blk.c   |    3 ++-
>  drivers/net/virtio_net.c     |    5 ++++-
>  drivers/virtio/virtio_ring.c |    9 ++++-----
>  include/linux/virtio.h       |    4 ++--
>  4 files changed, 12 insertions(+), 9 deletions(-)
>
> Index: kvm/drivers/virtio/virtio_ring.c
> ===================================================================
> --- kvm.orig/drivers/virtio/virtio_ring.c
> +++ kvm/drivers/virtio/virtio_ring.c
> @@ -220,7 +220,7 @@ static void *vring_get_buf(struct virtqu
>         return ret;
>  }
>
> -static bool vring_restart(struct virtqueue *_vq)
> +static bool vring_enable(struct virtqueue *_vq)
>  {
>         struct vring_virtqueue *vq = to_vvq(_vq);
>
> @@ -264,7 +264,7 @@ static struct virtqueue_ops vring_vq_ops
>         .add_buf = vring_add_buf,
>         .get_buf = vring_get_buf,
>         .kick = vring_kick,
> -       .restart = vring_restart,
> +       .enable = vring_enable,
>         .shutdown = vring_shutdown,
>  };
>
> @@ -299,9 +299,8 @@ struct virtqueue *vring_new_virtqueue(un
>         vq->in_use = false;
>  #endif
>
> -       /* No callback?  Tell other side not to bother us. */
> -       if (!callback)
> -               vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
> +       /* disable interrupts until we enable them */
> +       vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
>
>         /* Put everything in free lists. */
>         vq->num_free = num;
> Index: kvm/include/linux/virtio.h
> ===================================================================
> --- kvm.orig/include/linux/virtio.h
> +++ kvm/include/linux/virtio.h
> @@ -41,7 +41,7 @@ struct virtqueue
>   *     vq: the struct virtqueue we're talking about.
>   *     len: the length written into the buffer
>   *     Returns NULL or the "data" token handed to add_buf.
> - * @restart: restart callbacks after callback returned false.
> + * @enable: restart callbacks after callback returned false.
>   *     vq: the struct virtqueue we're talking about.
>   *     This returns "false" (and doesn't re-enable) if there are pending
>   *     buffers in the queue, to avoid a race.
> @@ -65,7 +65,7 @@ struct virtqueue_ops {
>
>         void *(*get_buf)(struct virtqueue *vq, unsigned int *len);
>
> -       bool (*restart)(struct virtqueue *vq);
> +       bool (*enable)(struct virtqueue *vq);
>
>         void (*shutdown)(struct virtqueue *vq);
>  };
> Index: kvm/drivers/net/virtio_net.c
> ===================================================================
> --- kvm.orig/drivers/net/virtio_net.c
> +++ kvm/drivers/net/virtio_net.c
> @@ -201,7 +201,7 @@ again:
>         /* Out of packets? */
>         if (received < budget) {
>                 netif_rx_complete(vi->dev, napi);
> -               if (unlikely(!vi->rvq->vq_ops->restart(vi->rvq))
> +               if (unlikely(!vi->rvq->vq_ops->enable(vi->rvq))
>                     && netif_rx_reschedule(vi->dev, napi))
>                         goto again;
>         }
> @@ -292,6 +292,9 @@ static int virtnet_open(struct net_devic
>                 return -ENOMEM;
>
>         napi_enable(&vi->napi);
> +
> +       vi->rvq->vq_ops->enable(vi->rvq);
> +       vi->svq->vq_ops->enable(vi->svq);
>
If you change it to:
if (!vi->rvq->vq_ops->enable(vi->rvq))
    vi->rvq->vq_ops->kick(vi->rvq);
if (!vi->rvq->vq_ops->enable(vi->svq))
    vi->rvq->vq_ops->kick(vi->svq);

You solve the race of packets already waiting in the queue without 
triggering the irq.
The same for the block device.
Regards,
Dor.
>
>         return 0;
>  }
>
> Index: kvm/drivers/block/virtio_blk.c
> ===================================================================
> --- kvm.orig/drivers/block/virtio_blk.c
> +++ kvm/drivers/block/virtio_blk.c
> @@ -183,7 +183,8 @@ static int virtblk_probe(struct virtio_d
>                 err = PTR_ERR(vblk->vq);
>                 goto out_free_vblk;
>         }
> -
> +       /* enable interrupts */
> +       vblk->vq->vq_ops->enable(vblk->vq);
>         vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct 
> virtblk_req));
>         if (!vblk->pool) {
>                 err = -ENOMEM;
>

Re: [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Christian Borntraeger]

Am Mittwoch, 12. Dezember 2007 schrieb Dor Laor:
> I think the change below handles the race. Otherwise please detail the 
> use case.
[...]
> > @@ -292,6 +292,9 @@ static int virtnet_open(struct net_devic
> >                 return -ENOMEM;
> >
> >         napi_enable(&vi->napi);
> > +
> > +       vi->rvq->vq_ops->enable(vi->rvq);
> > +       vi->svq->vq_ops->enable(vi->svq);
> >
> If you change it to:
> if (!vi->rvq->vq_ops->enable(vi->rvq))
>     vi->rvq->vq_ops->kick(vi->rvq);
> if (!vi->rvq->vq_ops->enable(vi->svq))
>     vi->rvq->vq_ops->kick(vi->svq);
> 
> You solve the race of packets already waiting in the queue without 
> triggering the irq.

Hmm, I dont fully understand your point. I think this will work as long as 
the host has not consumed all inbound buffers. It will also require that 
the host sends an additional packet, no? If no additional packet comes the 
host has no reason to send an interrupt just because it got a notify 
hypercall. kick inside a guest also does not trigger the poll routine. 

It also wont work on the following scenario:
in virtnet open we will allocate buffers and send them to the host using the 
kick callback. The host can now use _all_ buffers for incoming data while 
interrupts are still disabled and the guest is not running.( Lets say the 
host bridge has lots of multicast traffic and the guest gets not scheduled 
for a while). When the guest now continues and enables the interrupts 
nothing happens. Doing a kick does not help, as the host code will bail out 
with "no dma memory for transfer".

Christian


-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Dor Laor]

Christian Borntraeger wrote:
> Am Mittwoch, 12. Dezember 2007 schrieb Dor Laor:
>   
>> I think the change below handles the race. Otherwise please detail the 
>> use case.
>>     
> [...]
>   
>>> @@ -292,6 +292,9 @@ static int virtnet_open(struct net_devic
>>>                 return -ENOMEM;
>>>
>>>         napi_enable(&vi->napi);
>>> +
>>> +       vi->rvq->vq_ops->enable(vi->rvq);
>>> +       vi->svq->vq_ops->enable(vi->svq);
>>>
>>>       
>> If you change it to:
>> if (!vi->rvq->vq_ops->enable(vi->rvq))
>>     vi->rvq->vq_ops->kick(vi->rvq);
>> if (!vi->rvq->vq_ops->enable(vi->svq))
>>     vi->rvq->vq_ops->kick(vi->svq);
>>
>> You solve the race of packets already waiting in the queue without 
>> triggering the irq.
>>     
>
> Hmm, I dont fully understand your point. I think this will work as long as 
> the host has not consumed all inbound buffers. It will also require that 
> the host sends an additional packet, no? If no additional packet comes the 
> host has no reason to send an interrupt just because it got a notify 
> hypercall. kick inside a guest also does not trigger the poll routine. 
>
> It also wont work on the following scenario:
> in virtnet open we will allocate buffers and send them to the host using the 
> kick callback. The host can now use _all_ buffers for incoming data while 
> interrupts are still disabled and the guest is not running.( Lets say the 
> host bridge has lots of multicast traffic and the guest gets not scheduled 
> for a while). When the guest now continues and enables the interrupts 
> nothing happens. Doing a kick does not help, as the host code will bail out 
> with "no dma memory for transfer".
>   
> Christian
>
>   
You're right I got confused somehow.
So in that case setting the driver status field on open in addition to 
your enable will do the trick.
On DRIVER_OPEN the host will trigger an interrupt if the queue is not 
empty..
Thanks,
Dor

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [kvm-devel] [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Christian Borntraeger]

Am Donnerstag, 13. Dezember 2007 schrieb Dor Laor:
> You're right I got confused somehow.
> So in that case setting the driver status field on open in addition to 
> your enable will do the trick.
> On DRIVER_OPEN the host will trigger an interrupt if the queue is not 
> empty..
> Thanks,
> Dor

After looking into some other drivers, I prefer my first patch (moving 
napi_enable) ;-)

There are some drivers like xen-netfront, b44, which call napi_enable before 
the buffers are passed to the hardware. So it seems that moving napi is 
also a valid option.

But maybe I can just wait until Rusty returns from vacation (I will leave 
next week) so everything might be wonderful when I return ;-)

Rusty, if you decide to apply my patch, there is one downside: The debugging 
code in virtio_ring sometimes triggers with a false positive:

try_fill_recv calls vring_kick. Here we do a notify to the host. This might 
cause an immediate interrupt, triggering the poll routine before vring_kick 
can run END_USE. This is no real problem, as we dont touch the vq after 
notify.

Christian, facing 64 guest cpus unconvering all kind of races

Re: [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Rusty Russell]

On Friday 14 December 2007 05:30:31 Christian Borntraeger wrote:
> Rusty, if you decide to apply my patch, there is one downside: The
> debugging code in virtio_ring sometimes triggers with a false positive:
>
> try_fill_recv calls vring_kick. Here we do a notify to the host. This might
> cause an immediate interrupt, triggering the poll routine before vring_kick
> can run END_USE. This is no real problem, as we dont touch the vq after
> notify.

Hmm, how about we just do something like this?  (needs previous enable_cb
patch)

> Christian, facing 64 guest cpus unconvering all kind of races

Heroic is the word which comes to mind... :)

Cheers,
Rusty.
---
virtio: flush buffers on open

Fix bug found by Christian Borntraeger: if the other side fills all
the registered network buffers before we enable NAPI, we will never
get an interrupt.  The simplest fix is to process the input queue once
on open.

Signed-off-by: Rusty Russell <rusty-8n+1lVoiYb80n/F98K4Iww@public.gmane.org>

diff -r 1dd61213039c drivers/net/virtio_net.c
--- a/drivers/net/virtio_net.c	Tue Dec 18 17:34:18 2007 +1100
+++ b/drivers/net/virtio_net.c	Tue Dec 18 17:47:39 2007 +1100
@@ -326,6 +326,13 @@ static int virtnet_open(struct net_devic
 		return -ENOMEM;
 
 	napi_enable(&vi->napi);
+
+	/* If all buffers were filled by other side before we napi_enabled, we
+	 * won't get another interrupt, so process any outstanding packets
+	 * now.  virtnet_poll wants re-enable the queue, so we disable here. */
+	vi->rvq->vq_ops->disable_cb(vi->rvq);
+	netif_rx_schedule(vi->dev, &vi->napi);
+
 	return 0;
 }
 

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: [kvm-devel] [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Christian Borntraeger]

2nd try. I somehow enable html on the last post

Dor Laor wrote:
> Christian Borntraeger wrote:
>>
>> Hello Rusty,
>>
>> while implementing and testing virtio on s390 I found a problem in
>> virtio_net: The current virtio_net driver has a startup race, which
>> prevents any incoming traffic:
>>
>> If try_fill_recv submits buffers to the host system data might be
>> filled in and an interrupt is sent, before napi_enable finishes.
>> In that case the interrupt will kick skb_recv_done which will then
>> call netif_rx_schedule. netif_rx_schedule checks, if NAPI_STATE_SCHED
>> is set - which is not as we did not run napi_enable. No poll routine
>> is scheduled. Furthermore, skb_recv_done returns false, we disables
>> interrupts for this device.
>>
>> One solution is the enable napi before inbound buffer are available.
>>
> But then you might get recv interrupt without a buffer.

If I look at the current implementation (lguest) no interrupt is sent if
there is not buffer available, no?
On the other hand, if the host does send an interrupt when no buffer is
available, this is also no problem. Looking at virtnet_poll, it seems it
can cope with an empty ring. 

> The way other physical NICs doing it is by dis/en/abling interrupt 
> using registers (look at e1000).
> I suggest we can export add_status and use the original code but
> before enabling napi add a call to add_status(dev, 
> VIRTIO_CONFIG_DEV_OPEN).
> The host won't trigger an irq until it sees the above.

That would also work. We already have VRING_AVAIL_F_NO_INTERRUPT in
virtio_ring.c - maybe we can use that. Its hidden in callback and
restart handling, what about adding an explicit startup?
>
> BTW: Rusty is on vacation and that's probably the reason he didn't 
> respond.
> Regards,
> Dor.

Ok, didnt know that. At the moment I can live with my private patch
while we work on a final solution. Meanwhile I will try to debug virtio
on SMP guests - I still see some strange races on our test system. (But
block and net is now working on s390 and can cope with medium load. )

Christian

Re: [kvm-devel] [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Christian Borntraeger]

Am Dienstag, 11. Dezember 2007 schrieb Christian Borntraeger:
> > The way other physical NICs doing it is by dis/en/abling interrupt 
> > using registers (look at e1000).
> > I suggest we can export add_status and use the original code but
> > before enabling napi add a call to add_status(dev, 
> > VIRTIO_CONFIG_DEV_OPEN).
> > The host won't trigger an irq until it sees the above.
> 
> That would also work. We already have VRING_AVAIL_F_NO_INTERRUPT in
> virtio_ring.c - maybe we can use that. Its hidden in callback and
> restart handling, what about adding an explicit startup?

Ok, just to give an example what I thought about:
---
 drivers/block/virtio_blk.c   |    3 ++-
 drivers/net/virtio_net.c     |    2 ++
 drivers/virtio/virtio_ring.c |   16 +++++++++++++---
 include/linux/virtio.h       |    5 +++++
 4 files changed, 22 insertions(+), 4 deletions(-)

Index: kvm/drivers/virtio/virtio_ring.c
===================================================================
--- kvm.orig/drivers/virtio/virtio_ring.c
+++ kvm/drivers/virtio/virtio_ring.c
@@ -241,6 +241,16 @@ static bool vring_restart(struct virtque
 	return true;
 }
 
+static void vring_startup(struct virtqueue *_vq)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	START_USE(vq);
+	if (vq->vq.callback)
+		vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
+	END_USE(vq);
+}
+
+
 irqreturn_t vring_interrupt(int irq, void *_vq)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
@@ -265,6 +275,7 @@ static struct virtqueue_ops vring_vq_ops
 	.get_buf = vring_get_buf,
 	.kick = vring_kick,
 	.restart = vring_restart,
+	.startup = vring_startup,
 	.shutdown = vring_shutdown,
 };
 
@@ -299,9 +310,8 @@ struct virtqueue *vring_new_virtqueue(un
 	vq->in_use = false;
 #endif
 
-	/* No callback?  Tell other side not to bother us. */
-	if (!callback)
-		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
+	/* disable interrupts until we enable them */
+	vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
 
 	/* Put everything in free lists. */
 	vq->num_free = num;
Index: kvm/include/linux/virtio.h
===================================================================
--- kvm.orig/include/linux/virtio.h
+++ kvm/include/linux/virtio.h
@@ -45,6 +45,9 @@ struct virtqueue
  *	vq: the struct virtqueue we're talking about.
  *	This returns "false" (and doesn't re-enable) if there are pending
  *	buffers in the queue, to avoid a race.
+ * @startup: enable callbacks
+ *	vq: the struct virtqueue we're talking abount
+ *	Returns 0 or an error
  * @shutdown: "unadd" all buffers.
  *	vq: the struct virtqueue we're talking about.
  *	Remove everything from the queue.
@@ -67,6 +70,8 @@ struct virtqueue_ops {
 
 	bool (*restart)(struct virtqueue *vq);
 
+	void (*startup) (struct virtqueue *vq);
+
 	void (*shutdown)(struct virtqueue *vq);
 };
 
Index: kvm/drivers/net/virtio_net.c
===================================================================
--- kvm.orig/drivers/net/virtio_net.c
+++ kvm/drivers/net/virtio_net.c
@@ -292,6 +292,8 @@ static int virtnet_open(struct net_devic
 		return -ENOMEM;
 
 	napi_enable(&vi->napi);
+
+	vi->rvq->vq_ops->startup(vi->rvq);
 	return 0;
 }
 
Index: kvm/drivers/block/virtio_blk.c
===================================================================
--- kvm.orig/drivers/block/virtio_blk.c
+++ kvm/drivers/block/virtio_blk.c
@@ -183,7 +183,8 @@ static int virtblk_probe(struct virtio_d
 		err = PTR_ERR(vblk->vq);
 		goto out_free_vblk;
 	}
-
+	/* enable interrupts */
+	vblk->vq->vq_ops->startup(vblk->vq);
 	vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
 	if (!vblk->pool) {
 		err = -ENOMEM;



There is still one small problem: what if the host fills up all 
host-to-guest buffers before we call startup? So I start to think that your 
solution is better, given that the host is not only not sending interrupts 
but also not filling any buffers as long as VIRTIO_CONFIG_DEV_OPEN is not 
set. I will have a look but I think that add_status needs to be called 
after napi_enable, otherwise we have the same race.

Christian

Re: [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Dor Laor]

Christian Borntraeger wrote:
> Am Dienstag, 11. Dezember 2007 schrieb Christian Borntraeger:
>   
>>> The way other physical NICs doing it is by dis/en/abling interrupt 
>>> using registers (look at e1000).
>>> I suggest we can export add_status and use the original code but
>>> before enabling napi add a call to add_status(dev, 
>>> VIRTIO_CONFIG_DEV_OPEN).
>>> The host won't trigger an irq until it sees the above.
>>>       
>> That would also work. We already have VRING_AVAIL_F_NO_INTERRUPT in
>> virtio_ring.c - maybe we can use that. Its hidden in callback and
>> restart handling, what about adding an explicit startup?
>>     
>
> Ok, just to give an example what I thought about:
> ---
>  drivers/block/virtio_blk.c   |    3 ++-
>  drivers/net/virtio_net.c     |    2 ++
>  drivers/virtio/virtio_ring.c |   16 +++++++++++++---
>  include/linux/virtio.h       |    5 +++++
>  4 files changed, 22 insertions(+), 4 deletions(-)
>
> Index: kvm/drivers/virtio/virtio_ring.c
> ===================================================================
> --- kvm.orig/drivers/virtio/virtio_ring.c
> +++ kvm/drivers/virtio/virtio_ring.c
> @@ -241,6 +241,16 @@ static bool vring_restart(struct virtque
>  	return true;
>  }
>  
> +static void vring_startup(struct virtqueue *_vq)
> +{
> +	struct vring_virtqueue *vq = to_vvq(_vq);
> +	START_USE(vq);
> +	if (vq->vq.callback)
> +		vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
> +	END_USE(vq);
> +}
> +
> +
>  irqreturn_t vring_interrupt(int irq, void *_vq)
>  {
>  	struct vring_virtqueue *vq = to_vvq(_vq);
> @@ -265,6 +275,7 @@ static struct virtqueue_ops vring_vq_ops
>  	.get_buf = vring_get_buf,
>  	.kick = vring_kick,
>  	.restart = vring_restart,
> +	.startup = vring_startup,
>  	.shutdown = vring_shutdown,
>  };
>  
> @@ -299,9 +310,8 @@ struct virtqueue *vring_new_virtqueue(un
>  	vq->in_use = false;
>  #endif
>  
> -	/* No callback?  Tell other side not to bother us. */
> -	if (!callback)
> -		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
> +	/* disable interrupts until we enable them */
> +	vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
>  
>  	/* Put everything in free lists. */
>  	vq->num_free = num;
> Index: kvm/include/linux/virtio.h
> ===================================================================
> --- kvm.orig/include/linux/virtio.h
> +++ kvm/include/linux/virtio.h
> @@ -45,6 +45,9 @@ struct virtqueue
>   *	vq: the struct virtqueue we're talking about.
>   *	This returns "false" (and doesn't re-enable) if there are pending
>   *	buffers in the queue, to avoid a race.
> + * @startup: enable callbacks
> + *	vq: the struct virtqueue we're talking abount
> + *	Returns 0 or an error
>   * @shutdown: "unadd" all buffers.
>   *	vq: the struct virtqueue we're talking about.
>   *	Remove everything from the queue.
> @@ -67,6 +70,8 @@ struct virtqueue_ops {
>  
>  	bool (*restart)(struct virtqueue *vq);
>  
> +	void (*startup) (struct virtqueue *vq);
> +
>  	void (*shutdown)(struct virtqueue *vq);
>  };
>  
> Index: kvm/drivers/net/virtio_net.c
> ===================================================================
> --- kvm.orig/drivers/net/virtio_net.c
> +++ kvm/drivers/net/virtio_net.c
> @@ -292,6 +292,8 @@ static int virtnet_open(struct net_devic
>  		return -ENOMEM;
>  
>  	napi_enable(&vi->napi);
> +
> +	vi->rvq->vq_ops->startup(vi->rvq);
>  	return 0;
>  }
>  
> Index: kvm/drivers/block/virtio_blk.c
> ===================================================================
> --- kvm.orig/drivers/block/virtio_blk.c
> +++ kvm/drivers/block/virtio_blk.c
> @@ -183,7 +183,8 @@ static int virtblk_probe(struct virtio_d
>  		err = PTR_ERR(vblk->vq);
>  		goto out_free_vblk;
>  	}
> -
> +	/* enable interrupts */
> +	vblk->vq->vq_ops->startup(vblk->vq);
>  	vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
>  	if (!vblk->pool) {
>  		err = -ENOMEM;
>
>
>
> There is still one small problem: what if the host fills up all 
> host-to-guest buffers before we call startup? So I start to think that your 
> solution is better, given that the host is not only not sending interrupts 
>   
This is why initially I suggested another status code in order to split 
the ring logic with driver status.
> but also not filling any buffers as long as VIRTIO_CONFIG_DEV_OPEN is not 
> set. I will have a look but I think that add_status needs to be called 
>   
It can fill the buffers even without dev_open, when the dev is finally 
opened the host will issue an interrupt
if there are pending buffers. (I'm not sure it's worth solving, maybe 
just drop them like you suggested).
> after napi_enable, otherwise we have the same race.
>
>   
You're right, my mistake.
> Christian
>
>   


-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://sourceforge.net/services/buy/index.php

Re: [kvm-devel] [PATCH resent] virtio_net: Fix stalled inbound trafficon early packets

[Christian Borntraeger]

Am Mittwoch, 12. Dezember 2007 schrieb Dor Laor:
> This is why initially I suggested another status code in order to split 
> the ring logic with driver status.
> > but also not filling any buffers as long as VIRTIO_CONFIG_DEV_OPEN is 
not 
> > set. I will have a look but I think that add_status needs to be called 
> >   
> It can fill the buffers even without dev_open, when the dev is finally 
> opened the host will issue an interrupt if there are pending buffers. 

There is a problem associated with that scheme. Setting the value in the 
config space does not notify the hypervisor. That means the host will not 
send an interrupt. The interrupt is sent if the host tries to send the next 
packet. If somehow the host manages to use up all buffers before the device 
is finally opened, we have a problem.
> (I'm not sure it's worth solving, maybe  just drop them like you 
suggested).
As said above, dropping seems to me the preferred method. Did I miss 
something?

Christian