Re: [PATCH 2/2] selftests/mm: test fork of a device DAX PUD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests/mm: sysfs_ul() and rss_file_kb() duplicate vm_util.h helpers
--

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

selftests/mm: test fork of a device DAX PUD

This commit adds the fork_dax_pud selftest to verify that fork() properly
accounts a copied device DAX PUD mapping in the child's RssFile when page
tables are copied due to a guard region in the VMA.

> diff --git a/tools/testing/selftests/mm/fork_dax_pud.c b/tools/testing/selftests/mm/fork_dax_pud.c
> new file mode 100644
> index 0000000000000..1fcf46341ace2
> --- /dev/null
> +++ b/tools/testing/selftests/mm/fork_dax_pud.c
[ ... ]
> +#include "kselftest.h"
> +#include "vm_util.h"
> +
> +#define SZ_1G		(1UL << 30)
> +#define DAX_SYSFS	"/sys/bus/dax/devices"
> +
> +static sigjmp_buf sigbus_jmp;
> +
> +static void sigbus_handler(int sig)
> +{
> +	siglongjmp(sigbus_jmp, 1);
> +}
> +
> +static unsigned long sysfs_ul(const char *dev, const char *attr)
> +{
> +	char path[PATH_MAX], buf[64];
> +	unsigned long val = 0;
> +	FILE *f;
> +
> +	snprintf(path, sizeof(path), DAX_SYSFS "/%s/%s", dev, attr);
> +	f = fopen(path, "r");
> +	if (!f)
> +		return 0;
> +	if (fgets(buf, sizeof(buf), f))
> +		val = strtoul(buf, NULL, 0);
> +	fclose(f);
> +	return val;
> +}
> +
> +static bool dax_dev_ok(const char *path)
> +{
> +	const char *name = strrchr(path, '/');
> +
> +	name = name ? name + 1 : path;
> +	return sysfs_ul(name, "align") == SZ_1G &&
> +	       sysfs_ul(name, "size") >= 2 * SZ_1G;
> +}
> +
> +static long rss_file_kb(void)
> +{
> +	char line[256];
> +	long kb = -1;
> +	FILE *f = fopen("/proc/self/status", "r");
> +
> +	if (!f)
> +		return -1;
> +	while (fgets(line, sizeof(line), f))
> +		if (sscanf(line, "RssFile: %ld", &kb) == 1)
> +			break;
> +	fclose(f);
> +	return kb;
> +}

[Severity: Medium]
fork_dax_pud.c includes vm_util.h and links against vm_util.c, which already
provide shared helpers for reading sysfs attributes and parsing
/proc/self/status:

tools/testing/selftests/mm/vm_util.h:
bool check_for_pattern(FILE *fp, const char *pattern, char *buf, size_t len);
uint64_t read_pmd_pagesize(void);
unsigned long rss_anon(void);
...
int write_sysfs(const char *file_path, unsigned long val);
int read_sysfs(const char *file_path, unsigned long *val);

In main() -> dax_dev_ok() -> sysfs_ul(), the test hand-rolls sysfs unsigned
long reading with fopen(), fgets(), strtoul(), and fclose() to read the
device align and size attributes under /sys/bus/dax/devices/<dev>/.

Similarly, in the child path of main() -> rss_file_kb(), the test manually
scans /proc/self/status for RssFile using fopen(), fgets(), and sscanf().

Could sysfs_ul() use read_sysfs() (or read_file()), and could rss_file_kb()
use check_for_pattern() or be added alongside rss_anon() in vm_util.c instead
of reimplementing these helpers locally?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021-vstor-150143-copy_huge_pud-folio-refcount-v1-0-8bd88551f9be@virtuozzo.com?part=2