On Tue, Dec 14, 2010 at 01:29:10PM -0800, Mike Waychison wrote: > Representing the internal state within netconsole isn't really a boolean > value, but rather a state machine with transitions. > > This patch introduces 4 states that the netconsole multi-target can > handle. These states are: > - NETPOLL_DISABLED: > The netpoll structure hasn't been setup. > - NETPOLL_SETTINGUP: > The netpoll structure is being setup, and only whoever set this > state can transition out of it. Must come from the NETPOLL_DISABLED > state. > - NETPOLL_ENABLED: > The netpoll structure has been setup and we are good to emit > packets. > - NETPOLL_CLEANING: > Somebody is queued to call netpoll_clean. Whoever does so must > transition out of this state. Must come from the NETPOLL_CLEANING > state. > > The SETTINGUP state specifically targets the window where > netpoll_setup() can take a while (for example, waiting on RTNL). > During this window, we want to exclude console messages from being > emitted to this netpoll target. We also want to exclude any subsequent > user thread that tries to simultaneously enable or disable the target. > > The CLEANING state will be used in a subsequent patch to safely defer > netpoll_cleanup to scheduled work in process context (to fix the > deadlock that will occur whenever NETDEV_UNREGISTER is seen). > > When introducing these new transition states, it no longer makes sense > to 'disable' the target if the interface is going down, only when it is > being removed completely (or deslaved). In fact, prior to this change, > we would leak a reference to the network device if an interface was > brought down, the target removed, and netconsole unloaded. > > Signed-off-by: Mike Waychison <[email protected]> > Acked-by: Matt Mackall <[email protected]> > --- > drivers/net/netconsole.c | 62 > +++++++++++++++++++++++++++++----------------- > 1 files changed, 39 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c > index 6e16888..288a025 100644 > --- a/drivers/net/netconsole.c > +++ b/drivers/net/netconsole.c > @@ -71,16 +71,24 @@ static LIST_HEAD(target_list); > /* This needs to be a spinlock because write_msg() cannot sleep */ > static DEFINE_SPINLOCK(target_list_lock); > > +#define NETPOLL_DISABLED 0 > +#define NETPOLL_SETTINGUP 1 > +#define NETPOLL_ENABLED 2 > +#define NETPOLL_CLEANING 3 > + > /** > * struct netconsole_target - Represents a configured netconsole target. > * @list: Links this target into the target_list. > * @item: Links us into the configfs subsystem hierarchy. > - * @enabled: On / off knob to enable / disable target. > - * Visible from userspace (read-write). > - * We maintain a strict 1:1 correspondence between this and > - * whether the corresponding netpoll is active or inactive. > + * @np_state: Enabled / Disabled / SettingUp / Cleaning > + * Visible from userspace (read-write) as "enabled". > + * We maintain a state machine here of the valid states. Either a > + * target is enabled or disabled, but it may also be in a > + * transitional state whereby nobody is allowed to act on the > + * target other than whoever owns the transition. > + * > * Also, other parameters of a target may be modified at > - * runtime only when it is disabled (enabled == 0). > + * runtime only when it is disabled (np_state == NETPOLL_ENABLED). > * @np: The netpoll structure for this target. > * Contains the other userspace visible parameters: > * dev_name (read-write) > @@ -96,7 +104,7 @@ struct netconsole_target { > #ifdef CONFIG_NETCONSOLE_DYNAMIC > struct config_item item; > #endif > - int enabled; > + int np_state; > struct netpoll np; > }; > > @@ -189,7 +197,7 @@ static struct netconsole_target *alloc_param_target(char > *target_config) > if (err) > goto fail; > > - nt->enabled = 1; > + nt->np_state = NETPOLL_ENABLED; > > return nt; > > @@ -275,7 +283,8 @@ static long strtol10_check_range(const char *cp, long > min, long max) > > static ssize_t show_enabled(struct netconsole_target *nt, char *buf) > { > - return snprintf(buf, PAGE_SIZE, "%d\n", nt->enabled); > + return snprintf(buf, PAGE_SIZE, "%d\n", > + nt->np_state == NETPOLL_ENABLED); > } > > static ssize_t show_dev_name(struct netconsole_target *nt, char *buf) > @@ -337,9 +346,12 @@ static ssize_t store_enabled(struct netconsole_target > *nt, > > if (enabled) { /* 1 */ > spin_lock_irqsave(&target_list_lock, flags); > - if (nt->enabled) > + if (nt->np_state != NETPOLL_DISABLED) > goto busy; > - spin_unlock_irqrestore(&target_list_lock, flags); > + else { > + nt->np_state = NETPOLL_SETTINGUP; > + spin_unlock_irqrestore(&target_list_lock, flags); > + } > > /* > * Skip netpoll_parse_options() -- all the attributes are > @@ -350,9 +362,9 @@ static ssize_t store_enabled(struct netconsole_target *nt, > err = netpoll_setup(&nt->np); > spin_lock_irqsave(&target_list_lock, flags); > if (err) > - nt->enabled = 0; > + nt->np_state = NETPOLL_DISABLED; > else > - nt->enabled = 1; > + nt->np_state = NETPOLL_ENABLED; > spin_unlock_irqrestore(&target_list_lock, flags); > if (err) > return err; > @@ -360,10 +372,17 @@ static ssize_t store_enabled(struct netconsole_target > *nt, > printk(KERN_INFO "netconsole: network logging started\n"); > } else { /* 0 */ > spin_lock_irqsave(&target_list_lock, flags); > - nt->enabled = 0; > + if (nt->np_state == NETPOLL_ENABLED) > + nt->np_state = NETPOLL_CLEANING; > + else if (nt->np_state != NETPOLL_DISABLED) > + goto busy; > spin_unlock_irqrestore(&target_list_lock, flags); > > netpoll_cleanup(&nt->np); > + > + spin_lock_irqsave(&target_list_lock, flags); > + nt->np_state = NETPOLL_DISABLED; > + spin_unlock_irqrestore(&target_list_lock, flags); > } > > return strnlen(buf, count); > @@ -486,7 +505,7 @@ static ssize_t store_locked_##_name(struct > netconsole_target *nt, \ > unsigned long flags; \ > ssize_t ret; \ > spin_lock_irqsave(&target_list_lock, flags); \ > - if (nt->enabled) { \ > + if (nt->np_state != NETPOLL_DISABLED) { \ > printk(KERN_ERR "netconsole: target (%s) is enabled, " \ > "disable to update parameters\n", \ > config_item_name(&nt->item)); \ > @@ -642,7 +661,7 @@ static void drop_netconsole_target(struct config_group > *group, > * The target may have never been enabled, or was manually disabled > * before being removed so netpoll may have already been cleaned up. > */ > - if (nt->enabled) > + if (nt->np_state == NETPOLL_ENABLED) > netpoll_cleanup(&nt->np); > > config_item_put(&nt->item); > @@ -680,16 +699,17 @@ static int netconsole_netdev_event(struct > notifier_block *this, > struct net_device *dev = ptr; > > if (!(event == NETDEV_CHANGENAME || event == NETDEV_UNREGISTER || > - event == NETDEV_BONDING_DESLAVE || event == NETDEV_GOING_DOWN)) > + event == NETDEV_BONDING_DESLAVE)) > goto done; > > spin_lock_irqsave(&target_list_lock, flags); > list_for_each_entry(nt, &target_list, list) { > - if (nt->np.dev == dev) { > + if (nt->np_state == NETPOLL_ENABLED && nt->np.dev == dev) { > switch (event) { > case NETDEV_CHANGENAME: > strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ); > break; > + case NETDEV_BONDING_DESLAVE: > case NETDEV_UNREGISTER: I don't follow what you're doing here. Previously NETDEV_BONDING_DESLAVE events simply caused the netconsole interface to get deactivated, and now we're doing a dev_put on the bonded interface bacause of the above move. Given that NETDEV_BONDING_DESLAVE events are generated when a slave leaves the bond, this doesn't seem right, or am I missing something
Regards Neil -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
