Message-ID: <568CD893.3030608@oracle.com>
Date:	Wed, 6 Jan 2016 17:04:19 +0800
From:	Wengang Wang <wen.gang.wang@...cle.com>
To:	zhuyj <zyjzyj2000@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] net: take care of bonding in build_skb_flow_key

Please see if the V2 is clear.

thanks,
wengang

在 2016年01月06日 16:18, zhuyj 写道:
> On 01/06/2016 04:14 PM, Wengang Wang wrote:
>>
>>
>> 在 2016年01月06日 16:00, zhuyj 写道:
>>> 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.
>>>
>>
>> Maybe changing "The path MTU is set to the active slave device, not 
>> the bonding master" to
>> "The path MTU is tried to set to the active slave device, not to the 
>> bonding master" is better.
> Maybe you should explain your problem clearly.
>
> Zhu Yanjun
>>
>> It tried to change the PMTU to the slave. Whether the setting can 
>> succeed depends:
>> if the route is there(on slave), it goes successfully; if not no 
>> route found, it goes unsuccessfully.
>> For the no-route case, your suggestion breaks.
>>
>> thanks,
>> wengang
>>
>>> 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@...cle.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);
>>>>>>>>   }
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
> -- 
> 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

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