> selftests/bpf: Add tests for memcg kfuncs
TL;DR:
- Test issue: the "reclaim clamped" upper-bound assertion in test_reclaim()
likely fails, since only the reclaim request is clamped, not the result;
commit message is also inaccurate
- Minor: silent test__skip() with no message
> diff --git a/tools/testing/selftests/bpf/prog_tests/memcg_kfuncs.c
> b/tools/testing/selftests/bpf/prog_tests/memcg_kfuncs.c
> new file mode 100644
> index 000000000000..4022d97dc1a9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_kfuncs.c
[ ... ]
> +static int alloc_pagecache_or_skip(size_t size)
> +{
> + int fd;
> +
> + fd = alloc_pagecache(size);
> + if (fd == -EOPNOTSUPP) {
> + test__skip();
> + return -1;
> + }
A subsystem pattern flags this as potentially concerning: a test that skips
silently with no message should be reported.
When the working directory cannot back a reclaimable O_TMPFILE (no O_TMPFILE
support, or the cwd is on tmpfs), the memory_events and reclaim subtests are
marked skipped through test__skip() and nothing is printed. test__skip()
only sets the skipped flag:
void test__skip(void)
{
if (env.subtest_state)
env.subtest_state->skipped = true;
else
env.test_state->skip_cnt++;
}
So someone reading a SKIP for memcg_kfuncs/memory_events or
memcg_kfuncs/reclaim cannot tell why it was skipped. The arena_memcg test
that this helper resembles prints "%s:SKIP:no disk-backed O_TMPFILE in cwd"
before calling test__skip().
On the other side, this is minor: the skip is correctly a skip rather than a
failure, and the commit message documents the skip condition.
Could a similar message be printed here before test__skip()?
[ ... ]
> +static void test_reclaim(struct memcg_kfuncs *skel)
> +{
> + long ret = 0;
> + int fd, i;
> +
> + fd = alloc_pagecache_or_skip(PAGECACHE_SIZE);
> + if (fd < 0)
> + return;
> +
> + /* Ask for more than one batch to check the clamp. */
> + for (i = 0; i < RECLAIM_RETRIES && ret <= 0; i++)
> + ret = do_reclaim(skel, 4 * MEMCG_CHARGE_BATCH * page_size, 0);
> +
> + ASSERT_GT(ret, 0, "reclaimed");
> + ASSERT_LE(ret, MEMCG_CHARGE_BATCH * page_size, "reclaim clamped");
^^^^
Does the "reclaim clamped" assertion hold? bpf_proactive_reclaim() only
clamps the reclaim target:
nr_pages = min(size / PAGE_SIZE, (unsigned long)MEMCG_CHARGE_BATCH);
nr_reclaimed = try_to_free_mem_cgroup_pages(memcg, nr_pages, ...);
return nr_reclaimed * PAGE_SIZE;
Nothing limits what try_to_free_mem_cgroup_pages() actually returns to that
target. do_try_to_free_pages() checks sc->nr_reclaimed >= sc->nr_to_reclaim
only after each full priority pass:
do {
...
shrink_zones(zonelist, sc);
if (sc->nr_reclaimed >= sc->nr_to_reclaim)
break;
...
} while (--sc->priority >= 0);
In the classic LRU path, shrink_lruvec() checks a local nr_reclaimed
(starting at 0) against the full sc->nr_to_reclaim, so one pass can add up
to the whole target on top of what earlier passes already reclaimed.
The MGLRU path is the same: try_to_shrink_lruvec() calls should_abort_scan()
only after each evict_folios() batch of up to MIN_LRU_BATCH (64) pages.
Both paths compute the per-pass scan target as lruvec_size >> priority.
With the 32MB (8192 x 4K pages) of clean page cache that test_reclaim()
creates, and swappiness 0 (SCAN_FILE), priorities 12 through 8 reclaim
2 + 4 + 8 + 16 + 32 = 62 pages. That is below nr_to_reclaim = 64, so the
loop drops to priority 7. There, nr[LRU_INACTIVE_FILE] = 64 and the pass
reclaims another 64 pages, for a total of about 126 pages (~504KB).
ASSERT_LE(ret, 64 * page_size) then fails.
For any page cache size around 32MB, the running total jumps from at most 63
pages straight past 64, so this is not a rare edge case. Large page cache
folios make it worse: isolate_lru_folios() takes a whole folio even when
nr_to_scan is smaller, so a single folio bigger than 64 pages already
exceeds the bound in the first pass. The same arithmetic applies with 64K
pages.
The commit message says "bpf_proactive_reclaim() reclaims clean page cache
and the result is clamped to MEMCG_CHARGE_BATCH pages", but only the request
is clamped, not the result. The kfunc's own kernel-doc describes @size as
the amount to reclaim (clamped), not as an upper bound on the return value.
Should the upper-bound assertion be dropped or relaxed, and the commit
message fixed?
> + ASSERT_EQ(ret % page_size, 0, "reclaim page aligned");
> +
> + close(fd);
> +}
---
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/38421980805
