[patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Jiri Pirko]

No need to do share check here since call to netif_rx was removed and
skb is passed back.

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/bonding/bond_main.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f3e5d7f..e8c0f95 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1503,10 +1503,6 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
 	if (unlikely(!slave))
 		return skb;
 
-	skb = skb_share_check(skb, GFP_ATOMIC);
-	if (unlikely(!skb))
-		return NULL;
-
 	bond_dev = ACCESS_ONCE(skb->dev->master);
 	if (unlikely(!bond_dev))
 		return skb;
-- 
1.7.3.4

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Jiri Pirko]

Unapplicable, sorry (wrong branch :(). Here's corrected patch:

Subject: [PATCH net-next-2.6 v2] bonding: remove skb_share_check in handle_frame

No need to do share check here.

Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 drivers/net/bonding/bond_main.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 584f97b..367ea60 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1498,9 +1498,6 @@ 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 = ACCESS_ONCE(slave_dev->master);
 	if (unlikely(!bond_dev))
-- 
1.7.3.4

	

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Changli Gao]

On Tue, Mar 1, 2011 at 5:29 PM, Jiri Pirko <jpirko@redhat.com> wrote:
> Unapplicable, sorry (wrong branch :(). Here's corrected patch:
>
> Subject: [PATCH net-next-2.6 v2] bonding: remove skb_share_check in handle_frame
>
> No need to do share check here.
>

I don't think so. Although you avoid netif_rx(), you can't avoid
ptype_all handlers. In fact, all the RX handlers should has this
check(), if they may modify the skb.


-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Nicolas de Pesloüan]

Le 01/03/2011 16:12, Changli Gao a écrit :
> On Tue, Mar 1, 2011 at 5:29 PM, Jiri Pirko<jpirko@redhat.com>  wrote:
>> Unapplicable, sorry (wrong branch :(). Here's corrected patch:
>>
>> Subject: [PATCH net-next-2.6 v2] bonding: remove skb_share_check in handle_frame
>>
>> No need to do share check here.
>>
>
> I don't think so. Although you avoid netif_rx(), you can't avoid
> ptype_all handlers. In fact, all the RX handlers should has this
> check(), if they may modify the skb.

Can you please develop your explanation?

In current __netif_receive_skb() (after the recent patch from Jiri), we deliver the skb to ptype_all 
handlers inside a loop, while possibly changing skb->dev inside this loop.

Then, at the end of __netif_receive_skb(), we loop on ptype_base, without changing anything in skb.

Should we consider ptype_*->func() to be called in a pure sequential way? Should we consider that 
when a ptype_*->func() returns, nothing from this handler will use the skb in anyway later, in a 
parallel way?

Or should we, instead, consider that special precautions must be taken, because protocol handlers 
may run in parallel for the same skb? Which kind of precautions?

Thanks.

	Nicolas.

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Changli Gao]

On Wed, Mar 2, 2011 at 4:11 AM, Nicolas de Pesloüan
<nicolas.2p.debian@gmail.com> wrote:
> Le 01/03/2011 16:12, Changli Gao a écrit :
>>
>> On Tue, Mar 1, 2011 at 5:29 PM, Jiri Pirko<jpirko@redhat.com>  wrote:
>>>
>>> Unapplicable, sorry (wrong branch :(). Here's corrected patch:
>>>
>>> Subject: [PATCH net-next-2.6 v2] bonding: remove skb_share_check in
>>> handle_frame
>>>
>>> No need to do share check here.
>>>
>>
>> I don't think so. Although you avoid netif_rx(), you can't avoid
>> ptype_all handlers. In fact, all the RX handlers should has this
>> check(), if they may modify the skb.
>
> Can you please develop your explanation?
>
> In current __netif_receive_skb() (after the recent patch from Jiri), we
> deliver the skb to ptype_all handlers inside a loop, while possibly changing
> skb->dev inside this loop.
>
> Then, at the end of __netif_receive_skb(), we loop on ptype_base, without
> changing anything in skb.
>
> Should we consider ptype_*->func() to be called in a pure sequential way?
> Should we consider that when a ptype_*->func() returns, nothing from this
> handler will use the skb in anyway later, in a parallel way?
>
> Or should we, instead, consider that special precautions must be taken,
> because protocol handlers may run in parallel for the same skb? Which kind
> of precautions?
>

If the packets gotten by __netif_receive_skb() are unshared, the skb
gotten by bond should be unshared, as we call prev_pt before calling
bond. I don't see there is any  relationship with the previous patch
from Jiri. The bridge is in the same condition with bond here, and it
checks if the skb is shared or not. Does it imply that dev->rx_handler
may see shared skbs?

BTW: bond may change skb and packet data. But before change the packet
data, it doesn't check if the packet data is shared or not.

1519         if (bond_dev->priv_flags & IFF_MASTER_ALB &&
1520             bond_dev->priv_flags & IFF_BRIDGE_PORT &&
1521             skb->pkt_type == PACKET_HOST) {
1522                 u16 *dest = (u16 *) eth_hdr(skb)->h_dest;

skb_cow_head() should be added, otherwise the previous ptype handler
may get the unexpected dset MAC address.

1523
1524                 memcpy(dest, bond_dev->dev_addr, ETH_ALEN);
1525         }

Thanks.

-- 
Regards,
Changli Gao(xiaosuo@gmail.com)

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Nicolas de Pesloüan]

Le 02/03/2011 17:13, Changli Gao a écrit :
> On Wed, Mar 2, 2011 at 4:11 AM, Nicolas de Pesloüan
> <nicolas.2p.debian@gmail.com>  wrote:
>> Le 01/03/2011 16:12, Changli Gao a écrit :
>>> I don't think so. Although you avoid netif_rx(), you can't avoid
>>> ptype_all handlers. In fact, all the RX handlers should has this
>>> check(), if they may modify the skb.
>>
>> Can you please develop your explanation?
>>
>> In current __netif_receive_skb() (after the recent patch from Jiri), we
>> deliver the skb to ptype_all handlers inside a loop, while possibly changing
>> skb->dev inside this loop.
>>
>> Then, at the end of __netif_receive_skb(), we loop on ptype_base, without
>> changing anything in skb.
>>
>> Should we consider ptype_*->func() to be called in a pure sequential way?
>> Should we consider that when a ptype_*->func() returns, nothing from this
>> handler will use the skb in anyway later, in a parallel way?
>>
>> Or should we, instead, consider that special precautions must be taken,
>> because protocol handlers may run in parallel for the same skb? Which kind
>> of precautions?
>>
>
> If the packets gotten by __netif_receive_skb() are unshared, the skb
> gotten by bond should be unshared, as we call prev_pt before calling
> bond. I don't see there is any  relationship with the previous patch
> from Jiri. The bridge is in the same condition with bond here, and it
> checks if the skb is shared or not. Does it imply that dev->rx_handler
> may see shared skbs?

Thanks for you explanations.

My question is not strictly linked to bonding, but more general to __netif_receive_skb().

Jiri's patch added a "goto another_round" if the rx_handler changed skb->dev. (The idea was from 
me). The ptype_all list_foreach_entry_rcu delivery loop is between "another_round:" and "goto 
another_round", so some ptype_all handlers will receive an skb where skb->dev will have changed.

I wonder whether this might cause any troubles and if yes, what should be done to fix it.

And depending on the answer, I wonder whether we can move the ptype_base loop to the same place as 
the ptype_all loop. This would allow for a better handling of orig_dev, I think.

	Nicolas.

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Andy Gospodarek]

On Tue, Mar 01, 2011 at 10:29:07AM +0100, Jiri Pirko wrote:
> Unapplicable, sorry (wrong branch :(). Here's corrected patch:
> 
> Subject: [PATCH net-next-2.6 v2] bonding: remove skb_share_check in handle_frame
> 
> No need to do share check here.
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
> ---
>  drivers/net/bonding/bond_main.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 584f97b..367ea60 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1498,9 +1498,6 @@ 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 = ACCESS_ONCE(slave_dev->master);
>  	if (unlikely(!bond_dev))
> -- 
> 1.7.3.4
> 

Why did you decide to get rid of it here rather than the 3 places in the
bonding driver where it is currently needed?  I think this can cover
those cases since bond_handle_frame will be called after the ptype_all
handlers before any of the ptype handlers.

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Jiri Pirko]

Tue, Mar 01, 2011 at 09:38:43PM CET, andy@greyhouse.net wrote:
>On Tue, Mar 01, 2011 at 10:29:07AM +0100, Jiri Pirko wrote:
>> Unapplicable, sorry (wrong branch :(). Here's corrected patch:
>> 
>> Subject: [PATCH net-next-2.6 v2] bonding: remove skb_share_check in handle_frame
>> 
>> No need to do share check here.
>> 
>> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>> ---
>>  drivers/net/bonding/bond_main.c |    3 ---
>>  1 files changed, 0 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 584f97b..367ea60 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1498,9 +1498,6 @@ 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 = ACCESS_ONCE(slave_dev->master);
>>  	if (unlikely(!bond_dev))
>> -- 
>> 1.7.3.4
>> 
>
>Why did you decide to get rid of it here rather than the 3 places in the
>bonding driver where it is currently needed?  I think this can cover
>those cases since bond_handle_frame will be called after the ptype_all
>handlers before any of the ptype handlers.

I have already a patch prepared which converts bond ptype handlers into
being called from bond_handle_frame. You are propably right that this
should probably stay here.

So please Dave, drop this patch for now. Thanks.

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Jiri Pirko]

Wed, Mar 02, 2011 at 11:03:55AM CET, jpirko@redhat.com wrote:
>Tue, Mar 01, 2011 at 09:38:43PM CET, andy@greyhouse.net wrote:
>>On Tue, Mar 01, 2011 at 10:29:07AM +0100, Jiri Pirko wrote:
>>> Unapplicable, sorry (wrong branch :(). Here's corrected patch:
>>> 
>>> Subject: [PATCH net-next-2.6 v2] bonding: remove skb_share_check in handle_frame
>>> 
>>> No need to do share check here.
>>> 
>>> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
>>> ---
>>>  drivers/net/bonding/bond_main.c |    3 ---
>>>  1 files changed, 0 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 584f97b..367ea60 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -1498,9 +1498,6 @@ 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 = ACCESS_ONCE(slave_dev->master);
>>>  	if (unlikely(!bond_dev))
>>> -- 
>>> 1.7.3.4
>>> 
>>
>>Why did you decide to get rid of it here rather than the 3 places in the
>>bonding driver where it is currently needed?  I think this can cover
>>those cases since bond_handle_frame will be called after the ptype_all
>>handlers before any of the ptype handlers.
>
>I have already a patch prepared which converts bond ptype handlers into
>being called from bond_handle_frame. You are propably right that this
>should probably stay here.
>
>So please Dave, drop this patch for now. Thanks.

Thinking about this more I'm pretty convinced that skb_share_check is
not needed here.

If I got that correctly, skb_share_check is neede when user acually
modifies skb for his needs only. On the other hand, the only change
to skb is setting skb->dev and this change needs to be visible later on.
And given that skb is returned at the end of the function, changes are
never local (makes sense).

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Nicolas de Pesloüan]

Le 02/03/2011 11:03, Jiri Pirko a écrit :
> Tue, Mar 01, 2011 at 09:38:43PM CET, andy@greyhouse.net wrote:
>> On Tue, Mar 01, 2011 at 10:29:07AM +0100, Jiri Pirko wrote:
>>> Unapplicable, sorry (wrong branch :(). Here's corrected patch:
>>>
>>> Subject: [PATCH net-next-2.6 v2] bonding: remove skb_share_check in handle_frame
>>>
>>> No need to do share check here.
>>>
>>> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>>> ---
>>>   drivers/net/bonding/bond_main.c |    3 ---
>>>   1 files changed, 0 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index 584f97b..367ea60 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -1498,9 +1498,6 @@ 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 = ACCESS_ONCE(slave_dev->master);
>>>   	if (unlikely(!bond_dev))
>>> --
>>> 1.7.3.4
>>>
>>
>> Why did you decide to get rid of it here rather than the 3 places in the
>> bonding driver where it is currently needed?  I think this can cover
>> those cases since bond_handle_frame will be called after the ptype_all
>> handlers before any of the ptype handlers.
>
> I have already a patch prepared which converts bond ptype handlers into
> being called from bond_handle_frame. You are propably right that this
> should probably stay here.

Hi Jiri,

Do you plan to call the bonding ARP handler from inside bond_handle_frame()?

A few days ago (http://marc.info/?l=linux-netdev&m=129883949022340&w=2), I noticed that it is not 
possible to call the bonding ARP handler from inside the bonding rx_handler, because some frame 
processing may be required after the bonding rx_handler call, to put the frame in a suitable state 
for the bonding ARP handler.

This is at least true with the following setup, eth0 -> bond0 -> bond0.100, where the ARP frames are 
VLAN tagged at the time the bonding rx_handler process them.

	Nicolas.

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Jiri Pirko]

Wed, Mar 02, 2011 at 09:47:50PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 02/03/2011 11:03, Jiri Pirko a écrit :
>>Tue, Mar 01, 2011 at 09:38:43PM CET, andy@greyhouse.net wrote:
>>>On Tue, Mar 01, 2011 at 10:29:07AM +0100, Jiri Pirko wrote:
>>>>Unapplicable, sorry (wrong branch :(). Here's corrected patch:
>>>>
>>>>Subject: [PATCH net-next-2.6 v2] bonding: remove skb_share_check in handle_frame
>>>>
>>>>No need to do share check here.
>>>>
>>>>Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>>>>---
>>>>  drivers/net/bonding/bond_main.c |    3 ---
>>>>  1 files changed, 0 insertions(+), 3 deletions(-)
>>>>
>>>>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>>>index 584f97b..367ea60 100644
>>>>--- a/drivers/net/bonding/bond_main.c
>>>>+++ b/drivers/net/bonding/bond_main.c
>>>>@@ -1498,9 +1498,6 @@ 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 = ACCESS_ONCE(slave_dev->master);
>>>>  	if (unlikely(!bond_dev))
>>>>--
>>>>1.7.3.4
>>>>
>>>
>>>Why did you decide to get rid of it here rather than the 3 places in the
>>>bonding driver where it is currently needed?  I think this can cover
>>>those cases since bond_handle_frame will be called after the ptype_all
>>>handlers before any of the ptype handlers.
>>
>>I have already a patch prepared which converts bond ptype handlers into
>>being called from bond_handle_frame. You are propably right that this
>>should probably stay here.
>
>Hi Jiri,
>
>Do you plan to call the bonding ARP handler from inside bond_handle_frame()?

I do - it's part of patchset I've cooked (going to test that tomorrow).

>
>A few days ago
>(http://marc.info/?l=linux-netdev&m=129883949022340&w=2), I noticed
>that it is not possible to call the bonding ARP handler from inside
>the bonding rx_handler, because some frame processing may be required
>after the bonding rx_handler call, to put the frame in a suitable
>state for the bonding ARP handler.

Do you see another scenario besides the next one?

>
>This is at least true with the following setup, eth0 -> bond0 ->
>bond0.100, where the ARP frames are VLAN tagged at the time the
>bonding rx_handler process them.

Isn't this scenario resolved by vlan_on_bond_hook ?

eth0
  ->rx_handler -> another round
bond0
  ->vlan_hwaccel_do_receive -> __netif_receive_skb
bond0.100
  ->vlan_on_bond_hook -> reinject to bond0


>
>	Nicolas.

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Nicolas de Pesloüan]

Le 02/03/2011 22:12, Jiri Pirko a écrit :
> Wed, Mar 02, 2011 at 09:47:50PM CET, nicolas.2p.debian@gmail.com wrote:
>> Hi Jiri,
>>
>> Do you plan to call the bonding ARP handler from inside bond_handle_frame()?
>
> I do - it's part of patchset I've cooked (going to test that tomorrow).
>
>> A few days ago
>> (http://marc.info/?l=linux-netdev&m=129883949022340&w=2), I noticed
>> that it is not possible to call the bonding ARP handler from inside
>> the bonding rx_handler, because some frame processing may be required
>> after the bonding rx_handler call, to put the frame in a suitable
>> state for the bonding ARP handler.
>
> Do you see another scenario besides the next one?

None that currently work, but eth0 -> bond0 -> br0 -> br0.100 should work too.

>> This is at least true with the following setup, eth0 ->  bond0 ->
>> bond0.100, where the ARP frames are VLAN tagged at the time the
>> bonding rx_handler process them.
>
> Isn't this scenario resolved by vlan_on_bond_hook ?
>
> eth0
>    ->rx_handler ->  another round
> bond0
>    ->vlan_hwaccel_do_receive ->  __netif_receive_skb
> bond0.100
>    ->vlan_on_bond_hook ->  reinject to bond0

Yes, it is, but this hack does not solve the eth0 -> bond0 -> br0 -> br0.100 configuration.

All those handlers that call netif_rx() or __netif_receive_skb() sound horrible to me. Can you 
imagine the global overhead of the above receive path?

The reason why I suggested you introduce the goto another_round is because most - if not all - 
stacking configurations could/should be handled simply by returning the right skb from the 
rx_handler and let __netif_receive_skb() loop. And by having the right orig_dev logic inside 
__netif_receive_skb(), it could be possible to remove the current vlan_on_bond_hook hack.

My question about whether the skb is shared between the protocol handlers (in another thread) also 
target at this idea.

You will probably told me I'm free to propose patchs for all that, and you are right. Just missing 
the time to do so.

	Nicolas.

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Jiri Pirko]

Wed, Mar 02, 2011 at 10:54:03PM CET, nicolas.2p.debian@gmail.com wrote:
>Le 02/03/2011 22:12, Jiri Pirko a écrit :
>>Wed, Mar 02, 2011 at 09:47:50PM CET, nicolas.2p.debian@gmail.com wrote:
>>>Hi Jiri,
>>>
>>>Do you plan to call the bonding ARP handler from inside bond_handle_frame()?
>>
>>I do - it's part of patchset I've cooked (going to test that tomorrow).
>>
>>>A few days ago
>>>(http://marc.info/?l=linux-netdev&m=129883949022340&w=2), I noticed
>>>that it is not possible to call the bonding ARP handler from inside
>>>the bonding rx_handler, because some frame processing may be required
>>>after the bonding rx_handler call, to put the frame in a suitable
>>>state for the bonding ARP handler.
>>
>>Do you see another scenario besides the next one?
>
>None that currently work, but eth0 -> bond0 -> br0 -> br0.100 should work too.
>
>>>This is at least true with the following setup, eth0 ->  bond0 ->
>>>bond0.100, where the ARP frames are VLAN tagged at the time the
>>>bonding rx_handler process them.
>>
>>Isn't this scenario resolved by vlan_on_bond_hook ?
>>
>>eth0
>>   ->rx_handler ->  another round
>>bond0
>>   ->vlan_hwaccel_do_receive ->  __netif_receive_skb
>>bond0.100
>>   ->vlan_on_bond_hook ->  reinject to bond0
>
>Yes, it is, but this hack does not solve the eth0 -> bond0 -> br0 -> br0.100 configuration.
>
>All those handlers that call netif_rx() or __netif_receive_skb()
>sound horrible to me. Can you imagine the global overhead of the
>above receive path?
>
>The reason why I suggested you introduce the goto another_round is
>because most - if not all - stacking configurations could/should be
>handled simply by returning the right skb from the rx_handler and let
>__netif_receive_skb() loop. And by having the right orig_dev logic
>inside __netif_receive_skb(), it could be possible to remove the
>current vlan_on_bond_hook hack.

Well that wouldn't solve the problem. if we just got anorther_round the
packed would not be delivered to bond0.100 but only to bond0. That's
also unwanted. Well, this think shouldn't have been added here in the
first place :(

>
>My question about whether the skb is shared between the protocol
>handlers (in another thread) also target at this idea.
>
>You will probably told me I'm free to propose patchs for all that,
>and you are right. Just missing the time to do so.
>
>	Nicolas.

Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame

[Nicolas de Pesloüan]

Le 03/03/2011 07:14, Jiri Pirko a écrit :
> Wed, Mar 02, 2011 at 10:54:03PM CET, nicolas.2p.debian@gmail.com wrote:
[snip]
>> All those handlers that call netif_rx() or __netif_receive_skb()
>> sound horrible to me. Can you imagine the global overhead of the
>> above receive path?
>>
>> The reason why I suggested you introduce the goto another_round is
>> because most - if not all - stacking configurations could/should be
>> handled simply by returning the right skb from the rx_handler and let
>> __netif_receive_skb() loop. And by having the right orig_dev logic
>> inside __netif_receive_skb(), it could be possible to remove the
>> current vlan_on_bond_hook hack.
>
> Well that wouldn't solve the problem. if we just got anorther_round the
> packed would not be delivered to bond0.100 but only to bond0. That's
> also unwanted. Well, this think shouldn't have been added here in the
> first place :(

Yes, the packet would be delivered "exact match" to bond0, and this is what the bonding ARP handler 
expects, because it registered at this level, then deliver "exact match or NULL" to bond0.100, if 
appropriate, to whatever other protocol handlers.

There is a subtile difference between ptype_all and ptype_base delivery:

- The ptype_all handlers will receive the frame between two call to rx_handlers.
- The ptype_base handlers will receive the frame after all rx_handlers (and hard coded handlers like 
vlan) were called.

So a ptype_all handler might receive vlan tagged frame, if it register below the interface that 
untag the frame, while a ptype_base handler will always receive the untagged frame, because 
ptype_base delivery is done at the end of __netif_receive_skb().

I don't know whether it is desirable, but it used to be required for the bonding ARP monitor to work.

May be we should add some extra properties to protocol handler, so they can tell 
__netif_receive_skb() what they expect :

- a property to tell whether the protocol handler want the exact frame that cross the interface they 
registered on or the final frame, after all rx_handlers.
- a property to tell what they expect a the orig_dev value : the lowest interface known to 
__netif_receive_skb() (f_packet) or the interface one level below the interface they registered on 
(bonding ARP monitor).

Having those properties, I consider we would be in a position to remove most special hacks we 
currently have to handle particular stacking configuration.

	Nicolas.