Message-ID: <673cab54db1c1_2a097e2948c@willemb.c.googlers.com.notmuch>
Date: Tue, 19 Nov 2024 10:14:28 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>, 
 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)

Alexander Lobakin wrote:
> 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.

Smaller focused series might have been merged already.
 
> 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)

This is a misunderstanding. I need a working feature, on a predictable
timeline, in distro kernels.

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

Not as written, no, obviously.

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

Opinionated positions. Nice if you have unlimited time.

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

Powered by blists - more mailing lists