Message-ID: <CAKgT0UfyAQaPKApZoV6YJhMPAac3q3KBN4yHdF0j48mKZopsBw@mail.gmail.com>
Date: Tue, 16 Apr 2024 07:46:06 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: 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 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, and the worst that could
happen based on the current code is that the NIC driver will be a
memory hog.

I would much prefer the potential of being a memory hog on an untested
hardware over implementing said code untested and introducing
something like a memory leak or page fault issue.

That is why I am more than willing to make this an x86_64 only driver
for now and we can look at expanding out as I get time and get
equipment to to add support and test for other architectures.

Powered by blists - more mailing lists