Re: [PATCH v2 04/23] netconsole: Call netpoll_cleanup() in process context

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Nov 09, 2010 at 09:18:57AM -0800, Mike Waychison wrote:
> On Tue, Nov 9, 2010 at 4:07 AM, Neil Horman <nhorman@tuxdriver.com> 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):
> 
That would seem clearer to me yes.  Thanks!
Neil

> 
> 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.
>