Re: [PATCH bpf-next 2/2] samples/bpf: Add memcg async reclaim example

[bot+bpf-ci]

> diff --git a/samples/bpf/memcg_async_reclaim.h b/samples/bpf/memcg_async_reclaim.h
> new file mode 100644
> index 000000000000..96fe03d12912
> --- /dev/null
> +++ b/samples/bpf/memcg_async_reclaim.h
> @@ -0,0 +1,39 @@

[ ... ]

> +struct reclaim_cfg {
> +	/* Cgroup whose refaults are watched, i.e. the one under pressure. */
> +	__u64 monitor_cgroup_id;
> +	/* Cgroup that gets reclaimed from when the monitor refaults. */
> +	__u64 target_cgroup_id;
> +	/* Refault delta per tick that starts a reclaim round. */
> +	__u64 refault_threshold;
> +	/* Monitor tick period, in nanoseconds. */
> +	__u64 interval_ns;
> +	/* Bytes requested per bpf_wq callback. */
> +	__u64 batch_bytes;
> +	/* Callbacks per reclaim round. */
> +	__u64 max_batches;
                ^^^^

Does this comment accurately describe max_batches? The BPF side does not
count callbacks. It uses max_batches only to size a byte budget and keeps
requeueing until that budget is spent:

samples/bpf/memcg_async_reclaim.bpf.c:reclaim_work_fn() {
    elem->remaining = elem->max_batches * elem->batch_bytes;
    ...
    if (nr >= elem->remaining)
        elem->remaining = 0;
    else
        elem->remaining -= nr;
    if (elem->remaining)
        bpf_wq_start(&elem->work, 0);
}

bpf_proactive_reclaim() limits each pass to MEMCG_CHARGE_BATCH (64 pages),
which is 256 KiB with 4K pages.

With --batch 1M --max-batches 32 each callback reclaims at most about 256
KiB, so the round runs at least 128 callbacks, not 32. A partial reclaim
also adds callbacks. An overshoot, or a zero return that ends the round,
gives fewer.

Two other comments in the same patch describe it correctly: the .bpf.c
comment says "a clamped batch only means a round needs more callbacks,
because remaining is decremented by the bytes actually reclaimed", and the
loader says "One round budgets max_batches * batch_bytes". The header,
which documents the BPF/userspace interface, contradicts both.

The same wording is in the --max-batches help text in
memcg_async_reclaim_user.c.

Suggested wording: "Round budget, in units of batch_bytes."

> diff --git a/samples/bpf/memcg_async_reclaim_user.c b/samples/bpf/memcg_async_reclaim_user.c
> new file mode 100644
> index 000000000000..d5296fd0fab1
> --- /dev/null
> +++ b/samples/bpf/memcg_async_reclaim_user.c
> @@ -0,0 +1,1141 @@

[ ... ]

> +static void session_stop(struct session *s)
> +{
> +	unsigned long long failures, last_err;
> +
> +	if (!s->skel)
> +		return;
> +
> +	ring_buffer__free(s->rb);
> +	s->rb = NULL;
                ^^^^

In bench mode the reclaim_events ring buffer is never consumed, so the
event counters and --verbose output do not work there. s->called,
s->skipped_dying and s->target_gone are only updated from
on_reclaim_event(), and that callback only runs from ring_buffer__poll().

The only caller of ring_buffer__poll() is session_poll(), and the only
caller of session_poll() is do_watch().

do_bench() goes session_start() -> run_workload() -> session_stop(), and
session_stop() calls ring_buffer__free(s->rb) without a final
ring_buffer__consume().

So every bench run prints "events: called=0 skipped_dying=0 target_gone=0"
even when the "total:" line read from .bss shows calls=N > 0.

> +	failures = s->skel->bss->timer_failures;
> +	last_err = s->skel->bss->last_reclaim_err;
> +
> +	printf("\nran for %.1fs\n", now_sec() - s->start);
> +	print_counters(s, "total:");
> +	printf("events: called=%llu skipped_dying=%llu target_gone=%llu\n",
> +	       s->called, s->skipped_dying, s->target_gone);
> +	if (failures)
> +		printf("WARNING: monitor timer rearm failed %llu time(s), reclaim stopped early\n",
> +		       failures);
> +	if (last_err)
> +		printf("WARNING: bpf_proactive_reclaim() last failed with -%llu\n",
> +		       last_err);

The -v option, which usage() lists under "Common options" as "print every
reclaim event", prints nothing in bench mode.

The same missing final drain also drops any watch-mode events that arrive
between the last session_poll() and session_stop().

Should do_bench() poll or consume the ring buffer while the workload runs,
or should session_stop() call ring_buffer__consume(s->rb) before freeing
it?

[ ... ]

> +static int do_watch(struct options *o)
> +{
> +	struct session s = {};
> +	__u64 monitor_id, target_id;
> +	double next_stats, deadline = 0;
> +	int ret = 1;

[ ... ]

> +	while (!READ_ONCE(exiting)) {
> +		double now = now_sec();
> +		int wait_ms;
> +
> +		if (deadline && now >= deadline)
> +			break;
> +
> +		wait_ms = (int)((next_stats - now) * 1000);
> +		if (wait_ms < 0)
> +			wait_ms = 0;
> +
> +		if (session_poll(&s, wait_ms)) {
                    ^^^^

The --duration option is only checked once per statistics interval, so
watch mode can run well past the requested time.

wait_ms is computed only from next_stats and is never clamped to the
deadline. session_poll() keeps calling ring_buffer__poll() in 200 ms
slices until the whole wait_ms is used up, and it only returns early on a
signal or an error.

For example, with the default --stats 10 and --duration 3, nothing is
checked until about 10 s have passed on an idle system, so the program runs
for about 10 s instead of 3 s. With -s 60 -D 5 it runs for about 60 s.

usage() says "-D, --duration SEC    stop after SEC seconds".

Should wait_ms be limited to min(next_stats, deadline) - now?


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