Re: [PATCH v2] net/iavf: revert fix VLAN insertion

[Kevin Traynor <ktraynor@redhat.com>]

On 03/11/2022 14:06, Yang, Qiming wrote:
> Hi, Kevin
> 
>> -----Original Message-----
>> From: Kevin Traynor <ktraynor@redhat.com>
>> Sent: Thursday, November 3, 2022 6:52 PM
>> To: Zhou, YidingX <yidingx.zhou@intel.com>
>> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>
>> Subject: Re: [PATCH v2] net/iavf: revert fix VLAN insertion
>>
>> On 03/11/2022 10:38, Zhou, YidingX wrote:
>>> Hi, Kevin
>>>
>>> According to suggestion, we did many deep investigation and various
>>> attempts, unfortunately that the performance drop(about 40%) caused by
>> the previous commit:
>>> 0d58caa7d6d1 ("net/iavf: fix VLAN insertion") Still can not be
>>> resolved in this cycle due to tight schedule.
>>>
>>> Because the performance drop is too serious and the scope of impact is
>> relatively large, we think the previous commit is a mistake.
>>
>> ok, I'm trying to understand why a performance drop is worse than some
>> corrupt packets.
>>
>> What is the scope of the performance drop? Does the scope impact more
>> cases than just when the L2TAG2 is used?
>>
>> IOW, is it a functional issue in a small use case, and performance issue in
>> more use cases? If so, I can understand you wanting to revert.
>>
> 
> For the original issue, it is QinQ insert function can't work in vector path, because we don't support this function in vector path. And after revert the patch, user still can use QinQ insert by set the Tx function to normal path as a workaround.
> But with this unreasonable patch, all the case using vector mode will have 40% performance drop, the drop is too high to accept.
> So I think we should revert it first and enable the QinQ insert in vector path in next release after well design and full performance test.
> 

ok, thanks for the explanation. I will revert on 21.11 branch when it 
reverted on main.

Kevin.

> Qiming
> 
>>> For the original bug, we plan to fix it in the next cycle (by supporting
>> L2TAG2 on the vector path).
>>>
>>
>> With the revert, is there a way to disable use of L2TAG2 being used while it is
>> incorrect? At very least, the issue should be documented/bugzilla so a user
>> can know what doesn't work correctly.
>>
>>> So we are expecting to revert the above commit from main branch and it
>> should not be merged to stable branch.
>>
>> It has already been merged but if the decision for main branch is to revert,
>> then I can revert on stable also.
>>
>> thanks,
>> Kevin.
>>
>>> Sorry I'm a little late in explaining the situation. Your understanding would
>> be appreciated.
>>>
>>> /Yiding
>>>
>>>> -----Original Message-----
>>>> From: Zhou, YidingX
>>>> Sent: Friday, October 21, 2022 10:43 AM
>>>> To: Kevin Traynor <ktraynor@redhat.com>; dev@dpdk.org
>>>> Subject: RE: [PATCH v2] net/iavf: revert fix VLAN insertion
>>>>
>>>>
>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Kevin Traynor <ktraynor@redhat.com>
>>>>>>> Sent: Wednesday, October 19, 2022 4:53 PM
>>>>>>> To: Zhou, YidingX <yidingx.zhou@intel.com>; dev@dpdk.org
>>>>>>> Subject: Re: [PATCH v2] net/iavf: revert fix VLAN insertion
>>>>>>>
>>>>>>> On 19/10/2022 08:54, Yiding Zhou wrote:
>>>>>>>> When the kernel driver tells to use the L2TAG2 field for VLAN
>>>>>>>> insertion, the context descriptor needs to be used. There is an
>>>>>>>> issue on the vector Tx path, because it does not support the
>>>>>>>> context
>>>>> descriptor.
>>>>>>>>
>>>>>>>> The previous commit forces to select normal path to avoid the
>>>>>>>> above issue, but it results in a performance loss of around 40%.
>>>>>>>> So it needs to be reverted and the original issue needed to be
>>>>>>>> fixed by
>>>> rework.
>>>>>>>>
>>>>>>>
>>>>>>> Thank you, that is a much clearer explanation.
>>>>>>>
>>>>>>> Now on the approach, the commit being reverted says:
>>>>>>> "When the driver tells the VF to insert VLAN tag using the L2TAG2
>>>>>>> field, vector Tx path does not use Tx context descriptor and would
>>>>>>> cause VLAN tag inserted into the wrong location."
>>>>>>>
>>>>>>> So it means this revert is solving a performance regression, but
>>>>>>> re-introducing the functional issue above.
>>>>>>>
>>>>>>> Is there a correct fix for the original issue sent that can be
>>>>>>> applied too? If not, wouldn't it be better to wait until it is
>>>>>>> before doing the
>>>>> revert?
>>>>>>>
>>>>>>
>>>>>> Sorry, there is no correct fix yet.
>>>>>> We plan to support context descriptor on vector path to fix the
>>>>>> original issue, It may take more time and cannot be completed
>>>>>> within this
>>>>> cycle.
>>>>>>
>>>>>
>>>>> ok, but you didn't answer the second question.
>>>>>
>>>>> "When the driver tells the VF to insert VLAN tag using the L2TAG2
>>>>> field, vector Tx path does not use Tx context descriptor and would
>>>>> cause VLAN tag inserted into the wrong location."
>>>>>
>>>>> Please explain your justification for (re-)introducing this bug?
>>>>>
>>>>> Why is better to get (corrupt?) packets with incorrect VLAN tags
>>>>> than lose performance for this case? Or have I mis-interpreted the
>> patches.
>>>>>
>>>>>
>>>> Thanks for your review.
>>>> I agree with you.  It should not re-introduce functional issue .
>>>> This revert is not needed. I will resubmit a new patch for the performance
>> loss.
>>>>
>>>>>>>> To reverts
>>>>>>>> commit 0d58caa7d6d1 ("net/iavf: fix VLAN insertion")
>>>>>>>>
>>>>>>>> Fixes: 0d58caa7d6d1 ("net/iavf: fix VLAN insertion")
>>>>>>>>
>>>>>>>> Signed-off-by: Yiding Zhou <yidingx.zhou@intel.com>
>>>>>>>> ---
>>>>>>>>      drivers/net/iavf/iavf_rxtx_vec_common.h | 3 ---
>>>>>>>>      1 file changed, 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/iavf/iavf_rxtx_vec_common.h
>>>>>>>> b/drivers/net/iavf/iavf_rxtx_vec_common.h
>>>>>>>> index 4ab22c6b2b..a59cb2ceee 100644
>>>>>>>> --- a/drivers/net/iavf/iavf_rxtx_vec_common.h
>>>>>>>> +++ b/drivers/net/iavf/iavf_rxtx_vec_common.h
>>>>>>>> @@ -253,9 +253,6 @@ iavf_tx_vec_queue_default(struct
>>>>>>>> iavf_tx_queue
>>>>> *txq)
>>>>>>>>      	if (txq->offloads & IAVF_TX_NO_VECTOR_FLAGS)
>>>>>>>>      		return -1;
>>>>>>>>
>>>>>>>> -	if (txq->vlan_flag ==
>> IAVF_TX_FLAGS_VLAN_TAG_LOC_L2TAG2)
>>>>>>>> -		return -1;
>>>>>>>> -
>>>>>>>>      	if (txq->offloads & IAVF_TX_VECTOR_OFFLOAD)
>>>>>>>>      		return IAVF_VECTOR_OFFLOAD_PATH;
>>>>>>>>
>>>>>>
>>>
>