From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zoltan Kiss Subject: Re: [Xen-devel] [PATCH net v2 1/3] xen-netback: move NAPI add/remove calls Date: Mon, 11 Aug 2014 14:14:16 +0100 Message-ID: <53E8C1A8.4050302@citrix.com> References: <1407515833-2550-1-git-send-email-wei.liu2@citrix.com> <1407515833-2550-2-git-send-email-wei.liu2@citrix.com> <53E8B874.8070906@citrix.com> <53E8BBCB.50308@citrix.com> <53E8BE92.5070606@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: Ian Campbell To: David Vrabel , Wei Liu , , Return-path: Received: from smtp.citrix.com ([66.165.176.89]:40666 "EHLO SMTP.CITRIX.COM" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752904AbaHKNOT (ORCPT ); Mon, 11 Aug 2014 09:14:19 -0400 In-Reply-To: <53E8BE92.5070606@citrix.com> Sender: netdev-owner@vger.kernel.org List-ID: On 11/08/14 14:01, David Vrabel wrote: > On 11/08/14 13:49, Zoltan Kiss wrote: >> On 11/08/14 13:35, David Vrabel wrote: >>> On 08/08/14 17:37, Wei Liu wrote: >>>> 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 >>>> Cc: Ian Campbell >>>> Cc: Zoltan Kiss >>>> --- >>>> 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