On Wed, Jul 13, 2022 at 12:51:30PM +0800, Miaohe Lin wrote: > On 2022/7/12 11:28, Naoya Horiguchi wrote: > > From: Naoya Horiguchi <naoya.horiguchi@xxxxxxx> > > > > When handling memory error on a hugetlb page, the error handler tries to > > dissolve and turn it into 4kB pages. If it's successfully dissolved, > > PageHWPoison flag is moved to the raw error page, so that's all right. > > However, dissolve sometimes fails, then the error page is left as > > hwpoisoned hugepage. It's useful if we can retry to dissolve it to save > > healthy pages, but that's not possible now because the information about > > where the raw error pages is lost. > > > > Use the private field of a few tail pages to keep that information. The > > code path of shrinking hugepage pool uses this info to try delayed dissolve. > > In order to remember multiple errors in a hugepage, a singly-linked list > > originated from SUBPAGE_INDEX_HWPOISON-th tail page is constructed. Only > > simple operations (adding an entry or clearing all) are required and the > > list is assumed not to be very long, so this simple data structure should > > be enough. > > > > If we failed to save raw error info, the hwpoison hugepage has errors on > > unknown subpage, then this new saving mechanism does not work any more, > > so disable saving new raw error info and freeing hwpoison hugepages. > > > > Signed-off-by: Naoya Horiguchi <naoya.horiguchi@xxxxxxx> > > Reported-by: kernel test robot <lkp@xxxxxxxxx> > > This patch looks good to me with some nits below. > > > --- > ... > > +static int hugetlb_set_page_hwpoison(struct page *hpage, struct page *page) > > +{ > > + struct llist_head *head; > > + struct raw_hwp_page *raw_hwp; > > + struct llist_node *t, *tnode; > > + int ret = TestSetPageHWPoison(hpage) ? -EHWPOISON : 0; > > + > > + /* > > + * Once the hwpoison hugepage has lost reliable raw error info, > > + * there is little meaning to keep additional error info precisely, > > + * so skip to add additional raw error info. > > + */ > > + if (HPageRawHwpUnreliable(hpage)) > > + return -EHWPOISON; > > + head = raw_hwp_list_head(hpage); > > + llist_for_each_safe(tnode, t, head->first) { > > + struct raw_hwp_page *p = container_of(tnode, struct raw_hwp_page, node); > > + > > + if (p->page == page) > > + return -EHWPOISON; > > + } > > + > > + raw_hwp = kmalloc(sizeof(struct raw_hwp_page), GFP_ATOMIC); > > + if (raw_hwp) { > > + raw_hwp->page = page; > > + llist_add(&raw_hwp->node, head); > > + /* the first error event will be counted in action_result(). */ > > + if (ret) > > + num_poisoned_pages_inc(); > > + } else { > > + /* > > + * Failed to save raw error info. We no longer trace all > > + * hwpoisoned subpages, and we need refuse to free/dissolve > > + * this hwpoisoned hugepage. > > + */ > > + SetHPageRawHwpUnreliable(hpage); > > IMHO, when HPageRawHwpUnreliable is set, we can simply free the raw_hwp_list here > to save some memory as they're not used anymore. Yes, that's possible. Let's do this. > > > + } > > + return ret; > > +} > > + > > +void hugetlb_clear_page_hwpoison(struct page *hpage) > > +{ > > + struct llist_head *head; > > + struct llist_node *t, *tnode; > > + > > + if (!HPageRawHwpUnreliable(hpage)) > > + ClearPageHWPoison(hpage); > > + head = raw_hwp_list_head(hpage); > > + llist_for_each_safe(tnode, t, head->first) { > > + struct raw_hwp_page *p = container_of(tnode, struct raw_hwp_page, node); > > + > > + SetPageHWPoison(p->page); > > + kfree(p); > > + } > > + llist_del_all(head); > > +} > > + > > /* > > * Called from hugetlb code with hugetlb_lock held. > > * > > @@ -1698,7 +1771,7 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags) > > goto out; > > } > > > > - if (TestSetPageHWPoison(head)) { > > + if (hugetlb_set_page_hwpoison(head, page)) { > > ret = -EHWPOISON; > > goto out; > > } > > @@ -1710,7 +1783,6 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags) > > return ret; > > } > > > > -#ifdef CONFIG_HUGETLB_PAGE > > /* > > * Taking refcount of hugetlb pages needs extra care about race conditions > > * with basic operations like hugepage allocation/free/demotion. > > @@ -1751,7 +1823,7 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb > > lock_page(head); > > > > if (hwpoison_filter(p)) { > > - ClearPageHWPoison(head); > > + hugetlb_clear_page_hwpoison(head); > > If the code reach here, the hugetlb page should be poisoned first time or it will return > in above "res == -EHWPOISON" case. So ClearPageHWPoison should be fine here. But this change > will do the same thing while making the code more consistent. So it should be fine. :) Right, this change is for consistency. > > > res = -EOPNOTSUPP; > > goto out; > > } > > > > Anyway, for what it worth, > > Reviewed-by: Miaohe Lin <linmiaohe@xxxxxxxxxx> > > Thanks! Thank you. - Naoya Horiguchi