Re: [PATCH bpf-next v2] selftests/bpf: Add tests for memcg kfuncs

[Brahmajit Das <listout@listout.xyz>]

On 13.10.2026 15:53, bot+bpf-ci@kernel.org wrote:
> > 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
> 
...
> 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");
>                     ^^^^

Will do, for both the O_TMPFILE and the missing memory controller case.

> 
> 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?
> 

Right, only the request is clamped, and reclaim can overshoot it
(cgroup-v2.rst says as much for memory.reclaim). I'll drop the upper
bound and fix the commit message in v3.

> > +	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


-- 
Regards,
listout