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 4F3E3A0C45 for ; Thu, 17 Jun 2021 12:04:53 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3C49E40150; Thu, 17 Jun 2021 12:04:53 +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 012BF40150 for ; Thu, 17 Jun 2021 12:04:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1623924291; 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=QLcbdFMX1vw7N0RiIHvhsizDCEL5FE53g3nwOpaK/ZQ=; b=WRzFO1TsicQzso6zadyItlBvjMIrDrbwq93XM9kUsABBhemN42sfXyrT+UJennb8gHAKQ2 zvl9cU4KMfmGfNqmxcs93ShdH1w8tiyZ7ZOUEFtzRbSyQWTSuWZChrEH0CUkyqpl9Emkol pJOYQvevR84kavIJ6BSZniZzjgYLFeA= Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-241-zgfp050aNA-IWnxQQeGqPg-1; Thu, 17 Jun 2021 06:04:47 -0400 X-MC-Unique: zgfp050aNA-IWnxQQeGqPg-1 Received: by mail-wr1-f72.google.com with SMTP id x9-20020a5d49090000b0290118d8746e06so2711670wrq.10 for ; Thu, 17 Jun 2021 03:04:47 -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=QLcbdFMX1vw7N0RiIHvhsizDCEL5FE53g3nwOpaK/ZQ=; b=qowVQ5pChjDkwSXzsptyndw7862NvLWvJfTDNGhjif0RGE9BzTUXoq/MrAO0lQz5Bf RgdQjXDsa6whYTKZUBK43fBmVQAZ5xtNQnRyfVHpKiWosgJjiGs+RNViS7DvitxL9vsU In60WrG1VMbSysjO+Yr0mVvczD1DStdVc3q2MzrRUp7O/1dxDqgq6iN2BXKO9P6HZFz6 5AIrsd5isL774vdxRVQPzCn2KXcrJc+EoUspDmkweceeALcPg5mHaveOomqVz1mOeLK0 E++4EzaSvSxK/vG3Kw9S1fy9J9oWE3mCeLTBJx0qI+xnFY9XBSeWhJcEcjnOBmEaWx9Y LdNw== X-Gm-Message-State: AOAM532uTMj3RG0H5+1qW1jFTNF0nY7ukzPUt7yzHOJIOfb2QSXyZv8Y FUAv+I3b0L8mx+dK6X+hTNeYpMUzuHZQQyO+T3JVEHYBbVt2nqKd466KMz2tjyoh9AX+VBuY8qI av9RejRU= X-Received: by 2002:a1c:4954:: with SMTP id w81mr4135992wma.182.1623924286130; Thu, 17 Jun 2021 03:04:46 -0700 (PDT) X-Google-Smtp-Source: ABdhPJz/tyTda9r+8GV567QBPsSfD/XWso4LZqgiudkx9/Yqs3HWYmK3EpwCq896qH794vhyRT9CKg== X-Received: by 2002:a1c:4954:: with SMTP id w81mr4135975wma.182.1623924285912; Thu, 17 Jun 2021 03:04:45 -0700 (PDT) Received: from [192.168.0.36] ([78.17.79.77]) by smtp.gmail.com with ESMTPSA id c23sm827478wmb.38.2021.06.17.03.04.44 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 17 Jun 2021 03:04:45 -0700 (PDT) To: "Xueming(Steven) Li" , "Wang, Haiyue" , Luca Boccassi , "stable@dpdk.org" Cc: NBU-Contact-Thomas Monjalon , "christian.ehrhardt@canonical.com" , "Zhang, Qi Z" References: <20210611065825.47678-1-haiyue.wang@intel.com> <20210611071531.48411-1-haiyue.wang@intel.com> From: Kevin Traynor Message-ID: <30bf553b-032c-d992-487f-794cbe1816fe@redhat.com> Date: Thu, 17 Jun 2021 11:04:44 +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 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. 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. >> 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 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