Re: [Bonding-devel] [v3 Patch 2/3] bridge: make bridge support netpoll

[Jay Vosburgh <fubar@us.ibm.com>]

Cong Wang <amwang@redhat.com> wrote:

>Stephen Hemminger wrote:
>> On Mon, 12 Apr 2010 12:38:57 +0200
>> Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> 
>>> Le lundi 12 avril 2010 à 18:37 +0800, Cong Wang a écrit :
>>>> Stephen Hemminger wrote:
>>>>> There is no protection on dev->priv_flags for SMP access.
>>>>> It would better bit value in dev->state if you are using it as control flag.
>>>>>
>>>>> Then you could use 
>>>>> 			if (unlikely(test_and_clear_bit(__IN_NETPOLL, &skb->dev->state)))
>>>>> 				netpoll_send_skb(...)
>>>>>
>>>>>
>>>> Hmm, I think we can't use ->state here, it is not for this kind of purpose,
>>>> according to its comments.
>>>>
>>>> Also, I find other usages of IFF_XXX flags of ->priv_flags are also using
>>>> &, | to set or clear the flags. So there must be some other things preventing
>>>> the race...
>>> Yes, its RTNL that protects priv_flags changes, hopefully...
>>>
>> 
>> The patch was not protecting priv_flags with RTNL.
>> For example..
>> 
>> 
>> @@ -308,7 +312,9 @@ static void netpoll_send_skb(struct netp
>>  		     tries > 0; --tries) {
>>  			if (__netif_tx_trylock(txq)) {
>>  				if (!netif_tx_queue_stopped(txq)) {
>> +					dev->priv_flags |= IFF_IN_NETPOLL;
>>  					status = ops->ndo_start_xmit(skb, dev);
>> +					dev->priv_flags &= ~IFF_IN_NETPOLL;
>>  					if (status == NETDEV_TX_OK)
>>  						txq_trans_update(txq);
>
>Hmm, but I checked the bonding case (IFF_BONDING), it doesn't
>hold rtnl_lock. Strange.

	I looked, and there are a couple of cases in bonding that don't
have RTNL for adjusting priv_flags (in bond_ab_arp_probe when no slaves
are up, and a couple of cases in 802.3ad).  I think the solution there
is to move bonding away from priv_flags for some of this (e.g., convert
bonding to use a frame hook like bridge and macvlan, and greatly
simplify skb_bond_should_drop), but that's a separate topic.

	The majority of the cases, however, do hold RTNL.  Bonding
generally doesn't have to acquire RTNL itself, since whatever called
into bonding is holding it already.  For example, the slave add and
remove paths (bond_enslave, bond_release) are called either via sysfs or
ioctl, both of which acquire RTNL.  All of the set and clear operations
for IFF_BONDING fall into this category; look at bonding_store_slaves
for an example.

	Bonding does acquire RTNL itself when performing failovers,
e.g., bond_mii_monitor holds RTNL prior to calling bond_miimon_commit,
which will change priv_flags.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com