lkml.org 
[lkml]   [2010]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 03/22] netconsole: Introduce 'enabled' state-machine
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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2010-12-17 22:37    [W:1.077 / U:0.208 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site