Re: [Devel] [PATCH hci-8.1 v2 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 3 potential issue(s) to consider:
- [Medium] selftests/ve: large-folio guard accepts PMD-sized folios, can pass without hitting bug
- [Medium] selftests/ve: numa_migrate test needs 100% migration but large-folio dest alloc is no-retry
- [Low] selftests/ve: PAGE_SIZE hard-coded to 4096 in pagemap index arithmetic
--

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

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

This adds ve_numa_migrate_test. It 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 data back against the
written pattern. It also checks through memory.numa_stat that the file
pages moved, and checks through pagemap and kpageflags that the page
cache holds large folios.

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

> 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 0000000000000..b4a60dba8063d
> --- /dev/null
> +++ b/tools/testing/selftests/ve/ve_numa_migrate_test.c

[ ... ]

> +#define IMG_SIZE	(320UL << 20)
> +#define FILE_SIZE	(4UL << 20)
> +#define CHUNK_SIZE	(64UL << 10)
> +#define PAGE_SIZE	4096UL

[Severity: Low]
Is hard-coding PAGE_SIZE to 4096UL safe here?

large_folio_pages() uses it for the page count and for the pagemap offset:

    unsigned long nr = size / PAGE_SIZE, i;
    ...
    (unsigned long)map / PAGE_SIZE * sizeof(*pm)

On a host with a base page size other than 4K (arm64 or ppc64 with 64K
pages), both values are wrong. The folio check can then give a bogus
failure or a bogus result. The "FILE_SIZE / PAGE_SIZE" value in the
TH_LOG message uses the same constant.

Would sysconf(_SC_PAGESIZE) be a better choice? A libc definition of
PAGE_SIZE could also make this define a redefinition.

[ ... ]

> +TEST_F(numa_migrate, large_folio_order)
> +{

[ ... ]

> +	large = large_folio_pages(fd, FILE_SIZE, &folios);
> +	ASSERT_GE(large, 0) TH_LOG("cannot read the page cache folio sizes");
> +	TH_LOG("%ld of %lu file pages are in %ld large folios",
> +	       large, FILE_SIZE / PAGE_SIZE, folios);
> +	ASSERT_GT(large, 0)
> +		TH_LOG("no large folio in the page cache, the bug is out of reach");

[Severity: Medium]
Does this check fully back the commit message claim that a file without
a suitable large folio "would pass without touching the bug"?

large_folio_pages() counts every page with KPF_COMPOUND_HEAD or
KPF_COMPOUND_TAIL, and the test only asserts large > 0. The bug needs a
source folio smaller than PMD order, because the old
memcg_numa_migrate_new_folio() always allocated a PMD-sized destination.

If the page cache held only PMD-order folios, the old and new code would
allocate identical destinations. The test would then pass on an unfixed
kernel without hitting the bug.

The test already computes "heads" and "large". Could it also require that
the folios are smaller than 512 pages, for example by checking that
large / folios is below 512?

[ ... ]

> +	/* memory.numa_stat flushes per-cpu counters lazily, give it time. */
> +	for (i = 0; i < STAT_RETRIES; i++) {
> +		ASSERT_EQ(file_bytes_on_node(self->cg_fd, self->node_to,
> +					     &on_to_after), 0);
> +		if (on_to_after >= on_to_before + FILE_SIZE)
> +			break;
> +		usleep(STAT_RETRY_US);
> +	}
> +	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)
> +		TH_LOG("memory.numa_migrate moved nothing to node %d", self->node_to);

[Severity: Medium]
Can this assertion fail on a healthy kernel?

It requires every one of the 4 MiB of file pages to land on node_to. The
migration path is:

memcg_numa_migrate_pages()
  __memcg_numa_migrate_pages()
    migrate_pages(&pages, memcg_numa_migrate_new_folio, NULL,
                  (unsigned long)&ms, MIGRATE_ASYNC, MR_SYSCALL, NULL)

For a large source folio, memcg_numa_migrate_new_folio() allocates the
destination with:

    gfp_mask = GFP_TRANSHUGE_LIGHT | __GFP_THISNODE;
    order = folio_order(old_folio);

GFP_TRANSHUGE_LIGHT has no direct reclaim and no compaction. Under
fragmentation or memory pressure on the target node this allocation can
fail, and migrate_pages() then leaves that folio on node_from.

__memcg_numa_migrate_pages() only propagates ret < 0:

    putback_movable_pages(&pages);
    if (ret < 0)
        return ret;

The write to memory.numa_migrate therefore still succeeds after a partial
migration. memcg_numa_isolate_pages() also skips folios silently when
folio_try_get() fails, the folio is not on the LRU, it is under writeback,
or folio_trylock() fails.

Any single skipped or failed folio leaves the node_to count below
on_to_before + FILE_SIZE. The data comparison has already passed at that
point, so the test would report a failure although the fix works. The
retry loop only waits for the lazy memcg flush and does not tolerate a
shortfall.

Should the final check allow some slack, or otherwise be made robust
against partial migration?

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

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