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 39A87425DD; Wed, 20 Sep 2023 15:17:12 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 27B51410D3; Wed, 20 Sep 2023 15:17:12 +0200 (CEST) Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by mails.dpdk.org (Postfix) with ESMTP id 5219A402F2; Wed, 20 Sep 2023 15:17:11 +0200 (CEST) Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailout.nyi.internal (Postfix) with ESMTP id 614325C00C0; Wed, 20 Sep 2023 09:17:08 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute5.internal (MEProxy); Wed, 20 Sep 2023 09:17:08 -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= 1695215828; x=1695302228; bh=EfLTe9mckLepRkKASw/kNZtQQuR72VST4OM dUe+R/XY=; b=QdkrG2iUW9Dq4VCgoFlVMre0kQklVHf9JgoG9f7oPIIafE+EYeB gkAQBstpYz1U02Y3iHTFrtk5cGcD+sWMyYLWfB1jIN66DI/eXB9BPKkXbclop/NP 8VcFp+OXRl2TSR3K7+TU/+qjRNVHCnj8pvK/mzvRpBk42eW1swEey0KmpeZnkhzo CtLhzkjZ7qcqMVvdWSSzabiw3BY7iprV+lFxHpZK/dK1D58FZXWoDkHot23NFCl5 GpTd+Ylbyxsy9XzBGCJmWewJPh5oxHm53Ew+2/LhTxNiRRIGGgOJSBHLQan0VoLg UTvqoF60aioRdP4HfETAqIMwYF1UCO9Nm8g== 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= 1695215828; x=1695302228; bh=EfLTe9mckLepRkKASw/kNZtQQuR72VST4OM dUe+R/XY=; b=TB32YMtvwCnBSvXW8jjo6BuvMt+vQHDJkQu+PbYafm86zvHzO2t RgQh0wRa+InJSyaJBHZXqC/9xlR9B+b9SWzOEKKPYNzZhS8l276mWnFu5O9A3QN2 KAJB41IAdO6YYZSEBB/Fyq8jwTpk4ngT4NKz4dt6oSe0Fki1WkJHEkuNYE0FICwE G0vfX0TKi7aQCYWxZURDbrOC4iQIuY6aqUJ1o55uoDGHPSlqY5KGDEJ40w/AZtUg 0lNOjb3TMAlhzl52vO4/5y0ctUrnv6K6Z9PPkQEja3CSACX3B9lCACQFsME1r1da pYtON6t1SqQolEKRNjGOE4IvRjScXkeUd9Q== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedviedrudekfedgieefucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthhqredttddtjeenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpeetudfhffefueekteeuvdfgtdeitdeivddutddvtdffudegffdu tddvheehudektdenucffohhmrghinheptghuuggrshhvtgdrtghomhdpughpughkrdhorh hgnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepthhh ohhmrghssehmohhnjhgrlhhonhdrnhgvth X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 20 Sep 2023 09:17:05 -0400 (EDT) From: Thomas Monjalon To: Ferruh Yigit , Morten =?ISO-8859-1?Q?Br=F8rup?= , Mykola Kostenok , Christian Koue Muf 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: Wed, 20 Sep 2023 15:17:04 +0200 Message-ID: <4499805.irdbgypaU6@thomas> In-Reply-To: References: <20230816132552.2483752-1-mko-plv@napatech.com> <2362c757-3cf7-4b4b-b507-402781d9a810@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 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= =20 > >>>> all defines related to the register layout is part of the PMD code,= =20 > >>>> 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= =20 > >>> is not even possible to say if all code used or not. > >>> > >>> I can see code is already developed, and it is difficult to=20 > >>> restructure developed code, but restructure it into small pieces=20 > >>> really helps for reviews. > >>> > >>> > >>> Driver supports good list of features, can it be possible to=20 > >>> 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= =20 > >>> 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=20 > >>> of adding them as a bulk, relevant ones with a feature can be added=20 > >>> with the feature patch, this eliminates dead code in the base code=20 > >>> layer, also helps user/review to understand the link between driver=20 > >>> code and base code. Yes it would be interesting to see what is really needed for the basic init= ialization and what is linked to a specific offload or configuration 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: > >>=20 > >> 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 providi= ng a maintainer and support backporting of fixes to the PMD in LTS releases= =2E This should align with the vendor's business case for upstreaming their= driver. > >>=20 > >> If the vendor provides one big patch series, which may be difficult to= understand/review, the fallout mainly hits the vendor's customers (and thu= s the vendor's support organization), not the community as a whole. > >>=20 > > > >Hi Morten, > > > >I was thinking same before making my above comment, what happens if vend= ors submit as one big patch and when a problem occurs we can ask owner to f= ix. Probably this makes vendor happy and makes my life (or any other mainta= iner'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://linkpro= tect.cudasvc.com/url?a=3Dhttps%3a%2f%2fdpdk.org&c=3DE,1,NpoJejuuvPdOPfcFJYt= smkQF6PVrDjGsZ8x_gi5xDrTyZokK_nM11u4ZpzHgM10J9bOLlnhoR6fFAzWtCzOhRCzVruYj52= 0zZORv6-MjJeSC5TrGnIFL&typo=3D1, > >but upstreaming has many benefits. > > > >One of those benefits is upstreaming provides a quality assurance for ve= ndor's customers (that is why customer can be asking for this, as we are ha= ving 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 hand-ho= lding). > > > >If driver is one big patch series, it is practically not possible to rev= iew it, I can catch a few bits here or there, you may some others, but prac= tically it will be merged without review, and we will fail on our quality a= ssurance 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, reas= oning, relation between components gets lost, which makes it even harder fo= r 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 something, = even after vendor no more working on that product anymore, customer needs t= o understand the code or some reasoning in the code. > >Or if someone wants to backport the driver to rust, or a DPDK developer = wants to do a rework that requires updating all drivers, or a tester would = like to analyze the code to figure out behavior difference of the devices. = I think I have witness all above cases in real life. > > > >If driver is split into more patches, it makes patch easier to understan= d which makes code practically more accessible to other developers that 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 o= verhead for a code that is already developed, but I think benefit is big 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 featu= res. > >> We, the community, should not make it too difficult for vendors trying= to upstream their drivers. I certainly consider it unreasonable to ask a v= endor to postpone the release of some existing features by effectively an e= ntire 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 prefe= rences 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 a= re encouraging more vendors to be upstream their code, this is in best inte= rest 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 to spl= it 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 pressur= e, but I hope this approach can encourage vendors start upstreaming early o= r even better upstream as they develop the code. >=20 > Hi Ferruh, >=20 > First of all, thank you for starting the work to review our code. >=20 > 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. >=20 > 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 re= ady > to receive traffic. But at this point all packets would simply be discard= ed, > 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 those 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 7= k, > 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=20 > >>> external application can sent control commands via this interface. > >>> I am not sure about this side control channel, what is missing in the= =20 > >>> DPDK API? Can we try to address them in the DPDK layer instead of a=20 > >>> driver specific solution? > >>=20 > >> That would be great. > >>=20 > >> AFAIK, other vendors also has a bunch of out-of-band communication,=20 > >> e.g. magical EAL parameters to the MLX drivers. So let's not be too=20 > >> hard on the newcomers. ;-) > >>=20 > > > >I did some thinking for this one too, > > > >As we are in userspace, it is easy to have side control channel, and thi= s 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 s= ocket interface.) > > > >But this also reduces effort developers putting on DPDK layer solution, = because it is always easier to add more support to the driver only. > >And overall this reduces portability of the DPDK application, each appli= cation becomes unique to a device (This is a bad thing, but I also need som= e feedback how bad it is in real life.) > > > >To balance this, we said if a feature is too specific to a device, it ca= n add device specific API and this is better than device specific features = pollute the common, most used code. And push back to introduce more new PMD= specific APIs unless it is really needed. > > > >But creating a socket interface directly from the driver is more than PM= D specific API. Technically application control interface can rely complete= ly to this. Even we assume this is not for control, but just for debug, I c= an see it can be useful for debug and again practical thing to do, I am sti= ll not sure how much it hurts if each driver has a custom socket interface = for their debug needs. > > > >Overall it makes more sense to me to have a unified/common interface fro= m 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 patch,= but I would like to get more feedback from @techboard. > > >=20 > 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 decision. > >And related to not being too hard on the newcomers, unrelated to being a= newcomer or not, if a process/feature/approach approved once, some others = will point to it and will ask to do the same which is fair in their perspec= tive. 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 :) > > >=20 > We are grateful for any leniency you may show us ;-) >=20 > Thanks again, > Christian >=20 > > > >>> > >>> > >>> Thanks, > >>> ferruh > >>=20 > >> Thank you, Ferruh, for taking good care of the community by providing = constructive feedback like this to new NIC vendors! > >>=20 > >> 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. > >>=20 > >> -Morten We are going to discuss the process in the technical board today.