Re: [LTP] [PATCH] hugemmap10: Account for memory policy in counter checks

[Li Wang <li.wang@linux.dev>]

Hi Huaisheng,

This patch make sense, minor comments inline below:

>  .../kernel/mem/hugetlb/hugemmap/hugemmap10.c  | 73 ++++++++++++++++++-
>  1 file changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap10.c b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap10.c
> index 5b5577a0e..6d1cd6241 100644
> --- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap10.c
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap10.c
> @@ -13,6 +13,9 @@
>   */
> 
>  #define _GNU_SOURCE
> +#include <errno.h>
> +#include <linux/mempolicy.h>
> +#include <stdlib.h>
>  #include <unistd.h>
>  #include <stdio.h>
>  #include <sys/mount.h>
> @@ -21,8 +24,10 @@
>  #include <sys/types.h>
> 
>  #include "hugetlb.h"
> +#include "lapi/syscalls.h"
> 
>  #define MNTPOINT "hugetlbfs/"
> +#define ULONG_BITS (sizeof(unsigned long) * CHAR_BIT)
> 
>  static long hpage_size;
>  static int private_resv;
> @@ -48,6 +53,63 @@ static void read_meminfo_huge(long *total, long *free, long *resv, long *surp)
>  	*surp = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SURP);
>  }
> 
> +static int node_isset(unsigned long node, const unsigned long *nodemask)
> +{
> +	return nodemask[node / ULONG_BITS] & (1UL << (node % ULONG_BITS));
> +}
> +
> +static unsigned long read_allowed_free_hugepages(void)
> +{
> +	char path[PATH_MAX];
> +	size_t mask_size = getpagesize();
> +	unsigned long allowed_free = 0;
> +	unsigned long maxnode = mask_size * CHAR_BIT;
> +	unsigned long *policy_nodemask;
> +	unsigned long *cpuset_nodemask;
> +	unsigned long node, node_free;
> +	int mode;
> +
> +	policy_nodemask = SAFE_CALLOC(1, mask_size);
> +	cpuset_nodemask = SAFE_CALLOC(1, mask_size);
> +
> +	if (syscall(__NR_get_mempolicy, &mode, policy_nodemask,
> +		    maxnode, NULL, 0)) {
> +		if (errno == ENOSYS) {
> +			allowed_free = prev_free;
> +			goto out;
> +		}
> +		tst_brk(TBROK | TERRNO, "get_mempolicy() failed");
> +	}
> +
> +	if (syscall(__NR_get_mempolicy, NULL, cpuset_nodemask,
> +		    maxnode, NULL, MPOL_F_MEMS_ALLOWED))
> +		tst_brk(TBROK | TERRNO,
> +			"get_mempolicy(MPOL_F_MEMS_ALLOWED) failed");
> +
> +	mode &= ~MPOL_MODE_FLAGS;
> +	for (node = 0; node < maxnode; node++) {
> +		if (!node_isset(node, cpuset_nodemask))
> +			continue;
> +		if (mode == MPOL_BIND && !node_isset(node, policy_nodemask))
> +			continue;
> +
> +		snprintf(path, sizeof(path),
> +			 "/sys/devices/system/node/node%lu/hugepages/"
> +			 "hugepages-%ldkB/free_hugepages",
> +			 node, hpage_size / 1024);
> +		if (access(path, R_OK))
> +			continue;
> +
> +		SAFE_FILE_SCANF(path, "%lu", &node_free);
> +		allowed_free += node_free;
> +	}
> +
> +out:
> +	free(policy_nodemask);
> +	free(cpuset_nodemask);
> +	return allowed_free;
> +}
> +
>  static int kernel_has_private_reservations(void)
>  {
>  	int fd;
> @@ -178,10 +240,14 @@ out:
> 
>  static int map_(int s, int hpages, int flags, char *desc, int line)
>  {
> +	unsigned long allowed_free = 0;
>  	long et, ef, er, es;

> +	int creates_reservation = (flags & MAP_SHARED) || private_resv;

Maybe we can move this definition into setup() and then replace all the
reservation syntax globally.

> 
>  	map_fd[s] = tst_creat_unlinked(MNTPOINT, 0, 0600);
>  	map_size[s] = hpages * hpage_size;
> +	if (creates_reservation)
> +		allowed_free = read_allowed_free_hugepages();
>  	map_addr[s] = SAFE_MMAP(NULL, map_size[s], PROT_READ|PROT_WRITE, flags,
>  				map_fd[s], 0);
>  	touched[s] = 0;
> @@ -201,11 +267,16 @@ static int map_(int s, int hpages, int flags, char *desc, int line)
>  	 * mappings behave like MAP_SHARED at mmap time.  Otherwise,
>  	 * no counter updates will occur.
>  	 */
> -	if ((flags & MAP_SHARED) || private_resv) {
> +	if (creates_reservation) {
>  		unsigned long shortfall = 0;
> 
>  		if (hpages + prev_resv > prev_free)
>  			shortfall = hpages - prev_free + prev_resv;
> +
> +		if ((unsigned long)hpages > allowed_free)
> +			shortfall = MAX(shortfall,
> +					(unsigned long)hpages - allowed_free);
> +
>  		et += shortfall;
>  		ef += shortfall;
>  		er += hpages;
> --
> 2.52.0

Aside from this patch, my test encountered new failures on a four-node system.
Without your patch, the surplus failure can be reproduced consistently, so I
suspect it's a different issue.

...
hugemmap10.c:402: TFAIL: While doing munmap shared after touch: Bad HugePages_Total: expected 1, actual 2
hugemmap10.c:402: TFAIL: While doing munmap shared after touch: Bad HugePages_Free: expected 1, actual 2
hugemmap10.c:402: TFAIL: While doing munmap shared after touch: Bad HugePages_Surp: expected 0, actual 1


-- 
Regards,
Li Wang

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp