Re: [PATCH v3] hugetlb: only adjust reservation during unmapping if mapcount is 0

On 30 Jul 15:01, Andrew Morton wrote:
>
> On Tue, 28 Jul 2026 21:29:03 +0200 Guillaume Morin <[email protected]> wrote:
> 
> > Since df7a6d1f6405, __unmap_hugepage_range can adjust reservations. In
> > the case of folio mapped in both a parent and a child, if the parent
> > unmaps the range first, the reservation adjustment will result in
> > an underflow of the reserved count. Once the child unmaps the range, the
> > count is restored.
> > Change __unmap_hugepage_range() to check the mapcount before adjusting
> > the reservation.
> 
> Thanks.  AI review mentions a couple of things, one pre-existing:
> 	https://sashiko.dev/#/patchset/[email protected]
 
Not sure about the pre-existing one, perhaps Bruno/Rik can comment?

But if I understand the bot scenario correctly, the new issue is
incorrect.

The bot is correct that during the 2nd fault region_add() will
return 0 when the new reservation is committed. But that return value is
changed into a 1 at the end of __vma_reservation_common() for private
mappings (there is a lengthy comment about that). Therefore it does not
fallback to the 'hugepage_subpool_put_pages(spool, 1)' path and
therefore there shouldn't be any resv count change.

I wrote a quick repro of the scenario as I understand it and the
reserved count is as expected.

#include <ctype.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <unistd.h>

int hugetlb_nr_resv_pages(unsigned long *num)
{
       const char* field = "HugePages_Rsvd: ";
       FILE *f = fopen("/proc/meminfo", "r");
       char line[256];

       if (!f) {
               perror("Could not open /proc/meminfo");
               return 0;
       }
       while (fgets(line, sizeof(line), f)) {
               if (strncmp(line, field, strlen(field)))
                       continue;
               const char *p = line + strlen(field);
               while (*p && isspace(*p))
                       ++p;
               if (!*p) {
                       break;
               }
               *num = strtoul(p, NULL, 10);
               fclose(f);
               return 1;
       }
       fclose(f);
       return 0;
}

int main(void) {
    pid_t pid;
    unsigned long num;
    void *mem = mmap(NULL, 2048*1024, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB, -1, 0);
    if (mem == MAP_FAILED) {
        perror("mmap");
        return 1;
    }
    if (!hugetlb_nr_resv_pages(&num)) {
        perror("hugetlb_nr_resv_pages");
        return 1;
    }
    printf("Before the initial fault, number of reserved huge pages: %lu\n", num);
    *(char *)mem = 1;
    if (!hugetlb_nr_resv_pages(&num)) {
        perror("hugetlb_nr_resv_pages");
        return 1;
    }
    printf("After the initial fault, number of reserved huge pages: %lu\n", num);

    pid = fork();
    if (pid == -1) {
        perror("fork");
        return 1;
    }
    if (pid == 0) {
        // Child process
        pause();
        return 0;
    } 
    // Parent process
    if (madvise(mem, 2048*1024, MADV_DONTNEED)) {
        perror("madvise");
        return 1;
    }
    if (!hugetlb_nr_resv_pages(&num)) {
        perror("hugetlb_nr_resv_pages");
        return 1;
    }
    printf("After MADV_DONTNEED, number of reserved huge pages: %lu\n", num);
    if (!hugetlb_nr_resv_pages(&num)) {
        perror("hugetlb_nr_resv_pages");
        return 1;
    }
    *(char *)mem = 1;
    if (!hugetlb_nr_resv_pages(&num)) {
        perror("hugetlb_nr_resv_pages");
        return 1;
    }
    printf("After the new fault, number of reserved huge pages: %lu\n", num);
    kill(pid, SIGINT);

    return 0;
}


-- 
Guillaume Morin <[email protected]>