On Fri, Dec 11, 2020 at 9:36 PM Oscar Salvador <osalvador@xxxxxxx> wrote:
>
> 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.
It's better than mine. Thank you.
>
> 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.
Will do. Thank you.
>
> > + * 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?
>
Either is OK. The compiler will help us optimize the code when
free_vmemmap_pages_per_hpage always returns false.
Thanks for your suggestions. :-)
>
> --
> Oscar Salvador
> SUSE L3
--
Yours,
Muchun
