Re: [PATCH 1/1] mm:hugetlbfs: Fix hwpoison reserve accounting

[Naoya Horiguchi]

On Fri, Oct 20, 2017 at 10:49:46AM -0700, Mike Kravetz wrote:
> On 10/19/2017 07:30 PM, Naoya Horiguchi wrote:
> > On Thu, Oct 19, 2017 at 04:00:07PM -0700, Mike Kravetz wrote:
> >
> > Thank you for addressing this. The patch itself looks good to me, but
> > the reported issue (negative reserve count) doesn't reproduce in my trial
> > with v4.14-rc5, so could you share the exact procedure for this issue?
>
> Sure, but first one question on your test scenario below.
>
> >
> > When error handler runs over a huge page, the reserve count is incremented
> > so I'm not sure why the reserve count goes negative.
>
> I'm not sure I follow.  What specific code is incrementing the reserve
> count?

The call path is like below:

  hugetlbfs_error_remove_page
    hugetlb_fix_reserve_counts
      hugepage_subpool_get_pages(spool, 1)
        hugetlb_acct_memory(h, 1);
          gather_surplus_pages
            h->resv_huge_pages += delta;

>
> >                                                      My operation is like below:
> >
> >   $ sysctl vm.nr_hugepages=10
> >   $ grep HugePages_ /proc/meminfo
> >   HugePages_Total:      10
> >   HugePages_Free:       10
> >   HugePages_Rsvd:        0
> >   HugePages_Surp:        0
> >   $ ./test_alloc_generic -B hugetlb_file -N1 -L "mmap access memory_error_injection:error_type=madv_hard"  // allocate a 2MB file on hugetlbfs, then madvise(MADV_HWPOISON) on it.
> >   $ grep HugePages_ /proc/meminfo
> >   HugePages_Total:      10
> >   HugePages_Free:        9
> >   HugePages_Rsvd:        1  // reserve count is incremented
> >   HugePages_Surp:        0
>
> This is confusing to me.  I can not create a test where there is a reserve
> count after poisoning page.
>
> I tried to recreate your test.  Running unmodified 4.14.0-rc5.
>
> Before test
> -----------
> HugePages_Total:       1
> HugePages_Free:        1
> HugePages_Rsvd:        0
> HugePages_Surp:        0
> Hugepagesize:       2048 kB
>
> After open(creat) and mmap of 2MB hugetlbfs file
> ------------------------------------------------
> HugePages_Total:       1
> HugePages_Free:        1
> HugePages_Rsvd:        1
> HugePages_Surp:        0
> Hugepagesize:       2048 kB
>
> Reserve count is 1 as expected/normal
>
> After madvise(MADV_HWPOISON) of the single huge page in mapping/file
> --------------------------------------------------------------------
> HugePages_Total:       1
> HugePages_Free:        0
> HugePages_Rsvd:        0
> HugePages_Surp:        0
> Hugepagesize:       2048 kB
>
> In this case, the reserve (and free) count were decremented.  Note that
> before the poison operation the page was not associated with the mapping/
> file.  I did not look closely at the code, but assume the madvise may
> cause the page to be 'faulted in'.
>
> The counts remain the same when the program exits
> -------------------------------------------------
> HugePages_Total:       1
> HugePages_Free:        0
> HugePages_Rsvd:        0
> HugePages_Surp:        0
> Hugepagesize:       2048 kB
>
> Remove the file (rm /var/opt/oracle/hugepool/foo)
> -------------------------------------------------
> HugePages_Total:       1
> HugePages_Free:        0
> HugePages_Rsvd:    18446744073709551615
> HugePages_Surp:        0
> Hugepagesize:       2048 kB
>
> I am still confused about how your test maintains a reserve count after
> poisoning.  It may be a good idea for you to test my patch with your
> test scenario as I can not recreate here.

Interestingly, I found that this reproduces if all hugetlb pages are
reserved when poisoning.
Your testing meets the condition, and mine doesn't.

In gather_surplus_pages() we determine whether we extend hugetlb pool
with surplus pages like below:

    needed = (h->resv_huge_pages + delta) - h->free_huge_pages;
    if (needed <= 0) {
            h->resv_huge_pages += delta;
            return 0;
    }
    ...

needed is 1 if h->resv_huge_pages == h->free_huge_pages, and then
the reserve count gets inconsistent.
I confirmed that your patch fixes the issue, so I'm OK with it.

Acked-by: Naoya Horiguchi <n-horiguchi@xxxxxxxxxxxxx>

Thanks,
Naoya Horiguchi