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

[Mike Waychison]

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):


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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/