Re: [Devel] [PATCH hci-8.1 v3 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 2 potential issue(s) to consider:
- [Low] selftests/ve: numa_stat poll threshold FILE_SIZE exceeds the data written
- [Low] selftests/ve: test does not compare folio order before and after migration
--

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

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

This commit adds ve_numa_migrate_test. The test writes a file in 64K chunks on an XFS image from a cgroup bound to one NUMA node. It then runs memory.numa_migrate to a second node and compares the file contents. It also checks folio sizes and nodes through pagemap, kpageflags and move_pages().

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

[ ... ]

> +static void folio_scan_end_run(struct folio_scan *fs, long run, int run_node,
> +			       int node, long pmd_pages)
> +{
> +	int order = 0;
> +
> +	if (!run)
> +		return;
> +	while ((1L << order) < run && order < SCAN_ORDERS - 1)
> +		order++;
> +	fs->hist[run_node != node][order]++;
> +	if (run == 1)
> +		return;
> +	if (!fs->smallest || run < fs->smallest)
> +		fs->smallest = run;
> +	if (run < pmd_pages && run_node == node)
> +		fs->on_node++;

[Severity: Low]
The commit message says the test checks that memory.numa_migrate keeps
the folio order. Is that what this checks?

In folio_scan_end_run(), any folio with 1 < run < pmd_pages that sits
on the target node increments on_node. Nothing compares the order seen
before the migration with the order seen after it.

A regression that gave an order-4 source a destination of a different
non-PMD order, or split it into smaller folios, would still satisfy
the final check:

	ASSERT_GT(fs.on_node, 0)
		TH_LOG("no folio below the PMD size reached node %d in one piece",

The before and after histograms are only printed with TH_LOG.

Could the test compare the two histograms, or could the commit message
be narrowed to say it catches the PMD-sized destination case?

> +}

[ ... ]

> +TEST_F(numa_migrate, large_folio_order)
> +{

[ ... ]

> +	for (off = 0; off < FILE_SIZE; off += CHUNK_SIZE) {
> +		if (chunk_is_hole(off, pmd_size))
> +			continue;
> +		memset(buf, (off / CHUNK_SIZE) & 0xff, CHUNK_SIZE);
> +		ASSERT_EQ(pwrite(fd, buf, CHUNK_SIZE, off), (ssize_t)CHUNK_SIZE);
> +	}

[ ... ]

> +	/* 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);
> +	}

[Severity: Low]
Can this break ever be taken?

The write loop skips the chunks where chunk_is_hole() is true. That is
the chunks at offsets 0 and 2 MiB. At most FILE_SIZE minus 2 * 64 KiB
(3.875 MiB) of the file's pages can therefore be on node_to.

The threshold is on_to_before + FILE_SIZE (4 MiB). Unless unrelated
file pages of the cgroup, such as XFS metadata or loop backing-file
pages, move to node_to and make up the difference, the loop sleeps for
the full STAT_RETRIES x STAT_RETRY_US, about 5 seconds.

The result is also only printed:

	TH_LOG("file bytes on node %d: %llu -> %llu", self->node_to,
	       on_to_before, on_to_after);

Should the threshold be the number of bytes actually written, or should
the loop be dropped or its outcome asserted?

[ ... ]

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

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