Re: [External] Re: [PATCH v18 5/9] mm: hugetlb: set the PageHWPoison to the raw error page

[Muchun Song]

On Wed, Mar 10, 2021 at 11:28 PM Michal Hocko <mhocko@xxxxxxxx> wrote:
>
> On Mon 08-03-21 18:28:03, Muchun Song wrote:
> > Because we reuse the first tail vmemmap page frame and remap it
> > with read-only, we cannot set the PageHWPosion on some tail pages.
> > So we can use the head[4].private (There are at least 128 struct
> > page structures associated with the optimized HugeTLB page, so
> > using head[4].private is safe) to record the real error page index
> > and set the raw error page PageHWPoison later.
>
> Can we have more poisoned tail pages? Also who does consume that index
> and set the HWPoison on the proper tail page?

Good point. I look at the routine of memory failure closely.
If we do not clear the HWPoison of the head page, we cannot
poison another tail page.

So we should not set the destructor of the huge page from
HUGETLB_PAGE_DTOR to NULL_COMPOUND_DTOR
before calling alloc_huge_page_vmemmap(). In this case,
the below check of PageHuge() always returns true.

I need to fix this in the previous patch.

memory_failure()
    if (PageHuge(page))
        memory_failure_hugetlb()
            head = compound_head(page)
            if (TestSetPageHWPoison(head))
                return

Thanks.

>
> > Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> > Reviewed-by: Oscar Salvador <osalvador@xxxxxxx>
> > Acked-by: David Rientjes <rientjes@xxxxxxxxxx>
> > Tested-by: Chen Huang <chenhuang5@xxxxxxxxxx>
> > Tested-by: Bodeddula Balasubramaniam <bodeddub@xxxxxxxxxx>
> > ---
> >  mm/hugetlb.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 72 insertions(+), 8 deletions(-)
> >
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 377e0c1b283f..c0c1b7635ca9 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -1304,6 +1304,74 @@ static inline void destroy_compound_gigantic_page(struct page *page,
> >                                               unsigned int order) { }
> >  #endif
> >
> > +#ifdef CONFIG_HUGETLB_PAGE_FREE_VMEMMAP
> > +static inline void hwpoison_subpage_deliver(struct hstate *h, struct page *head)
> > +{
> > +     struct page *page;
> > +
> > +     if (!PageHWPoison(head) || !free_vmemmap_pages_per_hpage(h))
> > +             return;
> > +
> > +     page = head + page_private(head + 4);
> > +
> > +     /*
> > +      * Move PageHWPoison flag from head page to the raw error page,
> > +      * which makes any subpages rather than the error page reusable.
> > +      */
> > +     if (page != head) {
> > +             SetPageHWPoison(page);
> > +             ClearPageHWPoison(head);
> > +     }
> > +}
> > +
> > +static inline void hwpoison_subpage_set(struct hstate *h, struct page *head,
> > +                                     struct page *page)
> > +{
> > +     if (!PageHWPoison(head))
> > +             return;
> > +
> > +     if (free_vmemmap_pages_per_hpage(h)) {
> > +             set_page_private(head + 4, page - head);
> > +     } else if (page != head) {
> > +             /*
> > +              * Move PageHWPoison flag from head page to the raw error page,
> > +              * which makes any subpages rather than the error page reusable.
> > +              */
> > +             SetPageHWPoison(page);
> > +             ClearPageHWPoison(head);
> > +     }
> > +}
> > +
> > +static inline void hwpoison_subpage_clear(struct hstate *h, struct page *head)
> > +{
> > +     if (!PageHWPoison(head) || !free_vmemmap_pages_per_hpage(h))
> > +             return;
> > +
> > +     set_page_private(head + 4, 0);
> > +}
> > +#else
> > +static inline void hwpoison_subpage_deliver(struct hstate *h, struct page *head)
> > +{
> > +}
> > +
> > +static inline void hwpoison_subpage_set(struct hstate *h, struct page *head,
> > +                                     struct page *page)
> > +{
> > +     if (PageHWPoison(head) && page != head) {
> > +             /*
> > +              * Move PageHWPoison flag from head page to the raw error page,
> > +              * which makes any subpages rather than the error page reusable.
> > +              */
> > +             SetPageHWPoison(page);
> > +             ClearPageHWPoison(head);
> > +     }
> > +}
> > +
> > +static inline void hwpoison_subpage_clear(struct hstate *h, struct page *head)
> > +{
> > +}
> > +#endif
> > +
> >  static int update_and_free_page(struct hstate *h, struct page *page)
> >       __releases(&hugetlb_lock) __acquires(&hugetlb_lock)
> >  {
> > @@ -1357,6 +1425,8 @@ static int update_and_free_page(struct hstate *h, struct page *page)
> >               return -ENOMEM;
> >       }
> >
> > +     hwpoison_subpage_deliver(h, page);
> > +
> >       for (i = 0; i < pages_per_huge_page(h);
> >            i++, subpage = mem_map_next(subpage, page, i)) {
> >               subpage->flags &= ~(1 << PG_locked | 1 << PG_error |
> > @@ -1801,14 +1871,7 @@ int dissolve_free_huge_page(struct page *page)
> >                       goto retry;
> >               }
> >
> > -             /*
> > -              * Move PageHWPoison flag from head page to the raw error page,
> > -              * which makes any subpages rather than the error page reusable.
> > -              */
> > -             if (PageHWPoison(head) && page != head) {
> > -                     SetPageHWPoison(page);
> > -                     ClearPageHWPoison(head);
> > -             }
> > +             hwpoison_subpage_set(h, head, page);
> >               list_del(&head->lru);
> >               h->free_huge_pages--;
> >               h->free_huge_pages_node[nid]--;
> > @@ -1818,6 +1881,7 @@ int dissolve_free_huge_page(struct page *page)
> >                       h->surplus_huge_pages--;
> >                       h->surplus_huge_pages_node[nid]--;
> >                       h->max_huge_pages++;
> > +                     hwpoison_subpage_clear(h, head);
> >               }
> >       }
> >  out:
> > --
> > 2.11.0
> >
>
> --
> Michal Hocko
> SUSE Labs