From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751128AbdKTLRP (ORCPT ); Mon, 20 Nov 2017 06:17:15 -0500 Received: from mx2.suse.de ([195.135.220.15]:44593 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751068AbdKTLRN (ORCPT ); Mon, 20 Nov 2017 06:17:13 -0500 Subject: Re: [PATCH] xen-netfront: remove warning when unloading module To: Wei Liu , Eduardo Otubo Cc: xen-devel@lists.xenproject.org, netdev@vger.kernel.org, paul.durrant@citrix.com, linux-kernel@vger.kernel.org, vkuznets@redhat.com, cavery@redhat.com, cheshi@redhat.com, mgamal@redhat.com, Boris Ostrovsky References: <20171120104109.11585-1-otubo@redhat.com> <20171120104919.ziq45hikwbzlnvbo@citrix.com> From: Juergen Gross Message-ID: Date: Mon, 20 Nov 2017 12:17:11 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171120104919.ziq45hikwbzlnvbo@citrix.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 20/11/17 11:49, Wei Liu wrote: > CC netfront maintainers. > > On Mon, Nov 20, 2017 at 11:41:09AM +0100, Eduardo Otubo wrote: >> When unloading module xen_netfront from guest, dmesg would output >> warning messages like below: >> >> [ 105.236836] xen:grant_table: WARNING: g.e. 0x903 still in use! >> [ 105.236839] deferring g.e. 0x903 (pfn 0x35805) >> >> This problem relies on netfront and netback being out of sync. By the time >> netfront revokes the g.e.'s netback didn't have enough time to free all of >> them, hence displaying the warnings on dmesg. >> >> The trick here is to make netfront to wait until netback frees all the g.e.'s >> and only then continue to cleanup for the module removal, and this is done by >> manipulating both device states. >> >> Signed-off-by: Eduardo Otubo >> --- >> drivers/net/xen-netfront.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c >> index 8b8689c6d887..b948e2a1ce40 100644 >> --- a/drivers/net/xen-netfront.c >> +++ b/drivers/net/xen-netfront.c >> @@ -2130,6 +2130,17 @@ static int xennet_remove(struct xenbus_device *dev) >> >> dev_dbg(&dev->dev, "%s\n", dev->nodename); >> >> + xenbus_switch_state(dev, XenbusStateClosing); >> + while (xenbus_read_driver_state(dev->otherend) != XenbusStateClosing){ >> + cpu_relax(); >> + schedule(); >> + } >> + xenbus_switch_state(dev, XenbusStateClosed); >> + while (dev->xenbus_state != XenbusStateClosed){ >> + cpu_relax(); >> + schedule(); >> + } I really don't like the busy waits. Can't you use e.g. a wait queue and wait_event_interruptible() instead? BTW: what happens if the device is already in closed state if you enter xennet_remove()? In case this is impossible, please add a comment to indicate you've thought about that case. Other than that: you should run ./scripts/checkpatch.p1 against your patch to avoid common style problems. Juergen