[v3,03/22] netconsole: Introduce 'enabled' state-machine

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 <mikew@google.com>
Acked-by: Matt Mackall <mpm@selenic.com>
---
 drivers/net/netconsole.c |   62 +++++++++++++++++++++++++++++-----------------
 1 files changed, 39 insertions(+), 23 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

2010/12/14 Mike Waychison <mikew@google.com>:
> Representing the internal state within netconsole isn't really a boolean
> value, but rather a state machine with transitions.
[...]
> 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
[...]
> @@ -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;
[...]

Since the spinlock protects only nt->np_state setting, you might be
able to remove it altogether and use cmpxchg() where nt->np_state
transitions from enabled or disabled state.

Maybe the locking scheme just needs more thought altogether?

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

2010/12/14 Michał Mirosław <mirqus@gmail.com>:
> 2010/12/14 Mike Waychison <mikew@google.com>:
>> Representing the internal state within netconsole isn't really a boolean
>> value, but rather a state machine with transitions.
> [...]
>> 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
> [...]
>> @@ -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;
> [...]
>
> Since the spinlock protects only nt->np_state setting, you might be
> able to remove it altogether and use cmpxchg() where nt->np_state
> transitions from enabled or disabled state.
>
> Maybe the locking scheme just needs more thought altogether?

The target_list_lock protects the list, as well as the state
transitions.  This makes iterating through the list and getting a
consistent view of the targets a lot easier when it comes time to
transmitting packets we are guaranteed that nobody is changing the
target state underneath us if nt->np_state == NETPOLL_ENABLED while we
hold the lock.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 <mikew@google.com>
> Acked-by: Matt Mackall <mpm@selenic.com>
> ---
>  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 netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Dec 17, 2010 at 12:52 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Tue, Dec 14, 2010 at 01:29:10PM -0800, Mike Waychison wrote:

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

Ya, I did a poor job of explaining this bit.  Let me try now:

- NETDEV_GOING_DOWN is unneccesary because we will always see a
NETDEV_UNREGISTER event, which is why it is removed.
- I wasn't sure how to handle the NETDEV_BONDING_DESLAVE event in a
consistent way.  Originally we would disable the target, but this
actually leaks nt->np as it will never get cleaned up properly.  To
the user, it looks as if the target gets disabled.

I don't think you have any contention with the first comment above
(though I need to document it).

You are right in that with this patch, the target is left enabled,
when we should probably mark it as disabled.  I'd be happy to add the
transition from NETPOLL_ENABLED -> NETPOLL_DISABLED here if you'd
like, but it'd move again in the next patch anyway (which already
re-introduces the state transition in defer_netpoll_cleanup()).  Sorry
this is confusing :(   I'll make it clearer in the next series
version.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html