Date:	Sat, 8 Feb 2014 09:43:47 +0800
From:	Ding Tianhong <dingtianhong@...wei.com>
To:	Jay Vosburgh <fubar@...ibm.com>,
	"sfeldma@...ulusnetworks.com" <sfeldma@...ulusnetworks.com>
CC:	Cong Wang <cwang@...pensource.com>,
	Thomas Glanzmann <thomas@...nzmann.de>,
	Eric Dumazet <eric.dumazet@...il.com>,
	"Veaceslav Falico" <vfalico@...hat.com>, <andy@...yhouse.net>,
	Jiří Pírko <jiri@...nulli.us>,
	netdev <netdev@...r.kernel.org>
Subject: Re: RTNL: assertion failed at net/core/dev.c (4494) and RTNL: assertion
 failed at net/core/rtnetlink.c (940)

On 2014/2/8 9:21, Jay Vosburgh wrote:
> Jay Vosburgh <fubar@...ibm.com> wrote:
> 
>>
>> Cong Wang <cwang@...pensource.com> wrote:
>>
>>> On Thu, Feb 6, 2014 at 2:07 PM, Jay Vosburgh <fubar@...ibm.com> wrote:
>>>> Jay Vosburgh <fubar@...ibm.com> wrote:
>>>>
>>>>> Cong Wang <cwang@...pensource.com> wrote:
>>>>>
>>>>>
>>>>>       That would eliminate the warning, but is suboptimal.  Acquiring
>>>>> RTNL is not necessary on the vast majority of state machine runs
>>>>> (because no state changes take place, i.e., no ports are disabled or
>>>>> enabled).  The above change would add 10 round trips per second to RTNL,
>>>>> which seems excessive.
>>>>>
>>>>>       Also, we cannot unconditionally acquire RTNL in this function,
>>>>> as it would race with the call to cancel_delayed_work_sync from
>>>>> bond_close (via bond_work_cancel_all).
>>>
>>> OK.
>>>
>>>>
>>>>         Thought of one more problem: we can't hold a regular lock while
>>>> calling rtmsg_ifinfo, as it may sleep in alloc_skb.  The rtmsg_ifinfo
>>>> call has to be RTNL and nothing else.
>>>>
>>>
>>> s/GFP_KERNEL/GFP_ATOMIC/
>>
>> 	Yah, that would help with extra locks, but not totally solve
>> things.  I'm looking around, and seeing a number of other places that
>> will end up at one of these rtmsg_ifinfo calls with incorrect locking:
>>
>> 	bond_ab_arp_probe calls via bond_set_slave_active_flags and
>> bond_set_slave_inactive_flags without RTNL.
>>
>> 	bond_change_active_slave calls via bond_set_slave_inactive_flags
>> and bond_set_slave_active_flags with other locks held, and maybe without
>> RTNL; I'm not sure if bond_option_active_slave_set holds RTNL when it
>> calls bond_select_active_slave.
>>
>> 	bond_open calls via bond_set_slave_active_flags and
>> bond_set_slave_inactive_flags with RTNL, but also with other locks held.
>>
>> 	bond_loadbalance_arp_mon calls bond_set_active_slave and
>> bond_set_backup_slave without RTNL.
>>
>> 	This is in addition to the cases in the 802.3ad code from
>> __enable_port and __disable_port calls.
> 
> 	Just an update in case anybody else is looking into this, and
> some questions for Scott.
> 
> 	Acquiring RTNL for the __enable_port and __disable_port cases is
> difficult, as those calls generally already hold the state machine lock,
> and cannot unconditionally call rtnl_lock because either they already
> hold RTNL (for calls via bond_3ad_unbind_slave) or due to the potential
> for deadlock with bond_3ad_adapter_speed_changed,
> bond_3ad_adapter_duplex_changed, bond_3ad_link_change, or
> bond_3ad_update_lacp_rate.  All four of those are called with RTNL held,
> and acquire the state machine lock second.  The calling contexts for
> __enable_port and __disable_port already hold the state machine lock,
> and may or may not need RTNL.

Agree, it is hard to add RTNL here, deadlock is easily happened.

> 
> 	Scott: you added these calls, so can you explain what they're
> for?  I'm asking for two reasons:
> 
> 	First, if they do not occur synchronously is it going to be a
> problem?  E.g., for the 802.3ad case, if the rtmsg_ifinfo is called
> either at the end of the state machine run, or for non-state machine
> events, at the next run of the state machine (which is every 100 ms),
> would that be a problem?  Setting a flag in the slave somewhere that an
> rtmsg_ifinfo is needed should be doable for the 802.3ad case.
> 
> 	Second, what do the messages mean?  That the slave is now
> "active and usable"?  I'm asking because I suspect the bond_ab_arp_probe
> usage wherein it adjusts the flags and curr_active_slave should not
> actually call rtmsg_ifinfo, as the slave there is not really "up."
> What's going on there is that the ARP monitor cycles through each slave
> one by one, and tests to see if that slave works.  If it does work, then
> it is set as the active elsewhere in the monitor code.  This function
> adjusts the flags so that the ARP monitor will treat the "testing" slave
> as "active" for purposes of determining whether or not it is up.  I
> suspect this adjustment to the flags should not actually generate an
> rtmsg_ifinfo.
> 
> 	I think the remaining cases can be dealt with, but clarification
> on the above two questions would be very helpful.
> 
> 	-J
> 

commit 6fde8f037e604e05df1529 fix the problem for bond_loadbalance_arp_mon(),
and commit 66dd1c077a3f3c130d1 fix the problem for bond_activebackup_arp_mon(),
but we still miss the 3ad monitor, I think if the slave should send the message
by netlink, it is better to refer to fdb_notify() for bridge,I doubts that why we need to send so many
message, just slave info is enough, then RTNL is not needed here.

Ding


> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.com
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> .
> 


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

Powered by blists - more mailing lists