Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls

On 11/08/14 14:01, David Vrabel wrote:

On 11/08/14 13:49, Zoltan Kiss wrote:

quoted

On 11/08/14 13:35, David Vrabel wrote:

quoted

On 08/08/14 17:37, Wei Liu wrote:

quoted

Originally napi_add was in init_queue and napi_del was in deinit_queue,
while kthreads were handled in _connect and _disconnect. Move napi_add
and napi_remove to _connect and _disconnect so that they reside togother
with kthread operations.

Signed-off-by: Wei Liu <redacted>
Cc: Ian Campbell <redacted>
Cc: Zoltan Kiss <redacted>
---
   drivers/net/xen-netback/interface.c |   12 ++++++++----
   1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/xen-netback/interface.c

b/drivers/net/xen-netback/interface.c
index 48a55cd..fdb4fca 100644

--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c

@@ -528,9 +528,6 @@ int xenvif_init_queue(struct xenvif_queue *queue)

       init_timer(&queue->rx_stalled);

-    netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
-            XENVIF_NAPI_WEIGHT);
-
       return 0;
   }

@@ -618,6 +615,9 @@ int xenvif_connect(struct xenvif_queue *queue,

unsigned long tx_ring_ref,
       wake_up_process(queue->task);
       wake_up_process(queue->dealloc_task);

+    netif_napi_add(queue->vif->dev, &queue->napi, xenvif_poll,
+            XENVIF_NAPI_WEIGHT);
+
       return 0;

   err_rx_unbind:

@@ -675,6 +675,11 @@ void xenvif_disconnect(struct xenvif *vif)

       for (queue_index = 0; queue_index < num_queues; ++queue_index) {
           queue = &vif->queues[queue_index];
+        netif_napi_del(&queue->napi);
+    }

Why have you added an additional loop over all the queues?  The ordering
looks wrong as well.  I think you want

    1. unbind from irqhandler
    2. napi del
    3. stop task
    4. stop dealloc task
    5. unmap frontend rings.

And that's how they are ordered.

No, it isn't.  Did you mistakenly look at netfront which is correctly
ordered already?

You must unbind the irq handler before calling netif_napi_del() or an
interrupt may occur and the handler may call napi_schedule() with a
deleted instance.

I think xenvif_carrier_off (which call xenvif_down) does that. It is 
right before this new napi_del in xenvif_disconnect.

Zoli