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

Nicolas de Pesloüan 	[off-list ref] wrote:

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

quoted

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?

	My recollection is that it was done the way it is because there
was no "orig_dev" delivery logic at the time.  A handler registered to a
slave dev would receive no packets at all because assignment of skb->dev
to the master happened first, and the "orig_dev" knowledge was lost.

	When 802.3ad was added, a skb->real_dev field was created, but
it wasn't used for delivery.  802.3ad used real_dev to figure out which
slave a LACPDU arrived on.  The skb->real_dev was eventually replaced
with the orig_dev business that's there now.

	Later, I did the arp_validate stuff the same way as 802.3ad
because it worked and was easier than registering a handler per slave.

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?

	Isn't one purpose of switching to rx_handler that there won't
need to be any skb_bond_should_drop logic in __netif_receive_skb at all?

	Still, if you're just trying to simplify __netif_receive_skb
first, I don't see any reason not to register the packet handlers at the
slave level.  Looking at the ptype_base hash, I don't think that the
protocols bonding is registering (ARP and SLOW) will hash collide with
IP or IPv6, so I suspect there won't be much impact.

	Once an rx_handler is used, then I suspect there's no need for
the packet handlers at all, since the rx_handler is within bonding and
can just deal with the ARP or LACPDU directly.

	-J

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