Re: [patch net-next-2.6 6/8] bonding: move processing of recv handlers into handle_frame()

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

Le 06/03/2011 14:34, Jiri Pirko a écrit :
> Sun, Mar 06, 2011 at 01:24:32PM CET, nicolas.2p.debian@gmail.com wrote:
>> Le 05/03/2011 15:50, Nicolas de Pesloüan a écrit :
>>> Le 05/03/2011 15:43, Jiri Pirko a écrit :
>>>> Sat, Mar 05, 2011 at 03:33:30PM CET, nicolas.2p.debian@gmail.com wrote:
>>>>> Le 05/03/2011 11:29, Jiri Pirko a écrit :
>>>>>> Since now when bonding uses rx_handler, all traffic going into bond
>>>>>> device goes thru bond_handle_frame. So there's no need to go back into
>>>>>> bonding code later via ptype handlers. This patch converts
>>>>>> original ptype handlers into "bonding receive probes". These functions
>>>>>> are called from bond_handle_frame and they are registered per-mode.
>>>>>
>>>>> Does this still support having the arp_ip_target on a vlan?
>>>>>
>>>>> (eth0 ->  bond0 ->  bond0.100, with arp_ip_target only reachable
>>>>> through bond0.100).
>>>>
>>>> This case is still covered with vlan_on_bond_hook
>>>> eth0->
>>>> bond_handle_frame
>>>> bond0->
>>>> vlan_hwaccel_do_receive
>>>> bond0.5->
>>>> vlan_on_bond_hook ->  reinject into bond0
>>>> ->  bond_handle_frame (here it is processed)
>>>
>>> Sound good to me.
>>>
>>> Reviewed-by: Nicolas de Pesloüan<nicolas.2p.debian@free.fr>
>>
>> After another review, I think it won't work.
>>
>> vlan_on_bond() will reinject into bond0, but bond_handle_frame() is
>> registered as the rx_handler for the slaves (eth0 in the above
>> setup), not as the rx_handler for the master (bond0 in the above
>> setup). So, bond_handlee_frame() will never see the untagged ARP
>> request/reply and bonding ARP monitoring will fail.
>
> Damn, you are right. I mislooked.

So do I :-D

>> That being said, the current vlan_on_bond_hook() hack already suffer
>> other troubles and for example won't support the following setup:
>>
>> eth0 ->  bond0 ->  br0 ->  br0.100.
>
> blah. Well correct me if my thinking is wrong but I cannot imagine
> a scenario where there's not other way how to do arp monitoring than
> over vlan.

If you are connected to a switch smart enough to allow non tagged delivery for one vlan and all you 
arp_ip_targets are reachable through that vlan, then, yes, this hack might been useless...

But...

In real life, you may be connected to any kind of switch, including the most stupid one... or you 
may have trouble explaining to the network team the reason why you need all packets to be tagged 
expect for one vlan... or simply, some of your arp_ip_targets will be reachable through a router in 
another vlan.

> So how about to just remove the vlan_on_bond_hook and forbid the
> possibility. IMHO it should have never been introduced in the first
> place.

If it was never introduced, we would have been able to simply forbid it, but the feature exist since 
December 14th 2009, so I'm afraid we must keep it... :-/

And as a more general rule, I think we should support "all possible" interface stacking setups, 
because this would force us to think generic, instead of adding special hacks for special stacking 
setups.

>> I think we need to fix this stacking issue in a more general way.
>
> Well this issue is more or less out of the concept and breaks layering.
> I cannot think of how to resolve this nicely atm.

The only way I can imagine atm is something I described a few days ago:

1/ Add a late_delivery property to packet_type:

	struct packet_type {
	        __be16                  type;   /* This is really htons(ether_type). */
	        struct net_device       *dev;   /* NULL is wildcarded here           */
	        int                     (*func) (struct sk_buff *,
	                                         struct net_device *,
	                                         struct packet_type *);
	        struct sk_buff          *(*gso_segment)(struct sk_buff *skb,
	                                                u32 features);
	        int                     (*gso_send_check)(struct sk_buff *skb);
	        struct sk_buff          **(*gro_receive)(struct sk_buff **head,
	                                               struct sk_buff *skb);
	        int                     (*gro_complete)(struct sk_buff *skb);
	        void                    *af_packet_priv;
+		bool			late_delivery;
	        struct list_head        list;
	};

2/ Use the late_delivery property inside the ptype_all and ptype_base loops in 
__netif_receive_skb(), to skip the protocol handlers whose late_delivery property is false, while we 
walk the rx_handler path.

3/ At the end of __netif_receive_skb(), deliver the final skb to those ptype_all and ptype_base 
handlers whose late_delivery property is true.

4/ And revert back the bonding ARP monitoring to a normal protocol handler, having late_delivery == 
true.

This would work with eth0 -> bond0 -> bond0.100 and eth0 -> bond0 -> br0 -> br0.100.

This would also be reasonably generic, from my point of view. No reference to vlan, bonding, bridge 
or whatever...

	Nicolas.