Message-ID: <27369.1298749377@death>
Date:	Sat, 26 Feb 2011 11:42:57 -0800
From:	Jay Vosburgh <fubar@...ibm.com>
To:	=?ISO-8859-1?Q?Nicolas_de_Peslo=FCan?= 
	<nicolas.2p.debian@...il.com>
cc:	Jiri Pirko <jpirko@...hat.com>, David Miller <davem@...emloft.net>,
	kaber@...sh.net, eric.dumazet@...il.com, netdev@...r.kernel.org,
	shemminger@...ux-foundation.org, andy@...yhouse.net,
	"Fischer, Anna" <anna.fischer@...com>
Subject: Re: [patch net-next-2.6 V3] net: convert bonding to use rx_handler

Nicolas de Pesloüan 	<nicolas.2p.debian@...il.com> wrote:

>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?

	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@...ibm.com
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists