Message-ID: <dabd8755-16f6-448d-b054-9245f2f27a67@intel.com>
Date: Wed, 17 Apr 2024 12:39:38 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Alexander Duyck <alexander.duyck@...il.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

From: Alexander Duyck <alexander.duyck@...il.com>
Date: Tue, 16 Apr 2024 07:46:06 -0700

> 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.

I don't see any issue to not limit it to x86_64 only. It compiles just
fine and if you run it later on a non-x86_64 system, you'll test it
then. I don't think anyone will run it on a different platform prior to
that.

Thanks,
Olek

Powered by blists - more mailing lists