Message-ID: <CAADnVQLTJt5zxuuuF9WZBd9Ui8r0ixvo37ohySX8X9U4kk9XbA@mail.gmail.com>
Date: Wed, 23 Apr 2025 19:33:52 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Jordan Rife <jordan@...fe.io>
Cc: Network Development <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>, 
	Aditi Ghag <aditi.ghag@...valent.com>, Daniel Borkmann <daniel@...earbox.net>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Willem de Bruijn <willemdebruijn.kernel@...il.com>, 
	Kuniyuki Iwashima <kuniyu@...zon.com>
Subject: Re: [PATCH v5 bpf-next 2/6] bpf: udp: Make sure iter->batch always
 contains a full bucket snapshot

On Wed, Apr 23, 2025 at 4:51 PM Jordan Rife <jordan@...fe.io> wrote:
>
> Require that iter->batch always contains a full bucket snapshot. This
> invariant is important to avoid skipping or repeating sockets during
> iteration when combined with the next few patches. Before, there were
> two cases where a call to bpf_iter_udp_batch may only capture part of a
> bucket:
>
> 1. When bpf_iter_udp_realloc_batch() returns -ENOMEM [1].
> 2. When more sockets are added to the bucket while calling
>    bpf_iter_udp_realloc_batch(), making the updated batch size
>    insufficient [2].
>
> In cases where the batch size only covers part of a bucket, it is
> possible to forget which sockets were already visited, especially if we
> have to process a bucket in more than two batches. This forces us to
> choose between repeating or skipping sockets, so don't allow this:
>
> 1. Stop iteration and propagate -ENOMEM up to userspace if reallocation
>    fails instead of continuing with a partial batch.
> 2. Retry bpf_iter_udp_realloc_batch() two times without holding onto the
>    bucket lock (hslot2->lock) so that we can use GFP_USER and maximize
>    the chances that memory allocation succeeds. On the third attempt, if
>    we still haven't been able to capture a full bucket snapshot, hold
>    onto the bucket lock through bpf_iter_udp_realloc_batch() to
>    guarantee that the bucket size doesn't change while we allocate more
>    memory and fill the batch. On the last pass, we must use GFP_ATOMIC
>    since we hold onto the spin lock.
>
> Introduce the udp_portaddr_for_each_entry_from macro and use it instead
> of udp_portaddr_for_each_entry to make it possible to continue iteration
> from an arbitrary socket. This is required for this patch in the
> GFP_ATOMIC case to allow us to fill the rest of a batch starting from
> the middle of a bucket and the later patch which skips sockets that were
> already seen.
>
> Testing all scenarios directly is a bit difficult, but I did some manual
> testing to exercise the code paths where GFP_ATOMIC is used and where
> where ERR_PTR(err) is returned. I used the realloc test case included
> later in this series to trigger a scenario where a realloc happens
> inside bpf_iter_udp_batch and made a small code tweak to force the first
> two realloc attempts to allocate a too-small buffer, thus requiring
> another attempt until the GFP_ATOMIC case is hit. Some printks showed
> three reallocs with the tests passing:
>
> Apr 16 00:08:32 crow kernel: go again (mem_flags=GFP_USER)
> Apr 16 00:08:32 crow kernel: go again (mem_flags=GFP_USER)
> Apr 16 00:08:32 crow kernel: go again (mem_flags=GFP_ATOMIC)

It looks like overdesign.
I think it would be much simpler to do GFP_USER once,
grab the lock and follow with GFP_NOWAIT|__GFP_NOWARN.
GFP_ATOMIC will deplete memory reserves.
bpf iterator is certainly not a critical operation, so use GFP_NOWAIT.

pw-bot: cr

Powered by blists - more mailing lists