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_nM11u4ZpzHgM10J9bOLl >>>>> 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%26archive%3dboth&c=E,1,DmXU0iHwXoSaZ4bKn-yhX9J8XmFBispd2ut7pxLNBkK3Q4LVpG_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-arybchenko%40solarflare.com%2f&c=E,1,E9oUT_1WuNC2JA8x7an3rC_Pm5g1L5cxJKQ6pTwSbCWSJpiLH2GnmgfFkUqViOOwkpS2df8kgBvHjmulKaWhyr4BBizUT-sL5LJv21Hx4RtHtK3vjhcKpg,,&typo=1 >> to >> https://linkprotect.cudasvc.com/url?a=https%3a%2f%2fpatchwork.dpdk.org%2fproject%2fdpdk%2fpatch%2f1480436367-20749-56-git-send-email-arybchenko%40solarflare.com%2f&c=E,1,GByF_TiC_q11iVPpiPgpCMlSge-J0XfT0zHkriK0rde1Qt1RG7uf6mETQkTSQ-1V86Z7EtRcxlvSsed1sqn8RWfN8KFSbd7NaAkfbDiehn_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%26archive%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.
