Re: [RFC patch net-next-2.6] net: convert bonding to use rx_handler

[Jiri Pirko <jpirko@redhat.com>]

Thu, Feb 17, 2011 at 02:20:35PM CET, eric.dumazet@gmail.com wrote:
>Le jeudi 17 février 2011 à 13:52 +0100, Jiri Pirko a écrit :
>> Hello.
>> 
>> This is an attempt to convert bonding to use rx_handler. Result should be
>> cleaner __netif_receive_skb() with much less exceptions needed. I think I
>> covered all aspects, not sure though. I gave this quick smoke test on my
>> testing env. Please comment, test.
>> 
>> Thanks!
>> 
>> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>> ---
>>  drivers/net/bonding/bond_main.c |   75 ++++++++++++++++++++-
>>  include/linux/skbuff.h          |    1 +
>>  net/core/dev.c                  |  144 +++++++++++---------------------------
>>  3 files changed, 117 insertions(+), 103 deletions(-)
>> 
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>
>
>> +
>> +static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
>> +{
>> +	struct net_device *slave_dev;
>> +	struct net_device *bond_dev;
>> +
>> +	skb = skb_share_check(skb, GFP_ATOMIC);
>> +	if (unlikely(!skb))
>> +		return NULL;
>> +	slave_dev = skb->dev;
>> +	bond_dev = slave_dev->master;
>
>I suggest being 10%% safe here :
>
>	bond_dev = ACCESS_ONCE(slave_dev->master);

Right, will change this.

>
>> +	if (unlikely(!bond_dev))
>> +		return skb;
>> +
>> +	if (bond_dev->priv_flags & IFF_MASTER_ARPMON)
>> +		slave_dev->last_rx = jiffies;
>> +
>> +	if (bond_should_deliver_exact_match(skb, slave_dev, bond_dev)) {
>> +		skb->deliver_no_wcard = 1;
>> +		return skb;
>> +	}
>> +
>> +	skb->dev = bond_dev;
>> +
>> +	if (bond_dev->priv_flags & IFF_MASTER_ALB &&
>> +	    bond_dev->priv_flags & IFF_BRIDGE_PORT &&
>> +	    skb->pkt_type == PACKET_HOST) {
>> +		u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
>> +
>> +		memcpy(dest, bond_dev->dev_addr, ETH_ALEN);
>> +	}
>> +
>> +	netif_rx(skb);
>> +	return NULL;
>> +}
>> +
>
>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 31f02d0..15b54ea 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -325,6 +325,7 @@ struct sk_buff {
>>  
>>  	struct sock		*sk;
>>  	struct net_device	*dev;
>> +	struct net_device	*orig_dev;
>>  
>>  	/*
>>  	 * This is the control buffer. It is free to use for every
>
>Thats a problem. lifetime of this field is so small, I wonder if you
>cant find a solution to handle this differently. Maybe a percpu
>variable, or in cb[] ?

Yes, I was not feeling absolutely comfortable puting this here.
You mean global percpu variable living in net/core/dev.c? I must say I
would probably like skb->orig_dev more than that.

As for cb - I do not like that much. Also I think there might be
collision e.g. with bridge code.

>
>
>