linux-kernel - Re: [PATCH bpf-next 0/9] bpf: cpumap: enable GRO for XDP

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: <03634fe4-8c2f-432e-ae6e-08928d167b1f@intel.com>
Date: Wed, 4 Sep 2024 17:13:59 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann
	<daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Lorenzo Bianconi
	<lorenzo@...nel.org>, Daniel Xu <dxu@...uu.xyz>, John Fastabend
	<john.fastabend@...il.com>, Jesper Dangaard Brouer <hawk@...nel.org>, "Martin
 KaFai Lau" <martin.lau@...ux.dev>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	<bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next 0/9] bpf: cpumap: enable GRO for XDP_PASS frames

From: Jakub Kicinski <kuba@...nel.org>
Date: Wed, 4 Sep 2024 07:50:41 -0700

> On Wed, 4 Sep 2024 15:13:54 +0200 Alexander Lobakin wrote:
>>> Could you try to use the backlog NAPI? Allocating a fake netdev and
>>> using NAPI as a threading abstraction feels like an abuse. Maybe try
>>> to factor out the necessary bits? What we want is using the per-cpu 
>>> caches, and feeding GRO. None of the IRQ related NAPI functionality
>>> fits in here.  
>>
>> Lorenzo will try as he wrote. I can only add that in my old tree, I
>> factored out GRO bits and used them here just as you wrote. The perf was
>> the same, but the diffstat was several hundred lines only to factor out
>> stuff, while here the actual switch to NAPI removes more lines than
>> adds, also custom kthread logic is gone etc. It just looks way more
>> elegant and simple.
> 
> Once again we seem to be arguing whether lower LoC is equivalent to
> better code? :) If we can use backlog NAPI it hopefully won't be as

And once again I didn't say that explicitly :D When 2 patches work the
same way, but one has far shorter diffstat, we often prefer this one if
it's correct. This one for cpumap looked correct to me and Lorenzo and
we didn't have any other ideas, so I picked it.
I didn't say "it's better than backlog NAPI because it's shorter", the
only thing I said re backlog NAPI is that we'll try it. I didn't think
of this previously at all, I'm no backlog expert in general.

> long. Maybe other, better approaches are within reach, too.
> 
>> I could say that gro_cells also "abuses" NAPI the same way, don't you
>> think?
> 
> "same way"? :] Does it allocate a fake netdev, use NAPI as a threading
> abstraction or add extra fields to napi_struct ? 

Wait wait wait, you said "NAPI IRQ related logics doesn't fit here". I
could say the same for gro_cells -- IRQ related NAPI logics doesn't fit
there. gro_cells is an SW abstraction.
A fake netdev is used by multiple drivers to use GRO, you know that
(popular for wireless drivers). They also conflict with the queue config
effort.

> If other maintainers disagree I won't be upset, but I'm worried
> that letting NAPI grow into some generic SW abstraction with broad 
> use cases will hinder the ongoing queue config efforts.
> 
>> But nobody ever objected :>

Thanks,
Olek