Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753737Ab0KIRTW (ORCPT ); Tue, 9 Nov 2010 12:19:22 -0500 Received: from smtp-out.google.com ([74.125.121.35]:30817 "EHLO smtp-out.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752402Ab0KIRTV convert rfc822-to-8bit (ORCPT ); Tue, 9 Nov 2010 12:19:21 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=google.com; s=beta; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-type:content-transfer-encoding; b=nCtTMvWTjJ916aDufpxNDCkLGiMW0Q46LrNuDoyexGig+dQ6+Sg3b08pi0Ow5cbGil hWkk2O6Sk3Q6XRRdXz2w== MIME-Version: 1.0 In-Reply-To: <20101109120752.GA18269@hmsreliant.think-freely.org> References: <20101108203120.22479.19708.stgit@crlf.mtv.corp.google.com> <20101108203159.22479.48774.stgit@crlf.mtv.corp.google.com> <20101109120752.GA18269@hmsreliant.think-freely.org> From: Mike Waychison Date: Tue, 9 Nov 2010 09:18:57 -0800 Message-ID: Subject: Re: [PATCH v2 04/23] netconsole: Call netpoll_cleanup() in process context To: Neil Horman Cc: simon.kagstrom@netinsight.net, davem@davemloft.net, Matt Mackall , adurbin@google.com, linux-kernel@vger.kernel.org, chavey@google.com, Greg KH , =?ISO-8859-1?Q?Am=E9rico_Wang?= , akpm@linux-foundation.org, linux-api@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT X-System-Of-Record: true Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3121 Lines: 77 On Tue, Nov 9, 2010 at 4:07 AM, Neil Horman wrote: > On Mon, Nov 08, 2010 at 12:32:00PM -0800, Mike Waychison wrote: >> +static void deferred_netpoll_cleanup(struct work_struct *work) >> +{ >> + ? ? struct netconsole_target *nt; >> + ? ? unsigned long flags; >> + >> + ? ? nt = container_of(work, struct netconsole_target, cleanup_work); >> + ? ? netpoll_cleanup(&nt->np); >> + >> + ? ? spin_lock_irqsave(&target_list_lock, flags); >> + ? ? BUG_ON(nt->np_state != NETPOLL_CLEANING); >> + ? ? nt->np_state = NETPOLL_DISABLED; >> + ? ? spin_unlock_irqrestore(&target_list_lock, flags); >> + >> + ? ? netconsole_target_put(nt); >> +} >> + > Where is the synchronization on the new work queue when the module is getting > removed? The target get/put code does nothing to the module refcount, and > cleanup_netconsole just deletes targets, it doesn't block or fail on netconsole > refcounts, so you could run this work after the module has been removed and oops > the system. > The synchronization here is actually handled in free_param_target() with the call to cancel_work_sync(). After that call is made in the exit path, we know a task cannot be rescheduled for cleanup, so we can clean things up directly. The comment/name of free_param_target should probably change. I used to have this cancel_work_sync() call elsewhere, and folded both the dynamic and param-based target cleanup into one as they ended up being the same. Removal of the item from configfs however isn't handled. How about something like (no idea if this will get whitespace damage): diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 02ba5c4..ddd5e4f 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -686,13 +686,16 @@ static void drop_netconsole_target(struct config_group *group, spin_unlock_irqrestore(&target_list_lock, flags); /* - * The target may have never been enabled, or was manually disabled - * before being removed so netpoll may have already been cleaned up. + * The target may have never been disabled, or was disabled due + * to a netdev event, but we haven't had the chance to clean + * things up yet. * - * If it queued for cleanup already, that is fine, as that path holds a - * reference to the config_item. + * We can't wait for the target to be cleaned up by its + * scheduled work however, as that work doesn't pin this module + * in place. */ - if (nt->np_state == NETPOLL_ENABLED) + cancel_work_sync(&nt->cleanup_work); + if (nt->np_state == NETPOLL_ENABLED || nt->np-state == NETPOLL_CLEANING) netpoll_cleanup(&nt->np); config_item_put(&nt->item); I can fold this diff snippet into this patch for the next round if you agree it plugs the remaining hole. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/