Messages in this thread | | | Date | Fri, 11 Dec 2020 14:36:29 +0100 | From | Oscar Salvador <> | Subject | Re: [PATCH v8 07/12] mm/hugetlb: Set the PageHWPoison to the raw error page |
| |
On Thu, Dec 10, 2020 at 11:55:21AM +0800, Muchun Song wrote: > +static inline void subpage_hwpoison_deliver(struct hstate *h, struct page *head) > +{ > + struct page *page = head; > + > + if (!free_vmemmap_pages_per_hpage(h)) > + return; > + > + if (PageHWPoison(head)) > + 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); > + } > +}
I would make the names coherent. I am not definitely goot at names, but something like: hwpoison_subpage_{foo,bar} looks better.
Also, could not subpage_hwpoison_deliver be rewritten like:
static inline void subpage_hwpoison_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); } }
I think it is better code-wise.
> + * 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); > + }
I would put this in an else-if above:
if (free_vmemmap_pages_per_hpage(h)) { set_page_private(head + 4, page - head); return; } else if (page != head) { SetPageHWPoison(page); ClearPageHWPoison(head); }
or will we lose the optimization in case free_vmemmap_pages_per_hpage gets compiled out?
-- Oscar Salvador SUSE L3
| |