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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKgT0UcGN3-6R4pt8BQv2hD04oYk48GfFs1O_UGChvrrFT5eCw@mail.gmail.com>
Date: Mon, 15 Apr 2024 11:03:13 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: 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 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
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.

If we are going to use the page fragment API we need the ability to
convert a page pool page to a fragment in place, not have it get
pulled into the page fragment API and then immediately yanked right
back out. On top of that we don't need to be making significant
changes to the API that will slow down all the other users to
accomodate a driver that will not be used by most users.

This is a case where I agree with Jiri. It doesn't make sense to slow
down all the other users of the page fragment API by making it so that
we can pull variable sliced batches from the page pool fragment
interface. It makes much more sense for us to just fragment in place
and add as little overhead to the other users of page pool APIs as
possible.