Message-ID: <CAKgT0UcssuWvqYoVP7rkcCVnFA3zHkDaqe6Tp7=izFx0BH=2Zw@mail.gmail.com>
Date: Wed, 17 Apr 2024 09:09:02 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Leon Romanovsky <leon@...nel.org>
Cc: Alexander Lobakin <aleksander.lobakin@...el.com>, Jakub Kicinski <kuba@...nel.org>, 
	Yunsheng Lin <linyunsheng@...wei.com>, netdev@...r.kernel.org, 
	Alexander Duyck <alexanderduyck@...com>, davem@...emloft.net, pabeni@...hat.com
Subject: Re: [net-next PATCH 13/15] eth: fbnic: add basic Rx handling

On Wed, Apr 17, 2024 at 1:14 AM Leon Romanovsky <leon@...nel.org> wrote:
>
> On Tue, Apr 16, 2024 at 07:46:06AM -0700, Alexander Duyck wrote:
> > On Tue, Apr 16, 2024 at 7:05 AM Alexander Lobakin
> > <aleksander.lobakin@...el.com> wrote:
> > >
> > > From: Alexander Duyck <alexander.duyck@...il.com>
> > > Date: Mon, 15 Apr 2024 11:03:13 -0700
> > >
> > > > On Mon, Apr 15, 2024 at 10:11 AM Jakub Kicinski <kuba@...nel.org> wrote:
> > > >>
> > > >> On Mon, 15 Apr 2024 08:03:38 -0700 Alexander Duyck wrote:
> > > >>>>> The advantage of being a purpose built driver is that we aren't
> > > >>>>> running on any architectures where the PAGE_SIZE > 4K. If it came to
> > > >>>>
> > > >>>> I am not sure if 'being a purpose built driver' argument is strong enough
> > > >>>> here, at least the Kconfig does not seems to be suggesting it is a purpose
> > > >>>> built driver, perhaps add a 'depend on' to suggest that?
> > > >>>
> > > >>> I'm not sure if you have been following the other threads. One of the
> > > >>> general thoughts of pushback against this driver was that Meta is
> > > >>> currently the only company that will have possession of this NIC. As
> > > >>> such Meta will be deciding what systems it goes into and as a result
> > > >>> of that we aren't likely to be running it on systems with 64K pages.
> > > >>
> > > >> Didn't take long for this argument to float to the surface..
> > > >
> > > > This wasn't my full argument. You truncated the part where I
> > > > specifically called out that it is hard to justify us pushing a
> > > > proprietary API that is only used by our driver.
> > > >
> > > >> We tried to write some rules with Paolo but haven't published them, yet.
> > > >> Here is one that may be relevant:
> > > >>
> > > >>   3. External contributions
> > > >>   -------------------------
> > > >>
> > > >>   Owners of drivers for private devices must not exhibit a stronger
> > > >>   sense of ownership or push back on accepting code changes from
> > > >>   members of the community. 3rd party contributions should be evaluated
> > > >>   and eventually accepted, or challenged only on technical arguments
> > > >>   based on the code itself. In particular, the argument that the owner
> > > >>   is the only user and therefore knows best should not be used.
> > > >>
> > > >> Not exactly a contribution, but we predicted the "we know best"
> > > >> tone of the argument :(
> > > >
> > > > The "we know best" is more of an "I know best" as someone who has
> > > > worked with page pool and the page fragment API since well before it
> > > > existed. My push back is based on the fact that we don't want to
> > >
> > > I still strongly believe Jesper-style arguments like "I've been working
> > > with this for aeons", "I invented the Internet", "I was born 3 decades
> > > before this API was introduced" are not valid arguments.
> >
> > Sorry that is a bit of my frustration with Yunsheng coming through. He
> > has another patch set that mostly just moves my code and made himself
> > the maintainer. Admittedly I am a bit annoyed with that. Especially
> > since the main drive seems to be to force everything to use that one
> > approach and then optimize for his use case for vhost net over all
> > others most likely at the expense of everything else.
> >
> > It seems like it is the very thing we were complaining about in patch
> > 0 with other drivers getting penalized at the cost of optimizing for
> > one specific driver.
> >
> > > > allocate fragments, we want to allocate pages and fragment them
> > > > ourselves after the fact. As such it doesn't make much sense to add an
> > > > API that will have us trying to use the page fragment API which holds
> > > > onto the page when the expectation is that we will take the whole
> > > > thing and just fragment it ourselves.
> > >
> > > [...]
> > >
> > > Re "this HW works only on x86, why bother" -- I still believe there
> > > shouldn't be any hardcodes in any driver based on the fact that the HW
> > > is deployed only on particular systems. Page sizes, Endianness,
> > > 32/64-bit... It's not difficult to make a driver look like it's
> > > universal and could work anywhere, really.
> >
> > It isn't that this only works on x86. It is that we can only test it
> > on x86. The biggest issue right now is that I wouldn't have any
> > systems w/ 64K pages that I could test on.
>
> Didn't you write that you will provide QEMU emulation for this device?
>
> Thanks

Yes. I had already mentioned the possibility of testing it this way. I
am just not sure it adds much value to test the already limited
hardware and a limited platform setup in emulation. The issue is that
it will be hard to generate any stress in the QEMU environment since
it maxes out at only about 1 or 2 Gbps as the overhead for providing
TCAMs is software is not insignificant. To top it off, emulating a
non-native architecture will slow things down further. It would be
like asking someone to test a 100G nic on a system that only has PCIe
gen1.

I will probably just go the suggested route of enabling compile
testing on all platforms, and only support loading it on X86_64. As
time permits I can probably assign somebody the job of exploring the
page size larger than 4K issue, however it will be a matter of
weighing the trade-off for adding technical debt for a use case that
may not be applicable.

Powered by blists - more mailing lists