Re: [net-next PATCH 13/15] eth: fbnic: add basic Rx handling

[Leon Romanovsky <leon@kernel.org>]

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@intel.com> wrote:
> >
> > From: Alexander Duyck <alexander.duyck@gmail.com>
> > Date: Mon, 15 Apr 2024 11:03:13 -0700
> >
> > > On Mon, Apr 15, 2024 at 10:11 AM Jakub Kicinski <kuba@kernel.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