netdev - Re: [PATCH net-next v5 00/19] xdp: a fistful of generic changes (+libeth

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: <52650a34-f9f9-4769-8d16-01f549954ddf@intel.com>
Date: Tue, 19 Nov 2024 13:28:29 +0100
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC: Jakub Kicinski <kuba@...nel.org>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	Toke Høiland-Jørgensen <toke@...hat.com>, "Alexei
 Starovoitov" <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, "John
 Fastabend" <john.fastabend@...il.com>, Andrii Nakryiko <andrii@...nel.org>,
	Maciej Fijalkowski <maciej.fijalkowski@...el.com>, Stanislav Fomichev
	<sdf@...ichev.me>, Magnus Karlsson <magnus.karlsson@...el.com>,
	<nex.sw.ncis.osdt.itp.upstreaming@...el.com>, <bpf@...r.kernel.org>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v5 00/19] xdp: a fistful of generic changes
 (+libeth_xdp)

From: Willem De Bruijn <willemdebruijn.kernel@...il.com>
Date: Sat, 16 Nov 2024 10:31:08 -0500

> Jakub Kicinski wrote:
>> On Wed, 13 Nov 2024 16:24:23 +0100 Alexander Lobakin wrote:
>>> Part III does the following:
>>> * does some cleanups with marking read-only bpf_prog and xdp_buff
>>>   arguments const for some generic functions;
>>> * allows attaching already registered XDP memory model to Rxq info;
>>> * allows mixing pages from several Page Pools within one XDP frame;
>>> * optimizes &xdp_frame structure and removes no-more-used field;
>>> * adds generic functions to build skbs from xdp_buffs (regular and
>>>   XSk) and attach frags to xdp_buffs (regular and XSk);
>>> * adds helper to optimize XSk xmit in drivers;
>>> * extends libeth Rx to support XDP requirements (headroom etc.) on Rx;
>>> * adds libeth_xdp -- libeth module with common XDP and XSk routines.
>>
>> This clearly could be multiple series, please don't go over the limit.
> 
> Targeting different subsystems and thus reviewers. The XDP, page_pool
> and AF_XDP changes might move faster on their own.

Reviewers for page_pool, XDP and XSk (no idea why everyone name it
AF_XDP) are 90% time the same people.
Often times, you can't avoid cross-subsystem patches. These three are
closely tied to each other.

> 
> If pulling those out into separate series, that also allows splitting
> up the last patch. That weighs in at 3481 LoC, out of 4400 for the
> series.

1500 of which is kdoc if you read the cover letter.

libeth_xdp depends on every patch from the series. I don't know why you
believe this might anyhow move faster. Almost the whole series got
reviewed relatively quickly, except drivers/intel folder which people
often tend to avoid.

I remind you that the initial libeth + iavf series (11 patches) was
baking on LKML for one year. Here 2 Chapters went into the kernel within
2 windows and only this one (clearly much bigger than the previous ones
and containing only generic changes in contrary to the previous which
had only /intel code) didn't follow this rule, which doesn't
unnecessarily mean it will stuck for too long.

(+ I clearly mentioned several times that Chapter III will take longer
 than the rest and each time you had no issues with that)

> 
> The first 3 patches are not essential to IDFP XDP + AF_XDP either.

You don't seem to read the code. libeth_xdp won't even build without them.
I don't believe the model taken by some developers (not spelling names
loud) "let's submit minimal changes and almost draft code, I promise
I'll create a todo list and will be polishing it within next x years"
works at all, not speaking that it may work better than sending polished
mature code (I hope it is).

> The IDPF feature does not have to not depend on them.
> 
> Does not matter for upstream, but for the purpose of backporting this
> to distro kernels, it helps if the driver feature minimizes dependency
> on core kernel API changes. If patch 19 can be made to work without

OOT style of thinking.
Minimizing core changes == artificial self-limiting optimization and
functionality potential.
New kernels > LTSes and especially custom kernels which receive
non-upstream (== not officially supported by the community) feature
backports. Upstream shouldn't sacrifice anything in favor of those, this
way we end up one day sacrificing stuff for out-of-tree drivers (which I
know some people already try to do).

> some of the changes in 1..18, that makes it more robust from that PoV.

No it can't, I thought people first read the code and only then comment,
otherwise it's just wasting time.

Thanks,
Olek