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@xxxxxxxxxxxxx> 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.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
