On 10/9/2023 8:57 AM, Christian Koue Muf wrote: > On 9/29/2023 12:24 PM, Thomas Monjalon wrote: >> 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ø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,N >>>>>>>> poJejuuvPdOPfcFJYtsmkQF6PVrDjGsZ8x_gi5xDrTyZokK_nM11u4ZpzHgM10J9 >>>>>>>> bOLl nhoR6fFAzWtCzOhRCzVruYj520zZORv6-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://linkprotect.cudasvc.com/url?a=https%3a%2f%2fpatchwork.dpdk. >>>>> org%2fproject%2fdpdk%2flist%2f%3fseries%3d17449%26state%3d%252A%26a >>>>> rchive%3dboth&c=E,1,DmXU0iHwXoSaZ4bKn-yhX9J8XmFBispd2ut7pxLNBkK3Q4L >>>>> VpG_zmOf1jnWSS-Y0Fx-TNbPnQDHyBZkDj23Gu7zjPZ5nsA7pid5CsE2vxNk,&typo= >>>>> 1 >>>>> >>>>> >>>>> Solarflare sfc (before patchwork series support): >>>>> https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fpatchwork.dpdk. >>>>> org%2fproject%2fdpdk%2fpatch%2f1480436367-20749-2-git-send-email-ar >>>>> ybchenko%40solarflare.com%2f&c=E,1,E9oUT_1WuNC2JA8x7an3rC_Pm5g1L5cx >>>>> JKQ6pTwSbCWSJpiLH2GnmgfFkUqViOOwkpS2df8kgBvHjmulKaWhyr4BBizUT-sL5LJ >>>>> v21Hx4RtHtK3vjhcKpg,,&typo=1 >>>>> to >>>>> https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fpatchwork.dpdk. >>>>> org%2fproject%2fdpdk%2fpatch%2f1480436367-20749-56-git-send-email-a >>>>> rybchenko%40solarflare.com%2f&c=E,1,GByF_TiC_q11iVPpiPgpCMlSge-J0Xf >>>>> T0zHkriK0rde1Qt1RG7uf6mETQkTSQ-1V86Z7EtRcxlvSsed1sqn8RWfN8KFSbd7NaA >>>>> kfbDiehn_vSRzja45rQgv53Q,,&typo=1 >>>>> >>>>> >>>>> Intel ice: >>>>> https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fpatchwork.dpdk. >>>>> org%2fproject%2fdpdk%2flist%2f%3fseries%3d2842%26state%3d%252A%26ar >>>>> chive%3dboth&c=E,1,zQwvAIR3ToLIhT09bVxm_HEF-dp8eyTqhsKB3eOYgIJdd2WS >>>>> _0ZlTbQKfr9KLyTA3A2A2HzBbjIlz21D_hWVgS_INmmC5eew1J0QBH-PoRNd&typo=1 >>>>> >>>> >>>> Thank you for the links, they have been very helpful. >>>> >>>> After a lot of internal discussion, Napatech has decided to implement some >>>> 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 means >>>> that we will not be ready for the 23.11 release. The current goal is to >>>> attempt to upstream a quite basic PMD in time for 24.7, and a fully >>>> featured PMD for 24.11. >>>> >>>> >>> >>> Hi Christiam, >>> >>> Good to see there is a solid plan for upstreaming but also not that >>> good that it is postponed, >>> >>> 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/Tx? >>> It gives opportunity to experiment on device to users. >>> >>> We can accept it up to -rc3, which is end of October, so there is >>> still some time? >>> >>> 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. > > Hi Ferruh and Thomas, > > My apologies for the late response, I have been sick the last week. > > We can try to create a small PMD in time. The reason I'm cautious is because > Napatech plan to make quite large changes to the PMD, to achieve a more > stable and modular code-base. This means that future updates will have quite > large diffs, until these changes are in place. > >
Hi Christian, Mykola, What is the status of the 'ntnic'? Will there be some upstreaming effort for v24.07?
