Re: [patch net-next-2.6 2/8] bonding: register slave pointer for rx_handler

["Nicolas de Pesloüan" <nicolas.2p.debian@gmail.com>]

Le 05/03/2011 15:27, Jiri Pirko a écrit :
> Sat, Mar 05, 2011 at 03:06:58PM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 05/03/2011 11:29, Jiri Pirko a écrit :
>>> Register slave pointer as rx_handler data. That would eventually prevent
>>> need to loop over slave devices to find the right slave.
>>>
>>> Use synchronize_net to ensure that bond_handle_frame does not get slave
>>> structure freed when working with that.
>>>
>>> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>>> ---
>>>   drivers/net/bonding/bond_main.c |   17 +++++++++++------
>>>   drivers/net/bonding/bonding.h   |    3 +++
>>>   2 files changed, 14 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 0592e6d..1c19368 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -1495,21 +1495,22 @@ static bool bond_should_deliver_exact_match(struct sk_buff *skb,
>>>
>>>   static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
>>>   {
>>> -	struct net_device *slave_dev;
>>> +	struct slave *slave;
>>>   	struct net_device *bond_dev;
>>>
>>>   	skb = skb_share_check(skb, GFP_ATOMIC);
>>>   	if (unlikely(!skb))
>>>   		return NULL;
>>> -	slave_dev = skb->dev;
>>> -	bond_dev = ACCESS_ONCE(slave_dev->master);
>>> +
>>> +	slave = bond_slave_get_rcu(skb->dev);
>>> +	bond_dev = ACCESS_ONCE(slave->dev->master);
>>>   	if (unlikely(!bond_dev))
>>>   		return skb;
>>>
>>>   	if (bond_dev->priv_flags&   IFF_MASTER_ARPMON)
>>> -		slave_dev->last_rx = jiffies;
>>> +		slave->dev->last_rx = jiffies;
>>>
>>> -	if (bond_should_deliver_exact_match(skb, slave_dev, bond_dev)) {
>>> +	if (bond_should_deliver_exact_match(skb, slave->dev, bond_dev)) {
>>>   		skb->deliver_no_wcard = 1;
>>>   		return skb;
>>>   	}
>>
>> Up to this point, it looks like cleanup, and unrelated to the title and description of the patch.
>
> Nope - these are changes connected to the ability to get slave struct
> via bond_slave_get_rcu. Very related.

Agreed.

>>
>> Anyway, the cleanup looks good to me. Should just be in a separate patch.
>>
>>> @@ -1703,7 +1704,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>>   		pr_debug("Error %d calling netdev_set_bond_master\n", res);
>>>   		goto err_restore_mac;
>>>   	}
>>> -	res = netdev_rx_handler_register(slave_dev, bond_handle_frame, NULL);
>>> +	res = netdev_rx_handler_register(slave_dev, bond_handle_frame,
>>> +					 new_slave);
>>
>> And using rx_handler data for this purpose sounds good to me.
>>
>> Reviewed-by: Nicolas de Pesloüan<nicolas.2p.debian@free.fr>
>>
>>>   	if (res) {
>>>   		pr_debug("Error %d calling netdev_rx_handler_register\n", res);
>>>   		goto err_unset_master;
>>> @@ -1925,6 +1927,7 @@ err_close:
>>>
>>>   err_unreg_rxhandler:
>>>   	netdev_rx_handler_unregister(slave_dev);
>>> +	synchronize_net();
>>>
>>>   err_unset_master:
>>>   	netdev_set_bond_master(slave_dev, NULL);
>>> @@ -2108,6 +2111,7 @@ int bond_release(struct net_device *bond_dev, struct net_device *slave_dev)
>>>   	}
>>>
>>>   	netdev_rx_handler_unregister(slave_dev);
>>> +	synchronize_net();
>>>   	netdev_set_bond_master(slave_dev, NULL);
>>>
>>>   	slave_disable_netpoll(slave);
>>> @@ -2222,6 +2226,7 @@ static int bond_release_all(struct net_device *bond_dev)
>>>   		}
>>>
>>>   		netdev_rx_handler_unregister(slave_dev);
>>> +		synchronize_net();
>>>   		netdev_set_bond_master(slave_dev, NULL);
>>>
>>>   		slave_disable_netpoll(slave);
>>> diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
>>> index ff4e269..1aac5cd 100644
>>> --- a/drivers/net/bonding/bonding.h
>>> +++ b/drivers/net/bonding/bonding.h
>>> @@ -264,6 +264,9 @@ struct bonding {
>>>   #endif /* CONFIG_DEBUG_FS */
>>>   };
>>>
>>> +#define bond_slave_get_rcu(dev) \
>>> +	((struct slave *) rcu_dereference(dev->rx_handler_data))
>>> +
>>>   /**
>>>    * Returns NULL if the net_device does not belong to any of the bond's slaves
>>>    *
>>
>