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.
