Re: [PATCH bpf-next] selftests/bpf: Skip memcg_proactive_reclaim when data lands in shmem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] selftests/bpf: workload_dir() never falls back to . when /tmp is
  overlayfs
- [Low] selftests/bpf: unflushed per-CPU memcg stats read in
  charged_as_shmem()
--

commit e2cf55e88beac1906e9ded044c8e29f6b8bf9980
Author: Kurt Lin <lylin1023@gmail.com>

selftests/bpf: Skip memcg_proactive_reclaim when data lands in shmem

This commit skips the memcg_proactive_reclaim selftest when memory.stat
shows that the test file's pages were charged as shmem, such as when /tmp
is an overlayfs backed by tmpfs.

> diff --git a/tools/testing/selftests/bpf/prog_tests/memcg_proactive_reclaim.c b/tools/testing/selftests/bpf/prog_tests/memcg_proactive_reclaim.c
> index c22dcfabe856e..c035506760416 100644
> --- a/tools/testing/selftests/bpf/prog_tests/memcg_proactive_reclaim.c
> +++ b/tools/testing/selftests/bpf/prog_tests/memcg_proactive_reclaim.c
> @@ -46,6 +46,28 @@ static const char *workload_dir(void)
>  	return NULL;
>  }
>  
> +/*
> + * statfs() only sees the top of a stacking filesystem. When /tmp is an
> + * overlayfs whose upper layer lives on tmpfs, as in virtme-ng guests, the
> + * check above passes but the written data still ends up as shmem. Ask the
> + * cgroup how the pages were actually charged.
> + */
> +static bool charged_as_shmem(void)
> +{
> +	static char buf[8192];
> +	unsigned long long shmem;
> +	char *p;
> +
> +	if (read_cgroup_file(CG_PATH, "memory.stat", buf, sizeof(buf)))
> +		return false;
> +
> +	p = strstr(buf, "\nshmem ");
> +	if (!p || sscanf(p, "\nshmem %llu", &shmem) != 1)
> +		return false;
> +
> +	return shmem >= FILE_SIZE / 2;
> +}

[Severity: Low]
Can charged_as_shmem() read 0 for shmem when per-CPU memcg stats have not
been flushed yet?

Reading memory.stat calls __mem_cgroup_flush_stats(memcg, false), which skips
the rstat flush unless pending updates exceed MEMCG_CHARGE_BATCH *
num_online_cpus():

mm/memcontrol.c:memcg_vmstats_needs_flush() {
	return atomic_long_read(&vmstats->stats_updates) >
		MEMCG_CHARGE_BATCH * num_online_cpus();
}

On a 64K-page kernel with 24 or more online CPUs (or a 4K-page kernel with
384 or more CPUs), writing 32 MiB of shmem does not cross that threshold, so
charged_as_shmem() sees shmem as 0 and fails to skip the test.  Retrying
until the "file " counter in memory.stat reaches at least FILE_SIZE / 2
(across the 2s periodic flusher window, as in cgroup_iter_memcg.c) would
ensure the stats are flushed before checking shmem.

>  void test_memcg_proactive_reclaim(void)
>  {
>  	struct memcg_proactive_reclaim *skel = NULL;
> @@ -99,6 +121,12 @@ void test_memcg_proactive_reclaim(void)
>  	if (!ASSERT_OK(fsync(data_fd), "fsync"))
>  		goto out;
>  
> +	if (charged_as_shmem()) {
> +		printf("%s:SKIP: %s is backed by shmem\n", __func__, dir);
> +		test__skip();
> +		goto out;
> +	}

[Severity: Low]
Does jumping straight to out here prevent falling back to "." when /tmp is
overlayfs over tmpfs?

workload_dir() checks dirs[] = { "/tmp", "." } so the test can use the
current working directory when /tmp is on tmpfs.  When /tmp is an overlayfs
over tmpfs, statfs("/tmp", &st) sees OVERLAYFS_SUPER_MAGIC and returns
"/tmp" on the first iteration, and this check then skips the whole test
without ever trying "." even if test_progs is running from a writable
disk-backed directory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113180159.602033-1-lylin1023@gmail.com?part=1