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 B95DBA0C46 for ; Fri, 18 Jun 2021 12:13:10 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id AC8CA40142; Fri, 18 Jun 2021 12:13:10 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mails.dpdk.org (Postfix) with ESMTP id D383040142 for ; Fri, 18 Jun 2021 12:13:08 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1624011188; 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=HAhWVboRoczjWMB0pGrPpv0ohO5Bc2q/+Q30hHlzU/M=; b=P+D1L391GiOk3r1n33B+cxbpsbtJHbg+Mlc/+TtguFBe3IqWvmCBkIJWLdJKFNK0V9mf7L bkbyHxTeslnThUX1gTcXHuzgr1rbgvDtgQUkXOosggOe9W9ZlExvp8rQO2UCV7vdQyidTR kZn1uZtLRlpsCy9QxC0YaOwZlxLeryY= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-43-NTBPN4VhPp-AprA-Lfm5ug-1; Fri, 18 Jun 2021 06:12:36 -0400 X-MC-Unique: NTBPN4VhPp-AprA-Lfm5ug-1 Received: by mail-wm1-f71.google.com with SMTP id h9-20020a05600c3509b02901b985251fdcso3162157wmq.9 for ; Fri, 18 Jun 2021 03:12:35 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:cc:references:from:subject:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=HAhWVboRoczjWMB0pGrPpv0ohO5Bc2q/+Q30hHlzU/M=; b=JqXSGZvui/iTnAhGa2cSMNsuGfFocpVnYMZ2Wd0SN/oXy+JoY76yhnRXRGnlqiFgNW O7+8ib801dDhEJVYco2Bz5XY4ERQCSuxFec+Dtn1OJzFLSW7R3M/MZJTRFzynmyvGYVq VCgftINTnxpWyXR+WwgjcUAOh9h9+2cMjBO3OHdFoMRTr6d31Z3t/Qmnbm3BRJTU1KyR D1Efa3IIldNJrwSWfT9wTLDX+AQHnmV8qzYrAljTXpdWZGC2/sbHRQSUVofSfiNLAGyp kTCAx8n+0BmwEAyUECJmo09jKst+lh9DtVv584p+zTIbMCSjf4+AQ1qP+io7lr9uXmpM oiLQ== X-Gm-Message-State: AOAM532Ck3aoaFwlLNCDRTuAH0pf56hBqGwbmXQZ9rXi8IeENnY+5cY9 4+0CGaBL3RIRZoheuS3zjdoIwiORZDx3YFc4z78EAXseXxfkxrXYL2OOuKO9sCkhYclZ2S2MIo6 3ratjAWQ= X-Received: by 2002:a05:600c:2284:: with SMTP id 4mr10755248wmf.148.1624011154860; Fri, 18 Jun 2021 03:12:34 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyPd5kiCTQOUcGzTB1FJVAAK26H6jtBOmsSm8vOnJ3YGgkI+fkZjWLK51FkYuFOUyRUPGbiqA== X-Received: by 2002:a05:600c:2284:: with SMTP id 4mr10755233wmf.148.1624011154708; Fri, 18 Jun 2021 03:12:34 -0700 (PDT) Received: from [192.168.0.36] ([78.17.79.77]) by smtp.gmail.com with ESMTPSA id g83sm7490074wma.10.2021.06.18.03.12.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 18 Jun 2021 03:12:33 -0700 (PDT) To: "Wang, Haiyue" , "Xueming(Steven) Li" , Luca Boccassi , "stable@dpdk.org" Cc: NBU-Contact-Thomas Monjalon , "christian.ehrhardt@canonical.com" , "Zhang, Qi Z" , "Fu, Qi" References: <20210611065825.47678-1-haiyue.wang@intel.com> <20210611071531.48411-1-haiyue.wang@intel.com> <30bf553b-032c-d992-487f-794cbe1816fe@redhat.com> From: Kevin Traynor Message-ID: <89fa4744-6ae6-7fe4-c9fb-8e5dbff2cfd8@redhat.com> Date: Fri, 18 Jun 2021 11:12:32 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=ktraynor@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-stable] [PATCH 20.11 v2 00/18] Backport the new VLAN design for Intel ice PMD X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" On 18/06/2021 04:22, Wang, Haiyue wrote: >> -----Original Message----- >> From: Kevin Traynor >> Sent: Thursday, June 17, 2021 18:05 >> To: Xueming(Steven) Li ; Wang, Haiyue ; Luca Boccassi >> ; stable@dpdk.org >> Cc: NBU-Contact-Thomas Monjalon ; christian.ehrhardt@canonical.com; Zhang, Qi Z >> >> Subject: Re: [dpdk-stable] [PATCH 20.11 v2 00/18] Backport the new VLAN design for Intel ice PMD >> >> On 17/06/2021 09:53, Xueming(Steven) Li wrote: >>> Hi Haiyue, >>> >>>> -----Original Message----- >>>> From: Wang, Haiyue >>>> Sent: Thursday, June 17, 2021 9:16 AM >>>> To: Luca Boccassi ; stable@dpdk.org >>>> Cc: Xueming(Steven) Li ; NBU-Contact-Thomas Monjalon ; >>>> christian.ehrhardt@canonical.com; ktraynor@redhat.com; Zhang, Qi Z >>>> Subject: RE: [dpdk-stable] [PATCH 20.11 v2 00/18] Backport the new VLAN design for Intel ice PMD >>>> >>>>> -----Original Message----- >>>>> From: Luca Boccassi >>>>> Sent: Wednesday, June 16, 2021 23:47 >>>>> To: Wang, Haiyue ; stable@dpdk.org >>>>> Cc: xuemingl@nvidia.com; thomas@monjalon.net; >>>>> christian.ehrhardt@canonical.com; ktraynor@redhat.com; Zhang, Qi Z >>>>> >>>>> Subject: Re: [dpdk-stable] [PATCH 20.11 v2 00/18] Backport the new >>>>> VLAN design for Intel ice PMD >>>>> >>>>> On Fri, 2021-06-11 at 15:15 +0800, Haiyue Wang wrote: >>>>>> When LTS 20.11 was released, the Intel ice PMD has a basic VLAN >>>>>> offload, which can only handle single VLAN mode for firmware >>>>>> limitation. Now the firmware is updated to support double VLAN mode >>>>>> and single VLAN mode at the same time. It depends on the driver to do selection at the boot time. >>>>>> >>>>>> As VLAN protocol handling like strip, filter, flow is very common >>>>>> use, we request to support the ice PMD can run on the latest >>>>>> firmware for enabling the new design. This is compatible backport as the main tree. >>>>>> >>>>>> v2: Fix the subject fix with messy code like : PATCHÂ >>>>>> >>>>>> Haiyue Wang (4): >>>>>> net/ice/base: do not set VLAN mode in DCF mode >>>>>> net/ice: fix VLAN strip for double VLAN >>>>>> net/ice: fix VLAN 0 adding based on VLAN mode >>>>>> net/ice: update QinQ switch filter handling >>>>>> >>>>>> Junfeng Guo (1): >>>>>> net/ice: enable QinQ filter for switch >>>>>> >>>>>> Qi Zhang (12): >>>>>> net/ice/base: align add VSI and update VSI AQ command buffer >>>>>> net/ice/base: add interface to support configuring VLAN mode >>>>>> net/ice/base: fix outer VLAN related macro >>>>>> net/ice/base: add VLAN TPID for VLAN filters >>>>>> net/ice/base: support checking double VLAN mode >>>>>> net/ice/base: support configuring device in double VLAN mode >>>>>> net/ice/base: update boost TCAM for DVM >>>>>> net/ice/base: change protocol ID for VLAN in DVM >>>>>> net/ice/base: refactor post DDP download VLAN mode config >>>>>> net/ice/base: log if DDP/FW do not support QinQ >>>>>> net/ice/base: add inner VLAN protocol type for QinQ filter >>>>>> net/ice/base: fix QinQ PPPoE dummy packet selection >>>>>> >>>>>> Yuying Zhang (1): >>>>>> net/ice/base: add ethertype offset for QinQ dummy packet >>>>>> >>>>>> drivers/net/ice/base/ice_adminq_cmd.h | 268 ++++++++----- >>>>>> drivers/net/ice/base/ice_bitops.h | 45 +++ >>>>>> drivers/net/ice/base/ice_common.c | 38 ++ >>>>>> drivers/net/ice/base/ice_common.h | 4 + >>>>>> drivers/net/ice/base/ice_flex_pipe.c | 302 +++++++++++++-- >>>>>> drivers/net/ice/base/ice_flex_pipe.h | 12 + >>>>>> drivers/net/ice/base/ice_flex_type.h | 39 ++ >>>>>> drivers/net/ice/base/ice_protocol_type.h | 1 + >>>>>> drivers/net/ice/base/ice_switch.c | 124 +++++- >>>>>> drivers/net/ice/base/ice_switch.h | 15 + >>>>>> drivers/net/ice/base/ice_type.h | 4 + >>>>>> drivers/net/ice/base/ice_vlan_mode.c | 451 ++++++++++++++++++++++ >>>>>> drivers/net/ice/base/ice_vlan_mode.h | 16 + >>>>>> drivers/net/ice/base/meson.build | 1 + >>>>>> drivers/net/ice/ice_ethdev.c | 455 +++++++++++++---------- >>>>>> drivers/net/ice/ice_ethdev.h | 10 +- >>>>>> drivers/net/ice/ice_generic_flow.c | 8 + >>>>>> drivers/net/ice/ice_generic_flow.h | 1 + >>>>>> drivers/net/ice/ice_switch_filter.c | 114 +++++- >>>>>> 19 files changed, 1545 insertions(+), 363 deletions(-) create mode >>>>>> 100644 drivers/net/ice/base/ice_vlan_mode.c >>>>>> create mode 100644 drivers/net/ice/base/ice_vlan_mode.h >>>>> >>>>> Hi, >>>>> >>>>> At 1.9k diffstat, this series is quite large. Given it's a new >>>>> feature, rather than a series of bug fixes, this would seem a bit risky to me. >>>>> Final word of course belongs to Xueming, since he's managing this one. >>>>> See: >>>>> >>>> >> >> Thanks for using the questions as a way to discuss it, it is a good way >> to see if they are useful. Just to note, they were to try and capture >> some of the important things for a maintainer to consider, it is not a >> flow chart leading to a binary answer (though clearly some things like >> ABI breakage, probably would end the discussion). >> >>>> 01. Does the feature break API/ABI? >>>> >>>> NO. >>>> >>>> 02. Does the feature break backwards compatibility? >>>> >>>> NO. >>>> >>>> 03. Is it for the latest LTS release (to avoid LTS upgrade issues)? >>>> >>>> Yes. >>>> >>>> 04. Is there a commitment from the proposer or affiliation to validate the feature and check for >> regressions in related functionality? >>>> >>>> Passed internally, if needed, an official Test-by can be provided. >>>> >> >> It would be better to share test cases (even high level), not just a >> tested-by which doesn't give any idea of test coverage. > > + Fu Qi, who can share the test cases from dts. > >> >> I would look at it like: >> The new functionality not working with the new firmware and new code is >> not a big issue. >> >> The old functionality not working with the new firmware and the new code >> is a big issue. >> >> The old functionality not working with the old firmware and the new code >> is a very big issue. >> >> So regression testing of the old functionality would be the most >> important IMHO. > > This is some kind of compatibility matrix, sure, it make both works. This part > can be covered by the test cases. > >> >>>> 05. Is there a track record of the proposer or affiliation validating stable releases? >>>> >> >> Yes, Intel tests every LTS release. >> >>>> Bugzilla ? >>>> >>>> 06. Is it obvious that the feature will not impact existing functionality? >>>> >>>> Yes. >>>> >> >> No. It is 1.9KLOC change. The key part of the question is "obvious". It >> was meant so the maintainer could use their judgement and review that >> for example, a few lines of code adding a PCI ID or adding a case in a >> switch statement, is obviously not going to impact existing functionality. >> On the other hand, for a more complex code change to existing code, it >> is not immediately obvious that there would be no risk to existing >> functionality. >> >>>> 07. How intrusive is the code change? >>>> >>>> From LOC, yes, 1.9K seems to be BIG, but DPDK PMD related is 588, other is the share code in base >> (1320), which is tested and >>>> validated on other platform. >>>> >> >> Very intrusive. It seems to be big, because it is big. >> >>>> drivers/net/ice/ice_ethdev.c | 455 +++++++++++++---------- >>>> drivers/net/ice/ice_ethdev.h | 10 +- >>>> drivers/net/ice/ice_generic_flow.c | 8 + >>>> drivers/net/ice/ice_generic_flow.h | 1 + >>>> drivers/net/ice/ice_switch_filter.c | 114 +++++- >>>> >>>> 08. What is the scope of the code change? >>>> >>>> PMD only. >>>> >>>> 09. Does it impact common components or vendor specific? >>>> >>>> NO. >>>> >>>> 10. Is there a justifiable use case (a clear user need)? >>>> >>>> Yes, for firmware updated. And we have the customer who wants to use the VLAN feature on LTS 20.11. >>>> >> >> Well, like a lot of the considerations, this is subjective and everyone >> will think there is a need for their own patches, that is a given. It is >> for the maintainer to try and balance the need of the feature against >> the possible impacts to the LTS. >> >> It seems like you mentioned "for updated firmware" and "customer who >> wants to used the VLAN feature" as separate points. If there is a >> separate need for updating firmware aside from new VLAN functionality, >> it is good to state that. >> >>>> 11. Is there a community consensus about the backport? >>>> >>>> ... >>> >>> Kevin happens to updated the documents on new feature backport 4 months ago, thanks for checking >> them >>> one by one. Luca's only concern is size of the series, driver vendor is on it's own risk to backport >> a big patch set. >>> The series supports new fw and QinQ, is it easy to split? >>> >>> Kevin, is this the first case of feature backport? How do you think? >>> >> >> Like Luca, main concern would be the size and intrusiveness of the >> changes, and if it's ok to change 1.9KLOC in this driver now, then why >> not 20KLOC in next release to multiple drivers. I had pushed against a > > TBH, we won't want to change the stable i40e, ixgbe PMDs, but ice is a fresh > one, current VLAN has a limited usage, customer is hard to use. That's why we > try to request to backport the new VLAN design. > Just to note, I wasn't specifically referring to Intel drivers, just discussing what is an acceptable limit of churn. >> LOC limit when this was last discussed at the TB, as it's a crude way to >> judge code complexity/risk, but maybe it should be considered. >> >> On the positive side it is self-contained and Intel has an excellent >> track record for testing LTS. >> >>>> >>>>> https://doc.dpdk.org/guides/contributing/stable.html#what-changes-shou >>>>> ld-be-backported >>>>> >>>>> -- >>>>> Kind regards, >>>>> Luca Boccassi >