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

2007-12-17 Thread 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 [EMAIL PROTECTED]

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;
 }
 
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2007-12-13 Thread 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
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2007-12-13 Thread 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

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2007-12-12 Thread 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
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2007-12-12 Thread 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;

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2007-12-12 Thread 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;



--
To unsubscribe from this list: send the line

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

2007-12-12 Thread 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

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2007-12-11 Thread 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 [EMAIL PROTECTED]
---
 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
[EMAIL PROTECTED]
https://lists.sourceforge.net/lists/listinfo/kvm-devel





--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2007-12-11 Thread 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


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2007-12-11 Thread 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
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2007-12-11 Thread 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

  


--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2007-12-11 Thread 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.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html