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