Re: [Devel] [PATCH hci-8.1 2/2] selftests/ve: check memory.numa_migrate keeps the folio order

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] selftests/ve: numa_migrate test never checks that large folios were created
- [Medium] selftests/ve: strict numa_migrate pass threshold can fail on a correct kernel and leave the XFS mount behind
- [Medium] selftests/ve: mkfs.xfs skip probe checks fixed paths, not PATH
- [Low] selftests/ve: ve_numa_migrate_test binary missing from .gitignore
--

commit 53864b9544be735f4056fc53973030b5631878a2
Author: Konstantin Khorenko <khorenko@virtuozzo.com>

selftests/ve: check memory.numa_migrate keeps the folio order

This adds a selftest that writes a file on an XFS loop image from a
cgroup bound to one NUMA node, migrates the cgroup to a second node
with memory.numa_migrate, and reads the file back against the written
pattern. It is meant to catch memcg_numa_migrate_new_folio() allocating
an oversized destination for a large page cache folio. It skips when
root, two NUMA nodes, cgroup2 controllers or mkfs.xfs are missing.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-151607

> diff --git a/tools/testing/selftests/ve/Makefile b/tools/testing/selftests/ve/Makefile
> index 51f9316beb99..e96320a18384 100644
> --- a/tools/testing/selftests/ve/Makefile
> +++ b/tools/testing/selftests/ve/Makefile
> @@ -6,6 +6,7 @@ TEST_GEN_PROGS += ve_ns_owner_test
>  TEST_GEN_PROGS += ve_perms_test
>  TEST_GEN_PROGS += ve_mount_accounting_test
>  TEST_GEN_PROGS += ve_vtty_stress
> +TEST_GEN_PROGS += ve_numa_migrate_test

[Severity: Low]
Should tools/testing/selftests/ve/.gitignore get a matching entry?

The ignore list currently ends with ve_vtty_stress and has no entry for
ve_numa_migrate_test, which this line now builds.  After a build the
binary will show up as untracked in git status, and the patch does not
touch .gitignore.

[ ... ]

> diff --git a/tools/testing/selftests/ve/ve_numa_migrate_test.c b/tools/testing/selftests/ve/ve_numa_migrate_test.c
> new file mode 100644
> index 000000000000..5fbf4af49f7f
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_numa_migrate_test.c

[ ... ]

> + * Write a file on an XFS image (XFS allocates large folios for 64K writes)
> + * from a cgroup bound to one NUMA node, migrate the cgroup to another node
> + * and check that the data reads back intact.

[Severity: Medium]
Is it safe to assume large folios were actually created here?

Nothing in the test verifies it.  mapping_max_folio_order() returns 0
when CONFIG_TRANSPARENT_HUGEPAGE is not set.  Large folio allocation can
also fall back to order-0 under fragmentation or memory pressure.

In either case every folio is order-0, and the only precondition check
(ASSERT_GE on the node_to file bytes) is satisfied by order-0 folios as
well.  The memcmp readback then succeeds because nothing was mis-migrated,
and the test reports PASS without covering the folio_test_large() branch
in memcg_numa_migrate_new_folio().

Could the test check for large folios, for example through
/proc/kpageflags or the THP counters, and SKIP when none are seen?

[ ... ]

> +	if (access("/usr/sbin/mkfs.xfs", X_OK) && access("/sbin/mkfs.xfs", X_OK))
> +		SKIP(return, "needs mkfs.xfs");

[Severity: Medium]
Can this probe cause a false skip?

It only looks in /usr/sbin and /sbin, while prepare() runs the tool
through run_cmd() and system(), which resolves it via PATH:

	if (run_cmd("mkfs.xfs -q -f %s", self->img)) {
		*why = "mkfs.xfs failed";

On a host where mkfs.xfs is in /usr/bin or /usr/local/sbin, the test
skips with "needs mkfs.xfs" before prepare() would have found the tool.
A mkfs failure in prepare() already turns into a SKIP, so would it be
simpler to drop this pre-probe?

[ ... ]

> +	/* From here on a failure means eviction would crash the kernel. */
> +	self->leave_mounted = 1;
> +	snprintf(list, sizeof(list), "%d,%d", self->node_from, self->node_to);
> +	ASSERT_EQ(write_file_at(self->cg_fd, "cpuset.mems", list), 0);
> +	ASSERT_EQ(write_u64_at(self->cg_fd, "memory.numa_migrate",
> +			       self->node_to), 0);
> +
> +	ASSERT_EQ(file_bytes_on_node(self->cg_fd, self->node_to,
> +				     &on_to_after), 0);
> +	TH_LOG("file bytes on node %d: %llu -> %llu", self->node_to,
> +	       on_to_before, on_to_after);
> +	ASSERT_GE(on_to_after, on_to_before + FILE_SIZE);

[Severity: Medium]
Can this strict threshold fail on a correct kernel and then leave the XFS
mount behind?

The assertion requires every byte of the 4MB file to have moved to
node_to.  The memory.numa_migrate write does not guarantee that:

mm/memcontrol.c:__memcg_numa_migrate_pages() {
    ret = migrate_pages(&pages, memcg_numa_migrate_new_folio,
                        NULL, (unsigned long)&ms, MIGRATE_ASYNC,
                        MR_SYSCALL, NULL);
    putback_movable_pages(&pages);
    if (ret < 0)
        return ret;
    ...
}

Only a negative return is propagated.  A positive count of folios that
failed to migrate is dropped, and __memcg_numa_migrate_write() returns
"ret ?: nbytes", so the ASSERT_EQ on the write passes on a partial
migration.

Large destination folios are allocated in memcg_numa_migrate_new_folio()
with:

    if (folio_test_large(old_folio)) {
        gfp_mask = GFP_TRANSHUGE_LIGHT | __GFP_THISNODE;
        order = folio_order(old_folio);
    }

That has no direct reclaim or compaction and no node fallback, so it can
fail on a fragmented node_to and leave the folio on node_from.

memcg_numa_isolate_pages() also skips folios that are not on the LRU:

    /* Only take pages on LRU: a check now makes later tests safe */
    if (!folio_test_lru(folio))
        goto isolate_fail;

Folios still sitting in per-CPU LRU batches are skipped without a drain.

Any of these makes the ASSERT_GE fail without a kernel bug.  Because
leave_mounted was set just above, FIXTURE_TEARDOWN then skips the unmount
and the loop mount and image stay on the host.

Would a tolerant check, such as requiring a meaningful fraction of the
file to have moved, work better?  Could leave_mounted also be set only
after the data-integrity check has shown a problem, rather than before
the migration?

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261028-b4-prep-khorenko-vstor-151607-numa-migrate-order-v1-0-268336526677@virtuozzo.com?part=2

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel