Re: [PATCH] net: take care of bonding in build_skb_flow_key

[zhuyj <zyjzyj2000@gmail.com>]

On 01/06/2016 03:45 PM, Wengang Wang wrote:
>
>
> 在 2016年01月06日 15:35, zhuyj 写道:
>> IMHO, "The path MTU is set to the active slave device, not the 
>> bonding master."
>> Can we set PMTU to bonding master when path MTU is set to the active 
>> slave device?
>>
> Actually the route is set on bonding master, not on any slave, the 
> trying to set PMTU to the active slave failed(it didn't found a route 
> on slave).
In your commit log:

"
A problem is found that we are looking for route basing a bonding device and
deal with path MTU there: The path MTU is set to the active slave 
device, not
the bonding master.
"

and

"the trying to set PMTU to the active slave failed"

I am confused.

Zhu Yanjun

> So "set PMTU to bonding master when path MTU is set to the active 
> slave device" is lacking a base.
>
> thanks,
> wengang
>
>> If not appropriate, you can ignore it.
>>
>> Best Regards!
>> Zhu Yanjun
>>
>> On 01/06/2016 03:06 PM, Wengang Wang wrote:
>>> Hi Yanjun,
>>>
>>> Thanks for your review.
>>> Master MTU is same as that for slaves.
>>> Maybe fixing in bonding driver is a good idea, but I don't find a 
>>> good place to do that.
>>> Let's go through the simplified follow:
>>>
>>> ...
>>> 1) Fragmentation.
>>>    --This is is done is against the bonding master device(device MTU 
>>> and path MTU)
>>> 2) bond_start_xmit
>>> 3) ipoib_start_xmit(slaves are IPoIB interfaces)
>>>
>>> For the first send
>>> 1) fragment size is 7000(in my case)
>>> 2) bond_start_xmit its self is fine
>>> 3) ipoib_start_xmit sees the packet size 7000 is larger than the 
>>> internal limit 2044, drops the packet and try to update PMTU.
>>>     without the patch, it tried update PMTU on slave device(no 
>>> changes to master).
>>>
>>> the seconds send comes, since no changes happen on bonding 
>>> master(PMTU), the fragment size is still 7000 and the behavior is 
>>> just the same as the first send.
>>>
>>> With the patch, the bonding master PMTU is changed to 2044 after the 
>>> first send(hopefully), for the seconds send the fragment size is set 
>>> to 2044.
>>>
>>> To fix in bonding code, I don't find where we can.
>>>
>>> thanks,
>>> wengang
>>>
>>> 在 2016年01月06日 14:19, zhuyj 写道:
>>>>
>>>> IMHO, this should fix in bonding driver because the active slave 
>>>> mtu should be the same with the master.
>>>> bonding master's mtu is changed to path MTU, then slave dev's MTU 
>>>> should be changed, too.
>>>>
>>>> Zhu Yanjun
>>>> On 01/06/2016 01:49 PM, Wengang Wang wrote:
>>>>> A problem is found that we are looking for route basing a bonding 
>>>>> device and
>>>>> deal with path MTU there: The path MTU is set to the active slave 
>>>>> device, not
>>>>> the bonding master.
>>>>>
>>>>> The patch tries to fix the issue by letting build_skb_flow_key() 
>>>>> take care
>>>>> of the transition of device index from bonding slave to the master.
>>>>>
>>>>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>>>>> ---
>>>>>   net/ipv4/route.c | 11 ++++++++++-
>>>>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>>>>> index 85f184e..3053f10 100644
>>>>> --- a/net/ipv4/route.c
>>>>> +++ b/net/ipv4/route.c
>>>>> @@ -523,11 +523,20 @@ static void build_skb_flow_key(struct flowi4 
>>>>> *fl4, const struct sk_buff *skb,
>>>>>                      const struct sock *sk)
>>>>>   {
>>>>>       const struct iphdr *iph = ip_hdr(skb);
>>>>> -    int oif = skb->dev->ifindex;
>>>>> +    int oif;
>>>>> +    struct net_device *master = NULL;
>>>>> +
>>>>>       u8 tos = RT_TOS(iph->tos);
>>>>>       u8 prot = iph->protocol;
>>>>>       u32 mark = skb->mark;
>>>>>   +    if (skb->dev->flags & IFF_SLAVE)
>>>>> +        master = netdev_master_upper_dev_get(skb->dev);
>>>>> +    if (master)
>>>>> +        oif = master->ifindex;
>>>>> +    else
>>>>> +        oif = skb->dev->ifindex;
>>>>> +
>>>>>       __build_flow_key(fl4, sk, iph, oif, tos, prot, mark, 0);
>>>>>   }
>>>>
>>>
>>
>