On 03/11/2022 14:06, Yang, Qiming wrote:
Hi, Kevin
-----Original Message-----
From: Kevin Traynor <ktray...@redhat.com>
Sent: Thursday, November 3, 2022 6:52 PM
To: Zhou, YidingX <yidingx.z...@intel.com>
Cc: Yang, Qiming <qiming.y...@intel.com>; Zhang, Qi Z
<qi.z.zh...@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 <ktray...@redhat.com>; dev@dpdk.org
Subject: RE: [PATCH v2] net/iavf: revert fix VLAN insertion
-----Original Message-----
From: Kevin Traynor <ktray...@redhat.com>
Sent: Wednesday, October 19, 2022 4:53 PM
To: Zhou, YidingX <yidingx.z...@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.z...@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;
