Re: [PATCH bpf-next 04/16] libbpf: Copy the percpu chunks into the bpf arena

[bot+bpf-ci]

> libbpf: Copy the percpu chunks into the bpf arena

TL;DR:
- arena_percpu_populate() may pin the loader thread to cpus the app excluded
  (isolcpus)
- the final affinity restore may permanently shrink the thread's cpu mask, and
  its failure is unchecked

> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c

[ ... ]

> @@ -5649,6 +5650,49 @@ static struct bpf_map *bpf_object__sec_map(struct bpf_object *obj, int sec_idx)
> +/*
> + * Copy the percpu chunks into the mmapped bpf arena, one chunk at a
> + * time. Before copying each chunk, pin the thread to the chunk's cpu,
> + * so the copy faults the chunk's pages in from that cpu's NUMA node.
> + * If pinning fails, fall back to copying on the current cpu.
> + */
> +static void arena_percpu_populate(struct bpf_object *obj, void *arena_mmaped)
> +{
> +	size_t chunk_sz = obj->arena_percpu.chunk_sz;
> +	cpu_set_t *saved_set = NULL, *one = NULL;
> +	size_t set_sz;
> +	void *base;
> +	int cpu;
> +
> +	base = arena_mmaped + obj->arena_percpu.offset;
> +
> +	set_sz = CPU_ALLOC_SIZE(obj->arena_percpu.nr_chunks);
> +	saved_set = CPU_ALLOC(obj->arena_percpu.nr_chunks);
> +	one = CPU_ALLOC(obj->arena_percpu.nr_chunks);
> +	if (!saved_set || !one || sched_getaffinity(0, set_sz, saved_set) != 0) {
> +		/* no affinity info, fall back to copying on the current cpu */
> +		for (cpu = 0; cpu < obj->arena_percpu.nr_chunks; cpu++)
> +			memcpy(base + cpu * chunk_sz, obj->arena_percpu.chunk, chunk_sz);
> +		goto out;
> +	}
> +
> +	for (cpu = 0; cpu < obj->arena_percpu.nr_chunks; cpu++) {
> +		CPU_ZERO_S(set_sz, one);
> +		CPU_SET_S(cpu, set_sz, one);
> +		sched_setaffinity(0, set_sz, one);

Can this loop move the calling thread onto cpus the application deliberately
excluded?  saved_set is fetched but never consulted, so the loop pins to
every cpu in [0, nr_chunks) regardless of the caller's mask.

The kernel's __sched_setaffinity() only intersects the new mask with
cpuset_cpus_allowed(), not with the thread's earlier mask.  An application
that limited its threads with taskset or sched_setaffinity() (for example to
housekeeping cpus 0-3 on a box booted with isolcpus= or nohz_full=4-63) would
have libbpf run on every isolated cpu during bpf_object__load().  Each pin
also forces a migration through stop_one_cpu() that preempts whatever is
running on that cpu.

Would it work to pin only to cpus that are set in saved_set, and copy the
remaining chunks without pinning?

> +
> +		/* faults the chunk pages in from the target cpu's NUMA node */
> +		memcpy(base + cpu * chunk_sz, obj->arena_percpu.chunk, chunk_sz);
> +	}
> +
> +	sched_setaffinity(0, set_sz, saved_set);

Does this final sched_setaffinity() actually give back the original state?
The kernel's sched_getaffinity() returns:

    cpumask_and(mask, &p->cpus_mask, cpu_active_mask);

so saved_set holds only the cpus that are active at the time of the call, not
the thread's full cpus_mask.  A thread with default affinity has a cpus_mask
covering all possible cpus, so after this call its cpus_mask shrinks to the
cpus that were online when the object was loaded.

Because the syscall sets SCA_USER, that snapshot is also stored as
p->user_cpus_ptr, and __set_cpus_allowed_ptr() clamps later cpuset updates
to it:

    if (p->user_cpus_ptr && !(ctx->flags & (SCA_USER | ...)) &&
        cpumask_and(rq->scratch_mask, ctx->new_mask, p->user_cpus_ptr))
            ctx->new_mask = rq->scratch_mask;

For example, take a vm with possible=0-63 and online=0-15, loading an object
that has .arena.percpu variables.  From then on the loader thread, and every
thread or process it creates afterwards (cpus_mask and user_cpus_ptr are
inherited), could never run on cpus 16-63, even after they are hot-added or
the cpuset is widened.  Before this patch, bpf_object__load() never touched
scheduling state.

The return value of this sched_setaffinity() is also ignored.  If it fails
(-ENOMEM from alloc_user_cpus_ptr(), or -EINVAL when a concurrent cpuset or
hotplug change leaves saved_set with no allowed or active cpus), the thread
stays pinned to the last cpu it was moved to, and nothing reports it.

The commit message and the function comment only cover the case where pinning
fails.  They do not mention changing or restoring the caller's affinity, and
no later patch in this series changes arena_percpu_populate().

Would doing the per-cpu copies in a short-lived helper thread avoid touching
the caller's thread altogether?  Otherwise, could the return value of the
restore at least be checked and reported with pr_warn()?

> +
> +out:
> +	CPU_FREE(saved_set);
> +	CPU_FREE(one);
> +	zfree(&obj->arena_percpu.chunk);
> +}

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39434884362