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

Le 22/02/2011 00:20, Nicolas de Pesloüan a écrit :

After checking every protocol handlers installed by dev_add_pack(), it
appears that only 4 of them really use the orig_dev parameter given by
__netif_receive_skb():

- bond_3ad_lacpdu_recv() @ drivers/net/bonding/bond_3ad.c
- bond_arp_recv() @ drivers/net/bonding/bond_main.c
- packet_rcv() @ net/packet/af_packet.c
- tpacket_rcv() @ net/packet/af_packet.c

 From the bonding point of view, the meaning of orig_dev is obviously
"the device one layer below the bonding device, through which the packet
reached the bonding device". It is used by bond_3ad_lacpdu_recv() and
bond_arp_recv(), to find the underlying slave device through which the
LACPDU or ARP was received. (The protocol handler is registered at the
bonding device level).

 From the af_packet point of view, the meaning is documented (in commit
"[AF_PACKET]: Add option to return orig_dev to userspace") as the
"physical device [that] actually received the traffic, instead of having
the encapsulating device hide that information."

When the bonding device is just one level above the physical device, the
two meanings happen to match the same device, by chance.

So, currently, a bonding device cannot stack properly on top of anything
but physical devices. It might not be a problem today, but may change in
the future...

Hi Jay,

Still thinking about this orig_dev stuff, I wonder why the protocol handlers used in bonding 
(bond_3ad_lacpdu_recv() and bond_arp_rcv()) are registered at the master level instead of at the 
slave level ?

If they were registered at the slave level, they would simply receive skb->dev as the ingress 
interface and use this value instead of needing the orig_dev value given to them when they are 
registered at the master level.

As orig_dev is only used by bonding and by af_packet, but they disagree on the exact meaning of 
orig_dev, one way to fix this discrepancy would be to remove one of the usage. As the af_packet 
usage is exposed to user space, bonding seems the right place to stop using orig_dev, even if 
orig_dev was introduced for bonding :-)

I understand that this would add one entry per slave device to the ptype_base list, but this seems 
to be the only bad effect of registering at the slave level. Can you confirm that this was the 
reason to register at the master level instead?

If you think registering at the slave level would cause too much impact on ptype_base, then we might 
have another way to stop using orig_dev for bonding:

In __skb_bond_should_drop(), we already test for the two interesting protocols:

if ((dev->priv_flags & IFF_SLAVE_NEEDARP) && skb->protocol == __cpu_to_be16(ETH_P_ARP))
	return 0;

if (master->priv_flags & IFF_MASTER_8023AD && skb->protocol == __cpu_to_be16(ETH_P_SLOW))
	return 0;

Would it be possible to call the right handlers directly from inside __skb_bond_should_drop() then 
let __skb_bond_should_drop() return 1 ("should drop") after processing the frames that are only of 
interest for bonding?

	Nicolas.