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

[Kevin Traynor <ktraynor@redhat.com>]

re-adding dev mailing list

On 03/11/2022 10:51, Kevin Traynor wrote:
> 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 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;
>>>>>>>
>>>>>
>>
>