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ø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. > >
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://patchwork.dpdk.org/project/dpdk/list/?series=17449&state=%2A&archive=both Solarflare sfc (before patchwork series support): https://patchwork.dpdk.org/project/dpdk/patch/1480436367-20749-2-git-send-email-arybche...@solarflare.com/ to https://patchwork.dpdk.org/project/dpdk/patch/1480436367-20749-56-git-send-email-arybche...@solarflare.com/ Intel ice: https://patchwork.dpdk.org/project/dpdk/list/?series=2842&state=%2A&archive=both
