Re: [External] Re: [PATCH v16 4/9] mm: hugetlb: alloc the vmemmap pages associated with each HugeTLB page

[Oscar Salvador]

On Tue, Feb 23, 2021 at 04:41:28PM +0100, Oscar Salvador wrote:
> On Tue, Feb 23, 2021 at 11:50:05AM +0100, Oscar Salvador wrote:
> > > CPU0:                           CPU1:
> > >                                 set_compound_page_dtor(HUGETLB_PAGE_DTOR);
> > > memory_failure_hugetlb
> > >   get_hwpoison_page
> > >     __get_hwpoison_page
> > >       get_page_unless_zero
> > >                                 put_page_testzero()
> > > 
> > > Maybe this can happen. But it is a very corner case. If we want to
> > > deal with this. We can put_page_testzero() first and then
> > > set_compound_page_dtor(HUGETLB_PAGE_DTOR).
> > 
> > I have to check further, but it looks like this could actually happen.
> > Handling this with VM_BUG_ON is wrong, because memory_failure/soft_offline are
> > entitled to increase the refcount of the page.
> > 
> > AFAICS,
> > 
> >  CPU0:                                    CPU1:
> >                                           set_compound_page_dtor(HUGETLB_PAGE_DTOR);
> >  memory_failure_hugetlb
> >    get_hwpoison_page
> >      __get_hwpoison_page
> >        get_page_unless_zero
> >                                           put_page_testzero()
> >         identify_page_state
> >          me_huge_page
> > 
> > I think we can reach me_huge_page with either refcount = 1 or refcount =2,
> > depending whether put_page_testzero has been issued.
> > 
> > For now, I would not re-enqueue the page if put_page_testzero == false.
> > I have to see how this can be handled gracefully.
> 
> I took a brief look.
> It is not really your patch fault. Hugetlb <-> memory-failure synchronization is
> a bit odd, it definitely needs improvment.
> 
> The thing is, we can have different scenarios here.
> E.g: by the time we return from put_page_testzero, we might have refcount ==
> 0 and PageHWPoison, or refcount == 1 PageHWPoison.
> 
> The former will let a user get a page from the pool and get a sigbus
> when it faults in the page, and the latter will be even more odd as we
> will have a self-refcounted page in the free pool (and hwpoisoned).
> 
> As I said, it is not this patchset fault. I just made me realize this
> problem.
> 
> I have to think some more about this.

I have been thinking more about this.
memory failure events can occur at any time, and we might not be in a
position where we can handle gracefully the error, meaning that the page
might end up in non desirable state.

E.g: we could flag the page right before enqueing it.

I still think that VM_BUG_ON should go, as the refcount can be perfectly
increased by memory-failure/soft_offline handlers, so BUGing there does
not make much sense.

One think we could do is to check the state of the page we want to
retrieve from the free hugepage pool.
We should discard any HWpoisoned ones, and dissolve them.

The thing is, memory-failure/soft_offline should allocate a new hugepage
for the free pool, so keep the pool stable.
Something like [1].

Anyway, this is orthogonal to this patch, and something I will work on
soon.

[1] https://lore.kernel.org/linux-mm/20210222135137.25717-2-osalvador@xxxxxxx/T/#u

-- 
Oscar Salvador
SUSE L3