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 EE5A942672; Fri, 29 Sep 2023 12:23:53 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 906C3402AA; Fri, 29 Sep 2023 12:23:53 +0200 (CEST) Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by mails.dpdk.org (Postfix) with ESMTP id C140440287; Fri, 29 Sep 2023 12:23:52 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 5AC335C01DD; Fri, 29 Sep 2023 06:23:52 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute1.internal (MEProxy); Fri, 29 Sep 2023 06:23:52 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:content-type:date :date:from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to; s=fm2; t= 1695983032; x=1696069432; bh=qg/lx1o1AcfNUC5D6+inW4MXVboVMYGutj8 XWk9/O/s=; b=fsfBHc4kyO9rz9JqHH3YaITABt8bvVbmfKBOb4P/VKDjLe5aZf6 /vLzB8OlNsjgrez09lCBzQ2QENiVqYOkalNFCtasXp8hByiuQ4h+bqbvJ0UbgoB4 BVDRbuiYMKN2K26WD8WLdY/6F82RGuRNsGnpmaYfvYLNK+Z6j30sHRwlgmXtWzN2 3U5bvq54zhWJnCa8G9IfpmVIQi8u8AdMBZIUGCAbKy8VxNRbjgfjrs9XT06rUBld E9s22cpoPBAMDG7OusDQXkWYNUzldTHjumTF82AiHkQeN+MylKfPu4dtou3PRt+r fSIJ5Ro9Se25N/LTHUz1mdmxpYRvE6MxjdQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:content-type:date:date:feedback-id:feedback-id :from:from:in-reply-to:in-reply-to:message-id:mime-version :references:reply-to:sender:subject:subject:to:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t= 1695983032; x=1696069432; bh=qg/lx1o1AcfNUC5D6+inW4MXVboVMYGutj8 XWk9/O/s=; b=qMo0wEY77dEdl+QAv7qP+lcCd+Nz1qiigN9vqkLOKR4d89Mv7P3 RRodqgJCKwwhQPexSDbuyJLJhKYRjZX0sSkBF17ea0BkhfM1jYaAlxn6ABuLEfzW PEclFkyGsvlFJtQJoragZFBOflv1ZtHgR8J0/UZLpasi08kMfLek4jH62nsulzht WmX7+sKXVueC9MAwFqvGM5/VH68DySQqVO39PkFKCFiHObiy4MIl8iCjjqsPaCgd tEbxAhUbGPJ/XJyuW1mGS2bLVDSGak6KV4Ux7O66ewJi9FaEqtYwNZSWFzweKUrf 50DxvDu0MilJlPwdQXOKfpTIJlR3Nq8ro9A== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvkedrtddvgddvjecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpefhvfevufffkfgjfhgggfgtsehtqhertddttdejnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepteduhffffeeukeetuedvgfdtiedtiedvuddtvddtffdugeffuddt vdehhedukedtnecuffhomhgrihhnpegtuhgurghsvhgtrdgtohhmpdguphgukhdrohhrgh enucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhho mhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Fri, 29 Sep 2023 06:23:50 -0400 (EDT) From: Thomas Monjalon To: Christian Koue Muf , Morten =?ISO-8859-1?Q?Br=F8rup?= , Mykola Kostenok , Ferruh Yigit Cc: "dev@dpdk.org" , "andrew.rybchenko@oktetlabs.ru" , "techboard@dpdk.org" Subject: Re: [PATCH v16 1/8] net/ntnic: initial commit which adds register defines Date: Fri, 29 Sep 2023 12:23:49 +0200 Message-ID: <4244980.IobQ9Gjlxr@thomas> In-Reply-To: <4ba561dd-0fda-4132-a909-bbbb9da80919@amd.com> References: <20230816132552.2483752-1-mko-plv@napatech.com> <4ba561dd-0fda-4132-a909-bbbb9da80919@amd.com> MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="UTF-8" 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 29/09/2023 11:46, Ferruh Yigit: > On 9/29/2023 10:21 AM, Christian Koue Muf wrote: > > On 9/21/2023 4:05 PM, Ferruh Yigit wrote: > >> On 9/20/2023 2:17 PM, Thomas Monjalon wrote: > >>> Hello, > >>> > >>> 19/09/2023 11:06, Christian Koue Muf: > >>>> On 9/18/23 10:34 AM, Ferruh Yigit wrote: > >>>>> On 9/15/2023 7:37 PM, Morten Br=C3=B8rup wrote: > >>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@amd.com] > >>>>>>> Sent: Friday, 15 September 2023 17.55 > >>>>>>> > >>>>>>> On 9/8/2023 5:07 PM, Mykola Kostenok wrote: > >>>>>>>> From: Christian Koue Muf > >>>>>>>> > >>>>>>>> The NTNIC PMD does not rely on a kernel space Napatech driver, > >>>>>>>> thus all defines related to the register layout is part of the > >>>>>>>> PMD code, which will be added in later commits. > >>>>>>>> > >>>>>>>> Signed-off-by: Christian Koue Muf > >>>>>>>> Reviewed-by: Mykola Kostenok > >>>>>>>> > >>>>>>> > >>>>>>> Hi Mykola, Christiam, > >>>>>>> > >>>>>>> This PMD scares me, overall it is a big drop: > >>>>>>> "249 files changed, 87128 insertions(+)" > >>>>>>> > >>>>>>> I think it is not possible to review all in one release cycle, and > >>>>>>> it is not even possible to say if all code used or not. > >>>>>>> > >>>>>>> I can see code is already developed, and it is difficult to > >>>>>>> restructure developed code, but restructure it into small pieces > >>>>>>> really helps for reviews. > >>>>>>> > >>>>>>> > >>>>>>> Driver supports good list of features, can it be possible to > >>>>>>> distribute upstream effort into multiple release. > >>>>>>> Starting from basic functionality and add features gradually. > >>>>>>> Target for this release can be providing datapath, and add more if > >>>>>>> we have time in the release, what do you think? > >>> > >>> I was expecting to get only Rx/Tx in this release, not really more. > >>> > >>> I agree it may be interesting to discuss some design and check whether > >>> we need more features in ethdev as part of the driver upstreaming > >>> process. > >>> > >>> > >>>>>>> Also there are large amount of base code (HAL / FPGA code), > >>>>>>> instead of adding them as a bulk, relevant ones with a feature can > >>>>>>> be added with the feature patch, this eliminates dead code in the > >>>>>>> base code layer, also helps user/review to understand the link > >>>>>>> between driver code and base code. > >>> > >>> Yes it would be interesting to see what is really needed for the basic > >>> initialization and what is linked to a specific offload or configurat= ion feature. > >>> > >>> As a maintainer, I have to do some changes across all drivers > >>> sometimes, and I use git blame a lot to understand why something was = added. > >>> > >>> > >>>>>> Jumping in here with an opinion about welcoming new NIC vendors to= the community: > >>>>>> > >>>>>> Generally, if a NIC vendor supplies a PMD for their NIC, I expect = the vendor to take responsibility for the quality of the PMD, including pro= viding a maintainer and support backporting of fixes to the PMD in LTS rele= ases. This should align with the vendor's business case for upstreaming the= ir driver. > >>>>>> > >>>>>> If the vendor provides one big patch series, which may be difficul= t to understand/review, the fallout mainly hits the vendor's customers (and= thus the vendor's support organization), not the community as a whole. > >>>>>> > >>>>> > >>>>> Hi Morten, > >>>>> > >>>>> I was thinking same before making my above comment, what happens if= vendors submit as one big patch and when a problem occurs we can ask owner= to fix. Probably this makes vendor happy and makes my life (or any other m= aintainer's life) easier, it is always easier to say yes. > >>>>> > >>>>> > >>>>> But I come up with two main reasons to ask for a rework: > >>>>> > >>>>> 1- Technically any vendor can deliver their software to their > >>>>> customers via a public git repository, they don't have to upstream > >>>>> to > >>>>> https://linkprotect.cudasvc.com/url?a=3Dhttps%3a%2f%2fdpdk.org&c=3D= E,1,N > >>>>> poJejuuvPdOPfcFJYtsmkQF6PVrDjGsZ8x_gi5xDrTyZokK_nM11u4ZpzHgM10J9bOLl > >>>>> nhoR6fFAzWtCzOhRCzVruYj520zZORv6-MjJeSC5TrGnIFL&typo=3D1, > >>>>> but upstreaming has many benefits. > >>>>> > >>>>> One of those benefits is upstreaming provides a quality assurance f= or vendor's customers (that is why customer can be asking for this, as we a= re having in many cases), and this quality assurance comes from additional = eyes reviewing the code and guiding vendors for the DPDK quality standards = (some vendors already doing pretty good, but new ones sometimes requires ha= nd-holding). > >>>>> > >>>>> If driver is one big patch series, it is practically not possible t= o review it, I can catch a few bits here or there, you may some others, but= practically it will be merged without review, and we will fail on our qual= ity assurance task. > >>>>> > >>>>> 2- Make code more accessible to the rest of the world. > >>>>> > >>>>> When it is a big patch, code can be functional but lots of details,= reasoning, relation between components gets lost, which makes it even hard= er for an external developer, like me, to understand it (I am a mere guinea= pig here :). > >>>>> > >>>>> If a customer would like to add a feature themselves, or fix someth= ing, even after vendor no more working on that product anymore, customer ne= eds to understand the code or some reasoning in the code. > >>>>> Or if someone wants to backport the driver to rust, or a DPDK devel= oper wants to do a rework that requires updating all drivers, or a tester w= ould like to analyze the code to figure out behavior difference of the devi= ces. I think I have witness all above cases in real life. > >>>>> > >>>>> If driver is split into more patches, it makes patch easier to unde= rstand which makes code practically more accessible to other developers tha= t are not expert in driver. > >>> > >>> I fully agree about the 2 reasons for upstreaming piece by piece. > >>> > >>> > >>>>> Overall, yes splitting patch takes time and effort, and yes this is= an overhead for a code that is already developed, but I think benefit is b= ig so it worth doing the task. > >>> > >>> In the meantime, if some features are not yet upstreamed in a release, > >>> a user can apply the missing patches from the mailing list to get the= features. > >>> > >>> > >>>>>> We, the community, should not make it too difficult for vendors tr= ying to upstream their drivers. I certainly consider it unreasonable to ask= a vendor to postpone the release of some existing features by effectively = an entire year (considering that only LTS releases are relevant for most of= us) because we want the vendor to refactor the patch series to match our p= references within an unrealistic timeframe. > >>> > >>> You're right Morten, we try to be as welcoming as possible, but as > >>> Ferruh said, we want to be able to understand how a driver is built, > >>> even if not understanding all details. > >>> > >>> In Open Source, I think not only the code should be available, we must > >>> also take care of explanations and documentation. > >>> > >>> > >>>>> Agree to not make upstreaming difficult for new vendors, and indeed= we are encouraging more vendors to be upstream their code, this is in best= interest of both sides. > >>>>> > >>>>> Distributing upstreaming effort to a year was just a suggestion, it= can go in earlier as it is becomes ready but I can see it will take time t= o split driver into features and upstream them. > >>> > >>> Driver features can be added until -rc2 (in one month). > >>> > >>> > >>>>> As I am from a vendor too, I can understand the product/customer pr= essure, but I hope this approach can encourage vendors start upstreaming ea= rly or even better upstream as they develop the code. > >>>> > >>>> Hi Ferruh, > >>>> > >>>> First of all, thank you for starting the work to review our code. > >>>> > >>>> As Morten said Napatech plans to take all responsibility for the > >>>> quality of the PMD source code. We expect to provide all fixes needed > >>>> in the future. If for some reason Napatech stops maintaining the > >>>> code, then we have been informed that the DPDK community might delete > >>>> the PMD from the repository, and we understand that. > >>>> > >>>> In regards to splitting the code, I don't see this a good option. > >>>> While I of cause agree it would be easier to review and understand, > >>>> the code should also result in a meaningful product. Of the 87k lines > >>>> of code, 53k lines is needed to start-up the FPGA to a state the it > >>>> is ready to receive traffic. But at this point all packets would > >>>> simply be discarded, and to be honest, there are better and cheaper > >>>> options out there, if nothing more than basic functionality is > >>>> needed. 34k lines are used to setup filters based on rte_flow. The > >>>> thing is, that you need to initialize all modules in the FPGA TX- and > >>>> RX-pipelines with valid data, even if you don't need the features th= ose modules provide. > >>>> As a result, if you split up the 34k lines, then the product would > >>>> not be functional. Of cause some of the top level logic could be > >>>> split out, but at this point we are talking about splitting 87k lines > >>>> into 80k and 7k, which I don't think is worth it. > >>> > >>> Actually I think it is worth. > >>> There is a benefit in isolating the small basic init part from the > >>> more complex features. > >>> > >>> > >>>>>>> As far as I understand last patch opens a socket interface and an > >>>>>>> external application can sent control commands via this interface. > >>>>>>> I am not sure about this side control channel, what is missing in > >>>>>>> the DPDK API? Can we try to address them in the DPDK layer instead > >>>>>>> of a driver specific solution? > >>>>>> > >>>>>> That would be great. > >>>>>> > >>>>>> AFAIK, other vendors also has a bunch of out-of-band communication, > >>>>>> e.g. magical EAL parameters to the MLX drivers. So let's not be too > >>>>>> hard on the newcomers. ;-) > >>>>>> > >>>>> > >>>>> I did some thinking for this one too, > >>>>> > >>>>> As we are in userspace, it is easy to have side control channel, an= d this can make users life easy, so this is a practical thing to do. > >>>>> (Indeed there are already some ways to do this, without PMD exposing > >>>>> a socket interface.) > >>>>> > >>>>> But this also reduces effort developers putting on DPDK layer solut= ion, because it is always easier to add more support to the driver only. > >>>>> And overall this reduces portability of the DPDK application, each > >>>>> application becomes unique to a device (This is a bad thing, but I > >>>>> also need some feedback how bad it is in real life.) > >>>>> > >>>>> To balance this, we said if a feature is too specific to a device, = it can add device specific API and this is better than device specific feat= ures pollute the common, most used code. And push back to introduce more ne= w PMD specific APIs unless it is really needed. > >>>>> > >>>>> But creating a socket interface directly from the driver is more th= an PMD specific API. Technically application control interface can rely com= pletely to this. Even we assume this is not for control, but just for debug= , I can see it can be useful for debug and again practical thing to do, I a= m still not sure how much it hurts if each driver has a custom socket inter= face for their debug needs. > >>>>> > >>>>> Overall it makes more sense to me to have a unified/common interfac= e from drivers to DPDK applications, which is through the ethdev layer. > >>>>> And improve and extend the ethdev layer to satisfy driver needs. > >>>>> > >>>>> In this specific example, I am for rejecting the socket interface p= atch, but I would like to get more feedback from @techboard. > >>>>> > >>>> > >>>> The reason we have the addition control channel is not provide > >>>> additional functionality. We have customers with use-cases that > >>>> require multiple processes. Since Napatech adapters do not support > >>>> configuration through VFs, then secondary applications must send > >>>> their rte_flow to a main application, which will then setup the flow > >>>> through it's PF. This control channel "hides" these details, and make > >>>> the product easier for users to adapt to their existing solutions. > >>> > >>> I think you need to explore VF representors. > >>> This is what is done with other drivers, and it make them compatible. > >>> > >>>> If you stand firm on rejecting the control channel, then we have to > >>>> go back to the drawing board on this issue. We did look at DPDK's > >>>> multi-process support, and actually had some support for this, but we > >>>> determined that for our use-case it was better to have a > >>>> communication channel, and no shared memory. > >>> > >>> I'm not sure your need is about secondary process. > >>> Let's discuss this need in a meeting if needed. > >>> Anyway, the message is that we want to be part of such design decisio= n. > >>> > >>> > >>>>> And related to not being too hard on the newcomers, unrelated to be= ing a newcomer or not, if a process/feature/approach approved once, some ot= hers will point to it and will ask to do the same which is fair in their pe= rspective. I had multiple instance of this in the past. > >>>>> > >>>>> Of course we are being easy to newcomers but not in a way to allow > >>>>> code that we believe is not good thing to do, but going easy on > >>>>> process may be :) > >>>>> > >>>> > >>>> We are grateful for any leniency you may show us ;-) > >>>> > >>>> Thanks again, > >>>> Christian > >>>> > >>>>> > >>>>>>> > >>>>>>> > >>>>>>> Thanks, > >>>>>>> ferruh > >>>>>> > >>>>>> Thank you, Ferruh, for taking good care of the community by provid= ing constructive feedback like this to new NIC vendors! > >>>>>> > >>>>>> Please note that my feedback is entirely process related. I didn= =E2=80=99t review the driver, so I have no technical comments to the patch = series. > >>>>>> > >>>>>> -Morten > >>> > >>> > >>> We are going to discuss the process in the technical board today. > >>> > >>> > >> > >> Hi Mykola, Christiam, > >> > >> As discussed, following are a few good examples from the DPDK history,= there is no "fits all, fixed guidelines", but they can serve as samples: > >> > >> Marvell cnxk: > >> https://linkprotect.cudasvc.com/url?a=3Dhttps%3a%2f%2fpatchwork.dpdk.o= rg%2fproject%2fdpdk%2flist%2f%3fseries%3d17449%26state%3d%252A%26archive%3d= both&c=3DE,1,DmXU0iHwXoSaZ4bKn-yhX9J8XmFBispd2ut7pxLNBkK3Q4LVpG_zmOf1jnWSS-= Y0Fx-TNbPnQDHyBZkDj23Gu7zjPZ5nsA7pid5CsE2vxNk,&typo=3D1 > >> > >> > >> Solarflare sfc (before patchwork series support): > >> https://linkprotect.cudasvc.com/url?a=3Dhttps%3a%2f%2fpatchwork.dpdk.o= rg%2fproject%2fdpdk%2fpatch%2f1480436367-20749-2-git-send-email-arybchenko%= 40solarflare.com%2f&c=3DE,1,E9oUT_1WuNC2JA8x7an3rC_Pm5g1L5cxJKQ6pTwSbCWSJpi= LH2GnmgfFkUqViOOwkpS2df8kgBvHjmulKaWhyr4BBizUT-sL5LJv21Hx4RtHtK3vjhcKpg,,&t= ypo=3D1 > >> to > >> https://linkprotect.cudasvc.com/url?a=3Dhttps%3a%2f%2fpatchwork.dpdk.o= rg%2fproject%2fdpdk%2fpatch%2f1480436367-20749-56-git-send-email-arybchenko= %40solarflare.com%2f&c=3DE,1,GByF_TiC_q11iVPpiPgpCMlSge-J0XfT0zHkriK0rde1Qt= 1RG7uf6mETQkTSQ-1V86Z7EtRcxlvSsed1sqn8RWfN8KFSbd7NaAkfbDiehn_vSRzja45rQgv53= Q,,&typo=3D1 > >> > >> > >> Intel ice: > >> https://linkprotect.cudasvc.com/url?a=3Dhttps%3a%2f%2fpatchwork.dpdk.o= rg%2fproject%2fdpdk%2flist%2f%3fseries%3d2842%26state%3d%252A%26archive%3db= oth&c=3DE,1,zQwvAIR3ToLIhT09bVxm_HEF-dp8eyTqhsKB3eOYgIJdd2WS_0ZlTbQKfr9KLyT= A3A2A2HzBbjIlz21D_hWVgS_INmmC5eew1J0QBH-PoRNd&typo=3D1 > >> > >=20 > > Thank you for the links, they have been very helpful. > >=20 > > After a lot of internal discussion, Napatech has decided to implement s= ome architectural changes to our PMD that will allow us to easier split up = the code into smaller features. The work will require some time, which mean= s that we will not be ready for the 23.11 release. The current goal is to a= ttempt to upstream a quite basic PMD in time for 24.7, and a fully featured= PMD for 24.11. > >=20 > >=20 >=20 > Hi Christiam, >=20 > Good to see there is a solid plan for upstreaming but also not that good > that it is postponed, >=20 > I am aware it is all tied to your internal planning/resourcing etc, but > since the effort already started, can it be possible to squeeze very > basic driver in this release, which just does link up and most basic Rx/T= x? > It gives opportunity to experiment on device to users. >=20 > We can accept it up to -rc3, which is end of October, so there is still > some time? >=20 > This is just a suggestion though, no pressure intended. I agree with Ferruh, better to start early and small. It shouldn't be too hard to introduce the skeleton of the driver.