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_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.
I agree with Ferruh, better to start early and small. It shouldn't be too hard to introduce the skeleton of the driver.
