From patchwork Sun Oct 5 04:26:21 2008 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Herbert Xu X-Patchwork-Id: 2730 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.176.167]) by ozlabs.org (Postfix) with ESMTP id CA20EDDD04 for ; Sun, 5 Oct 2008 15:29:26 +1100 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750808AbYJEE00 (ORCPT ); Sun, 5 Oct 2008 00:26:26 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750777AbYJEE00 (ORCPT ); Sun, 5 Oct 2008 00:26:26 -0400 Received: from rhun.apana.org.au ([64.62.148.172]:59042 "EHLO arnor.apana.org.au" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750706AbYJEE0Z (ORCPT ); Sun, 5 Oct 2008 00:26:25 -0400 Received: from gondolin.me.apana.org.au ([192.168.0.6] ident=Debian-exim) by arnor.apana.org.au with esmtp (Exim 4.63 #1 (Debian)) id 1KmLCI-0001rQ-T1; Sun, 05 Oct 2008 15:26:23 +1100 Received: from herbert by gondolin.me.apana.org.au with local (Exim 4.69) (envelope-from ) id 1KmLCH-0003E2-Ch; Sun, 05 Oct 2008 12:26:21 +0800 From: Herbert Xu To: benjamin.thery@bull.net (Benjamin Thery) Subject: Re: [PATCH] net: deadlock during net device unregistration Cc: davem@davemloft.net, netdev@vger.kernel.org, dlezcano@fr.ibm.com, benjamin.thery@bull.net Organization: Core In-Reply-To: <20080929175421.722037051@theryb.frec.bull.fr> X-Newsgroups: apana.lists.os.linux.netdev User-Agent: tin/1.7.4-20040225 ("Benbecula") (UNIX) (Linux/2.6.17-rc4 (i686)) Message-Id: Date: Sun, 05 Oct 2008 12:26:21 +0800 Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Benjamin Thery wrote: > > 1. Unregister a device, the following routines are called: > > -> unregister_netdev > -> rtnl_lock > -> unregister_netdevice > -> rtnl_unlock > -> netdev_run_todo > -> netdev_wait_allrefs OK, this explains lots of dead-locks that people have been seeing. But I think we can go a step further: net: Fix netdev_run_todo dead-lock Benjamin Thery tracked down a bug that explains many instances of the error unregister_netdevice: waiting for %s to become free. Usage count = %d It turns out that netdev_run_todo can dead-lock with itself if a second instance of it is run in a thread that will then free a reference to the device waited on by the first instance. The problem is really quite silly. We were trying to create parallelism where none was required. As netdev_run_todo always follows a RTNL section, and that todo tasks can only be added with the RTNL held, by definition you should only need to wait for the very ones that you've added and be done with it. There is no need for a second mutex or spinlock. This is exactly what the following patch does. Signed-off-by: Herbert Xu Cheers, diff --git a/net/core/dev.c b/net/core/dev.c index e8eb2b4..021f531 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3808,14 +3808,11 @@ static int dev_new_index(struct net *net) } /* Delayed registration/unregisteration */ -static DEFINE_SPINLOCK(net_todo_list_lock); static LIST_HEAD(net_todo_list); static void net_set_todo(struct net_device *dev) { - spin_lock(&net_todo_list_lock); list_add_tail(&dev->todo_list, &net_todo_list); - spin_unlock(&net_todo_list_lock); } static void rollback_registered(struct net_device *dev) @@ -4142,33 +4139,24 @@ static void netdev_wait_allrefs(struct net_device *dev) * free_netdev(y1); * free_netdev(y2); * - * We are invoked by rtnl_unlock() after it drops the semaphore. + * We are invoked by rtnl_unlock(). * This allows us to deal with problems: * 1) We can delete sysfs objects which invoke hotplug * without deadlocking with linkwatch via keventd. * 2) Since we run with the RTNL semaphore not held, we can sleep * safely in order to wait for the netdev refcnt to drop to zero. + * + * We must not return until all unregister events added during + * the interval the lock was held have been completed. */ -static DEFINE_MUTEX(net_todo_run_mutex); void netdev_run_todo(void) { struct list_head list; - /* Need to guard against multiple cpu's getting out of order. */ - mutex_lock(&net_todo_run_mutex); - - /* Not safe to do outside the semaphore. We must not return - * until all unregister events invoked by the local processor - * have been completed (either by this todo run, or one on - * another cpu). - */ - if (list_empty(&net_todo_list)) - goto out; - /* Snapshot list, allow later requests */ - spin_lock(&net_todo_list_lock); list_replace_init(&net_todo_list, &list); - spin_unlock(&net_todo_list_lock); + + __rtnl_unlock(); while (!list_empty(&list)) { struct net_device *dev @@ -4200,9 +4188,6 @@ void netdev_run_todo(void) /* Free network device */ kobject_put(&dev->dev.kobj); } - -out: - mutex_unlock(&net_todo_run_mutex); } static struct net_device_stats *internal_stats(struct net_device *dev) diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 71edb8b..d6381c2 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -73,7 +73,7 @@ void __rtnl_unlock(void) void rtnl_unlock(void) { - mutex_unlock(&rtnl_mutex); + /* This fellow will unlock it for us. */ netdev_run_todo(); }