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ørup wrote: > >>> From: Ferruh Yigit [mailto:ferruh.yi...@amd.com] > >>> Sent: Friday, 15 September 2023 17.55 > >>> > >>> On 9/8/2023 5:07 PM, Mykola Kostenok wrote: > >>>> From: Christian Koue Muf <c...@napatech.com> > >>>> > >>>> 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 <c...@napatech.com> > >>>> Reviewed-by: Mykola Kostenok <mko-...@napatech.com> > >>>> > >>> > >>> 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 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: > >> > >> 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 > >> providing a maintainer and support backporting of fixes to the PMD in LTS > >> releases. This should align with the vendor's business case for > >> upstreaming their driver. > >> > >> If the vendor provides one big patch series, which may be difficult 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 > >maintainer'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=https%3a%2f%2fdpdk.org&c=E,1,NpoJejuuvPdOPfcFJYtsmkQF6PVrDjGsZ8x_gi5xDrTyZokK_nM11u4ZpzHgM10J9bOLlnhoR6fFAzWtCzOhRCzVruYj520zZORv6-MjJeSC5TrGnIFL&typo=1, > >but upstreaming has many benefits. > > > >One of those benefits is upstreaming provides a quality assurance for > >vendor's customers (that is why customer can be asking for this, as we are > >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 > >hand-holding). > > > >If driver is one big patch series, it is practically not possible to 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 > >quality 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 harder > >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 something, even > >after vendor no more working on that product anymore, customer needs to > >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 understand > >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 > >overhead 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 features. > >> We, the community, should not make it too difficult for vendors trying 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 > >> preferences 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 to 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 pressure, > >but I hope this approach can encourage vendors start upstreaming early 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 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 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, and 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 solution, > >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 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 PMD > >specific API. Technically application control interface can rely completely > >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 am still > >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 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 patch, 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 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 > >perspective. 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 providing > >> constructive feedback like this to new NIC vendors! > >> > >> Please note that my feedback is entirely process related. I didn’t 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.
