From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6D9ACA00C2; Thu, 3 Nov 2022 15:43:01 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 05FA542D03; Thu, 3 Nov 2022 15:43:01 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id 7E60642D02 for ; Thu, 3 Nov 2022 15:42:58 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1667486577; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TSsdOZDSj0N84AlZ8SJwo95HbBG75+yOsWsDBwlds30=; b=QibFY7ZM9izAEvsNbl5M7kDmkMideFiyDoR+I1aQNvxoIkBIC7BJSN4wYcU7ljR/N2kau8 Y2Qk7r6zEwVkRQRjbak1Q3LEDKv8UNhP5fD+6EB9bkgFE8KoGYgvuM9Tr5XR6xfIz/OCIP I8p4DlGxlY9G4V8orrEgn6+rgj0Yw94= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-628-ZZYuJ97mPNapNlJRpwkRvw-1; Thu, 03 Nov 2022 10:42:56 -0400 X-MC-Unique: ZZYuJ97mPNapNlJRpwkRvw-1 Received: by mail-wm1-f72.google.com with SMTP id p14-20020a05600c204e00b003cf4cce4da5so576358wmg.0 for ; Thu, 03 Nov 2022 07:42:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:subject:from:references:cc:to :content-language:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=TSsdOZDSj0N84AlZ8SJwo95HbBG75+yOsWsDBwlds30=; b=aRX3NhqWP/vUXjofEAQyxHK8Io4+9AMivYFs7JA6HM5xxEWgNW4IAe61hfyH6j7E3e zFzusjkay77CQ+MkvPZ8zpBfwyLSBId7C4N7dEq7i3s44Yetblg1AnuGWHYQoU5Gosgt 0vA1Gyznu4jvDO/E2nbRC5Zc9I14iNLW3VuSCrTubjD+mmbTPrXTS1seXztpr/R6hDK7 yVZAQQ2Yx8uE6053pDK/KmAKLGRCPNTIwqyYZzIK+I/RNdDTYJQ4Tk+Ew/q0ScaC6iSQ OENIi7Ruc3J++rFMuXF5rhYE4xKHZZFLBotJ0BUECFM3V4hCAzwh5L/K6zaeJgRQaVZA usaw== X-Gm-Message-State: ACrzQf3pMm9mww0Zw7p8hauHTZkDnUxU4gYRUCzkp+Fx1O14YOcdY8oH QV5d7Me0IaELddPnaqGg9HwzQfBWJ77AYG/Ig1gNY+AklnXE6MB9zDZ1KcsxU8NjxAAiX7KcK0A DdBM= X-Received: by 2002:a5d:45d0:0:b0:236:8201:1cb7 with SMTP id b16-20020a5d45d0000000b0023682011cb7mr18801363wrs.417.1667486575446; Thu, 03 Nov 2022 07:42:55 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7d+pzNnbGU/UjhbJJJaJ3jFHGg9pBDobwSN9xDlirdxWSSjXRV8KVNmRnglYYNOa8X0NJ45g== X-Received: by 2002:a5d:45d0:0:b0:236:8201:1cb7 with SMTP id b16-20020a5d45d0000000b0023682011cb7mr18801354wrs.417.1667486575267; Thu, 03 Nov 2022 07:42:55 -0700 (PDT) Received: from [192.168.0.36] ([78.17.177.122]) by smtp.gmail.com with ESMTPSA id l6-20020a5d5266000000b00236a16c00ffsm1088476wrc.43.2022.11.03.07.42.54 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 03 Nov 2022 07:42:54 -0700 (PDT) Message-ID: <6df8df46-9b94-663e-017c-8cebf2c27ed9@redhat.com> Date: Thu, 3 Nov 2022 14:42:53 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.0 To: "Yang, Qiming" , "Zhou, YidingX" , "dev@dpdk.org" Cc: "Zhang, Qi Z" References: <20221018102602.217673-1-yidingx.zhou@intel.com> <20221019075432.9698-1-yidingx.zhou@intel.com> <30c5f90a-acd5-eacb-7ad9-8e8e6819d67d@redhat.com> <322c348e-3461-c7ab-a845-2782ffce5ef9@redhat.com> From: Kevin Traynor Subject: Re: [PATCH v2] net/iavf: revert fix VLAN insertion In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 03/11/2022 14:06, Yang, Qiming wrote: > Hi, Kevin > >> -----Original Message----- >> From: Kevin Traynor >> Sent: Thursday, November 3, 2022 6:52 PM >> To: Zhou, YidingX >> Cc: Yang, Qiming ; Zhang, Qi Z >> >> 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 ; dev@dpdk.org >>>> Subject: RE: [PATCH v2] net/iavf: revert fix VLAN insertion >>>> >>>> >>>> >>>>>>> -----Original Message----- >>>>>>> From: Kevin Traynor >>>>>>> Sent: Wednesday, October 19, 2022 4:53 PM >>>>>>> To: Zhou, YidingX ; 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 >>>>>>>> --- >>>>>>>> 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; >>>>>>>> >>>>>> >>> >