Re: [dpdk-dev] [PATCH] net/bonding: strengthen the judgment of lacp packets

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/19/2017 5:09 AM, zengganghui wrote:
> Local LACP packets do not have VLANs, and ethertype must be ETHER_TYPE_SLOW. But when the PMD supports VLAN strip, you cannot directly determine the ethertype from the packet, but depends on whether the VLAN is stripped. If a VLAN is stripped, then this is not a local LACP packet, but it may be a need to pass through packet. The previous code was not rigorous in determining whether the VLAN was being stripped.
> Did I answer your question?

Hi Declan,

Are you OK with the patch?
You already have your ack on patch but discussion was going on...


> 
> BR.
> Zeng Ganghui
> Huawei Technologies Co., Ltd.
> 
> -----Original Message-----
> From: Doherty, Declan [mailto:declan.doherty@intel.com] 
> Sent: Monday, September 18, 2017 9:11 PM
> To: zengganghui; dev@dpdk.org
> Subject: Re: [PATCH] net/bonding: strengthen the judgment of lacp packets
> 
> On 18/09/2017 1:50 PM, zengganghui wrote:
>> All mbuf packets have been init to zero when pktmbuf pool create. So judgment this flag is safe, whether or not support VLAN stripping.
> 
> Ok, but is there any need to check the ol_flags for PKT_RX_VLAN_PKT or check the vlan_tci at all. I haven't come across anything in the specification which allows LACP links to be formed on top of VLANs but I may be missing something? So if the ethertype is not ETHER_TYPE_SLOW it is irrelevant whether the packet has a VLAN tag or not.
> 
> Also on the basis that you could have LAG groups on top of VLANs, if the NIC doesn't support VLAN stripping/insertion then we would miss all the ingress LACP PDU at the moment now anyway, since the ethertype would be VLAN and not ETHER_TYPE_SLOW, so is_lacp_packet() would always return 0, and we would also fail to encapsulate the LACP PDU in the correct VLAN on egress as that isn't supported in the bonding implementation.
> 
>>
>> BR.
>> Zeng Ganghui
>> Huawei Technologies Co., Ltd.
>>
>> -----Original Message-----
>> From: Doherty, Declan [mailto:declan.doherty@intel.com]
>> Sent: Monday, September 18, 2017 8:34 PM
>> To: zengganghui; dev@dpdk.org
>> Subject: Re: [PATCH] net/bonding: strengthen the judgment of lacp 
>> packets
>>
>> On 18/09/2017 12:12 PM, zengganghui wrote:
>>> For example, when packets received from an MLX network card, the value of mbuf->vlan_tci is a random value. So that this value cannot be used to determine whether VLAN packets . We need to judgment mbuf->ol_flags first.
>>>
>>> BR.
>>> Zeng Ganghui
>>> Huawei Technologies Co., Ltd.
>>>
>>> -----Original Message-----
>>> From: Doherty, Declan [mailto:declan.doherty@intel.com]
>>> Sent: Monday, September 18, 2017 5:14 PM
>>> To: zengganghui; dev@dpdk.org
>>> Subject: Re: [PATCH] net/bonding: strengthen the judgment of lacp 
>>> packets
>>>
>>> On 30/08/2017 4:46 AM, ZengGanghui wrote:
>>>> When the nic does not support vlan rx offload may be wrong, 
>>>> resulting in lacp packets will not be processed.
>>>>
>>>> Signed-off-by: ZengGanghui <zengganghui@huawei.com>
>>>> ---
>>> ...
>>>>
>>>
>>> Acked-by: Declan Doherty <declan.doherty@intel.com>
>>>
>>
>> Ok, I see your point. A LACP PDU can't be encapsulated in a VLAN packet anyway, as it is link local traffic. So a check for ol_flags & PKT_RX_VLAN_PKT != 0 should be sufficient, otherwise if the PKT_RX_VLAN_PKT flag is true the packet cannot be link local and therefore a LACP PDU. I think that it's safe to assume all PMDs must set this flag if VLAN stripping is enabled?
>>