[v3,2/4] mm/hwpoison: check if a subpage of a hugetlb folio is raw HWPOISON

  Add the functionality, is_raw_hwp_subpage, to tell if a subpage of a
hugetlb folio is a raw HWPOISON page. This functionality relies on
RawHwpUnreliable to be not set; otherwise hugepage's raw HWPOISON list
becomes meaningless.

is_raw_hwp_subpage needs to hold hugetlb_lock in order to synchronize
with __get_huge_page_for_hwpoison, who iterates and inserts an entry to
raw_hwp_list. llist itself doesn't ensure insertion is synchornized with
the iterating used by __is_raw_hwp_list. Caller can minimize the
overhead of lock cycles by first checking if folio / head page's
HWPOISON flag is set.

Exports this functionality to be immediately used in the read operation
for hugetlbfs.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
---
 include/linux/hugetlb.h | 19 +++++++++++++++++++
 include/linux/mm.h      |  7 +++++++
 mm/hugetlb.c            | 10 ++++++++++
 mm/memory-failure.c     | 34 ++++++++++++++++++++++++----------
 4 files changed, 60 insertions(+), 10 deletions(-)
  

On Fri, Jul 07, 2023 at 08:19:02PM +0000, Jiaqi Yan wrote:
> Add the functionality, is_raw_hwp_subpage, to tell if a subpage of a

This is incorrect naming.  "subpage" was needed before we had the
folio concept, but it should not be used any more.  We have folios
and pages now.

Also, abbreviating "hwpoison" as "hwp" seems like a bad idea to me.
hwp is already used as an acronym by acpi, intel_pstate, some clock
drivers, an ethernet driver, and a scsi driver.
  

On Fri, 7 Jul 2023 21:31:39 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Fri, Jul 07, 2023 at 08:19:02PM +0000, Jiaqi Yan wrote:
> > Add the functionality, is_raw_hwp_subpage, to tell if a subpage of a
> 
> This is incorrect naming.  "subpage" was needed before we had the
> folio concept, but it should not be used any more.  We have folios
> and pages now.
> 
> Also, abbreviating "hwpoison" as "hwp" seems like a bad idea to me.
> hwp is already used as an acronym by acpi, intel_pstate, some clock
> drivers, an ethernet driver, and a scsi driver.

Thanks, Matthew.  I'll merge v3 so we hit next -next (which actually
won't be until Monday), and add a note that v4 is expected.
  

On Fri, Jul 07, 2023 at 09:31:39PM +0100, Matthew Wilcox wrote:
> On Fri, Jul 07, 2023 at 08:19:02PM +0000, Jiaqi Yan wrote:
> > Add the functionality, is_raw_hwp_subpage, to tell if a subpage of a
> 
> This is incorrect naming.  "subpage" was needed before we had the
> folio concept, but it should not be used any more.  We have folios
> and pages now.

I think we can address the raw hwpoison page by the offset in folio/hugepage
to eliminate the concept of "subpage".

> 
> Also, abbreviating "hwpoison" as "hwp" seems like a bad idea to me.
> hwp is already used as an acronym by acpi, intel_pstate, some clock
> drivers, an ethernet driver, and a scsi driver.

I originally introduced the abbreviation "hwp" to avoid using a lengthy
function name such as "folio_test_hugetlb_raw_hwpoison_unreliable()."
Therefore, I prefer using "rawhwp" instead of a longer form like
"raw_hwpoison," although I don't expect any confusion between "hwp" and
"rawhwp."
As for "hwp_walk", another user of "hwp" in in mm/memory-failure.c,
we can easily substitute it with "hwpoison_walk."

Thanks,
Naoya Horiguchi
  

On Sun, Jul 9, 2023 at 5:21 PM Naoya Horiguchi
<naoya.horiguchi@linux.dev> wrote:
>
> On Fri, Jul 07, 2023 at 09:31:39PM +0100, Matthew Wilcox wrote:
> > On Fri, Jul 07, 2023 at 08:19:02PM +0000, Jiaqi Yan wrote:
> > > Add the functionality, is_raw_hwp_subpage, to tell if a subpage of a
> >
> > This is incorrect naming.  "subpage" was needed before we had the
> > folio concept, but it should not be used any more.  We have folios
> > and pages now.
>

Thanks for your comment, Matthew.

> I think we can address the raw hwpoison page by the offset in folio/hugepage
> to eliminate the concept of "subpage".
>
> >
> > Also, abbreviating "hwpoison" as "hwp" seems like a bad idea to me.
> > hwp is already used as an acronym by acpi, intel_pstate, some clock
> > drivers, an ethernet driver, and a scsi driver.
>
> I originally introduced the abbreviation "hwp" to avoid using a lengthy
> function name such as "folio_test_hugetlb_raw_hwpoison_unreliable()."
> Therefore, I prefer using "rawhwp" instead of a longer form like
> "raw_hwpoison," although I don't expect any confusion between "hwp" and
> "rawhwp."

These are names in my mind, what do you think?
* is_rawhwp_page_in_hugepage
* is_raw_hwpoison_page_in_hugepage // I prefer this one
* folio_test_hugetlb_raw_hwpoison_page

> As for "hwp_walk", another user of "hwp" in in mm/memory-failure.c,
> we can easily substitute it with "hwpoison_walk."

In this "hwp_walk" case, I also prefer "hwpoison" than "hwp". I can
create a separate renaming patch.

>
> Thanks,
> Naoya Horiguchi
  

On Fri, Jul 7, 2023 at 7:57 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
>
> On 2023/7/8 4:19, Jiaqi Yan wrote:
> > Add the functionality, is_raw_hwp_subpage, to tell if a subpage of a
> > hugetlb folio is a raw HWPOISON page. This functionality relies on
> > RawHwpUnreliable to be not set; otherwise hugepage's raw HWPOISON list
> > becomes meaningless.
> >
> > is_raw_hwp_subpage needs to hold hugetlb_lock in order to synchronize
> > with __get_huge_page_for_hwpoison, who iterates and inserts an entry to
> > raw_hwp_list. llist itself doesn't ensure insertion is synchornized with
> > the iterating used by __is_raw_hwp_list. Caller can minimize the
> > overhead of lock cycles by first checking if folio / head page's
> > HWPOISON flag is set.
> >
> > Exports this functionality to be immediately used in the read operation
> > for hugetlbfs.
> >
> > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
> > Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> > Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
> > ---
> >  include/linux/hugetlb.h | 19 +++++++++++++++++++
> >  include/linux/mm.h      |  7 +++++++
> >  mm/hugetlb.c            | 10 ++++++++++
> >  mm/memory-failure.c     | 34 ++++++++++++++++++++++++----------
> >  4 files changed, 60 insertions(+), 10 deletions(-)
> > ...
> > -static inline struct llist_head *raw_hwp_list_head(struct folio *folio)
> > +bool __is_raw_hwp_subpage(struct folio *folio, struct page *subpage)
> >  {
> > -     return (struct llist_head *)&folio->_hugetlb_hwpoison;
> > +     struct llist_head *raw_hwp_head;
> > +     struct raw_hwp_page *p, *tmp;
> > +     bool ret = false;
> > +
> > +     if (!folio_test_hwpoison(folio))
> > +             return false;
> > +
> > +     /*
> > +      * When RawHwpUnreliable is set, kernel lost track of which subpages
> > +      * are HWPOISON. So return as if ALL subpages are HWPOISONed.
> > +      */
> > +     if (folio_test_hugetlb_raw_hwp_unreliable(folio))
> > +             return true;
> > +
> > +     raw_hwp_head = raw_hwp_list_head(folio);
> > +     llist_for_each_entry_safe(p, tmp, raw_hwp_head->first, node) {
>
> Since we don't free the raw_hwp_list, does llist_for_each_entry works same as llist_for_each_entry_safe?
>
> > +             if (subpage == p->page) {
> > +                     ret = true;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     return ret;
> >  }
>
> It seems there's a race between __is_raw_hwp_subpage and unpoison_memory:
>   unpoison_memory               __is_raw_hwp_subpage
>                                   if (!folio_test_hwpoison(folio)) -- hwpoison is set
>     folio_free_raw_hwp            llist_for_each_entry_safe raw_hwp_list
>       llist_del_all                 ..
>     folio_test_clear_hwpoison
>

Thanks Miaohe for raising this concern.

> But __is_raw_hwp_subpage is used in hugetlbfs, unpoison_memory couldn't reach here because there's a
> folio_mapping == NULL check before folio_free_raw_hwp.

I agree. But in near future I do want to make __is_raw_hwp_subpage
work for shared-mapping hugetlb, so it would be nice to work with
unpoison_memory. It doesn't seem to me that holding mf_mutex in
__is_raw_hwp_subpage is nice or even absolutely correct. Let me think
if I can come up with something in v4.

>
> Anyway, this patch looks good to me.
>
> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
> Thanks.
>
  

On Mon, Jul 10, 2023 at 08:11:48AM -0700, Jiaqi Yan wrote:
> On Sun, Jul 9, 2023 at 5:21 PM Naoya Horiguchi
> <naoya.horiguchi@linux.dev> wrote:
> >
> > On Fri, Jul 07, 2023 at 09:31:39PM +0100, Matthew Wilcox wrote:
> > > On Fri, Jul 07, 2023 at 08:19:02PM +0000, Jiaqi Yan wrote:
> > > > Add the functionality, is_raw_hwp_subpage, to tell if a subpage of a
> > >
> > > This is incorrect naming.  "subpage" was needed before we had the
> > > folio concept, but it should not be used any more.  We have folios
> > > and pages now.
> >
> 
> Thanks for your comment, Matthew.
> 
> > I think we can address the raw hwpoison page by the offset in folio/hugepage
> > to eliminate the concept of "subpage".
> >
> > >
> > > Also, abbreviating "hwpoison" as "hwp" seems like a bad idea to me.
> > > hwp is already used as an acronym by acpi, intel_pstate, some clock
> > > drivers, an ethernet driver, and a scsi driver.
> >
> > I originally introduced the abbreviation "hwp" to avoid using a lengthy
> > function name such as "folio_test_hugetlb_raw_hwpoison_unreliable()."
> > Therefore, I prefer using "rawhwp" instead of a longer form like
> > "raw_hwpoison," although I don't expect any confusion between "hwp" and
> > "rawhwp."
> 
> These are names in my mind, what do you think?
> * is_rawhwp_page_in_hugepage
> * is_raw_hwpoison_page_in_hugepage // I prefer this one

This one is fine to me.

> * folio_test_hugetlb_raw_hwpoison_page
> 
> > As for "hwp_walk", another user of "hwp" in in mm/memory-failure.c,
> > we can easily substitute it with "hwpoison_walk."
> 
> In this "hwp_walk" case, I also prefer "hwpoison" than "hwp". I can
> create a separate renaming patch.

Great, thank you.

- Naoya Horiguchi
  

On Mon, Jul 10, 2023 at 8:16 AM Jiaqi Yan <jiaqiyan@google.com> wrote:
>
> On Fri, Jul 7, 2023 at 7:57 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
> >
> > On 2023/7/8 4:19, Jiaqi Yan wrote:
> > > Add the functionality, is_raw_hwp_subpage, to tell if a subpage of a
> > > hugetlb folio is a raw HWPOISON page. This functionality relies on
> > > RawHwpUnreliable to be not set; otherwise hugepage's raw HWPOISON list
> > > becomes meaningless.
> > >
> > > is_raw_hwp_subpage needs to hold hugetlb_lock in order to synchronize
> > > with __get_huge_page_for_hwpoison, who iterates and inserts an entry to
> > > raw_hwp_list. llist itself doesn't ensure insertion is synchornized with
> > > the iterating used by __is_raw_hwp_list. Caller can minimize the
> > > overhead of lock cycles by first checking if folio / head page's
> > > HWPOISON flag is set.
> > >
> > > Exports this functionality to be immediately used in the read operation
> > > for hugetlbfs.
> > >
> > > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
> > > Reviewed-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> > > Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>
> > > ---
> > >  include/linux/hugetlb.h | 19 +++++++++++++++++++
> > >  include/linux/mm.h      |  7 +++++++
> > >  mm/hugetlb.c            | 10 ++++++++++
> > >  mm/memory-failure.c     | 34 ++++++++++++++++++++++++----------
> > >  4 files changed, 60 insertions(+), 10 deletions(-)
> > > ...
> > > -static inline struct llist_head *raw_hwp_list_head(struct folio *folio)
> > > +bool __is_raw_hwp_subpage(struct folio *folio, struct page *subpage)
> > >  {
> > > -     return (struct llist_head *)&folio->_hugetlb_hwpoison;
> > > +     struct llist_head *raw_hwp_head;
> > > +     struct raw_hwp_page *p, *tmp;
> > > +     bool ret = false;
> > > +
> > > +     if (!folio_test_hwpoison(folio))
> > > +             return false;
> > > +
> > > +     /*
> > > +      * When RawHwpUnreliable is set, kernel lost track of which subpages
> > > +      * are HWPOISON. So return as if ALL subpages are HWPOISONed.
> > > +      */
> > > +     if (folio_test_hugetlb_raw_hwp_unreliable(folio))
> > > +             return true;
> > > +
> > > +     raw_hwp_head = raw_hwp_list_head(folio);
> > > +     llist_for_each_entry_safe(p, tmp, raw_hwp_head->first, node) {
> >
> > Since we don't free the raw_hwp_list, does llist_for_each_entry works same as llist_for_each_entry_safe?

Sorry I missed this comment. Yes they are the same but
llist_for_each_entry doesn't need "tmp". I will switch to
llist_for_each_entry in v4.

>
> >
> > > +             if (subpage == p->page) {
> > > +                     ret = true;
> > > +                     break;
> > > +             }
> > > +     }
> > > +
> > > +     return ret;
> > >  }
> >
> > It seems there's a race between __is_raw_hwp_subpage and unpoison_memory:
> >   unpoison_memory               __is_raw_hwp_subpage
> >                                   if (!folio_test_hwpoison(folio)) -- hwpoison is set
> >     folio_free_raw_hwp            llist_for_each_entry_safe raw_hwp_list
> >       llist_del_all                 ..
> >     folio_test_clear_hwpoison
> >
>
> Thanks Miaohe for raising this concern.
>
> > But __is_raw_hwp_subpage is used in hugetlbfs, unpoison_memory couldn't reach here because there's a
> > folio_mapping == NULL check before folio_free_raw_hwp.
>
> I agree. But in near future I do want to make __is_raw_hwp_subpage
> work for shared-mapping hugetlb, so it would be nice to work with
> unpoison_memory. It doesn't seem to me that holding mf_mutex in
> __is_raw_hwp_subpage is nice or even absolutely correct. Let me think
> if I can come up with something in v4.

At my 2nd thought, if __is_raw_hwp_subpage simply takes mf_mutex
before llist_for_each_entry, it will introduce a deadlock:

unpoison_memory                       __is_raw_hwp_subpage
  held mf_mutex                         held hugetlb_lock
  get_hwpoison_hugetlb_folio            attempts mf_mutex
    attempts hugetlb lock

Not for this patch series, but for future, is it a good idea to make
mf_mutex available to hugetlb code? Then enforce the order of locking
to be mf_mutex first, hugetlb_lock second? I believe this is the
current locking pattern / order for try_memory_failure_hugetlb.


>
> >
> > Anyway, this patch looks good to me.
> >
> > Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
> > Thanks.
> >
  

On 07/11/23 10:05, Jiaqi Yan wrote:
> On Mon, Jul 10, 2023 at 8:16 AM Jiaqi Yan <jiaqiyan@google.com> wrote:
> > On Fri, Jul 7, 2023 at 7:57 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
> > > On 2023/7/8 4:19, Jiaqi Yan wrote:
> > >
> > > > +             if (subpage == p->page) {
> > > > +                     ret = true;
> > > > +                     break;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     return ret;
> > > >  }
> > >
> > > It seems there's a race between __is_raw_hwp_subpage and unpoison_memory:
> > >   unpoison_memory               __is_raw_hwp_subpage
> > >                                   if (!folio_test_hwpoison(folio)) -- hwpoison is set
> > >     folio_free_raw_hwp            llist_for_each_entry_safe raw_hwp_list
> > >       llist_del_all                 ..
> > >     folio_test_clear_hwpoison
> > >
> >
> > Thanks Miaohe for raising this concern.
> >
> > > But __is_raw_hwp_subpage is used in hugetlbfs, unpoison_memory couldn't reach here because there's a
> > > folio_mapping == NULL check before folio_free_raw_hwp.
> >
> > I agree. But in near future I do want to make __is_raw_hwp_subpage
> > work for shared-mapping hugetlb, so it would be nice to work with
> > unpoison_memory. It doesn't seem to me that holding mf_mutex in
> > __is_raw_hwp_subpage is nice or even absolutely correct. Let me think
> > if I can come up with something in v4.
> 
> At my 2nd thought, if __is_raw_hwp_subpage simply takes mf_mutex
> before llist_for_each_entry, it will introduce a deadlock:
> 
> unpoison_memory                       __is_raw_hwp_subpage
>   held mf_mutex                         held hugetlb_lock
>   get_hwpoison_hugetlb_folio            attempts mf_mutex
>     attempts hugetlb lock
> 
> Not for this patch series, but for future, is it a good idea to make
> mf_mutex available to hugetlb code? Then enforce the order of locking
> to be mf_mutex first, hugetlb_lock second? I believe this is the
> current locking pattern / order for try_memory_failure_hugetlb.

I think only holding mf_mutex in __is_raw_hwp_subpage would be sufficient
to prevent races with unpoison_memory.  memory failure code needs to take
both mf_mutex and hugetlb_lock.  The hugetlb lock is to prevent hugetlb
page state changes.  IIUC, __is_raw_hwp_subpage is only taking hugetlb_lock
to prevent races with memory failure code.

Of course, I could be missing something as there are subtle issues with
locking in the memory failure code.
  

On Tue, Jul 11, 2023 at 11:02 AM Mike Kravetz <mike.kravetz@oracle.com> wrote:
>
> On 07/11/23 10:05, Jiaqi Yan wrote:
> > On Mon, Jul 10, 2023 at 8:16 AM Jiaqi Yan <jiaqiyan@google.com> wrote:
> > > On Fri, Jul 7, 2023 at 7:57 PM Miaohe Lin <linmiaohe@huawei.com> wrote:
> > > > On 2023/7/8 4:19, Jiaqi Yan wrote:
> > > >
> > > > > +             if (subpage == p->page) {
> > > > > +                     ret = true;
> > > > > +                     break;
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     return ret;
> > > > >  }
> > > >
> > > > It seems there's a race between __is_raw_hwp_subpage and unpoison_memory:
> > > >   unpoison_memory               __is_raw_hwp_subpage
> > > >                                   if (!folio_test_hwpoison(folio)) -- hwpoison is set
> > > >     folio_free_raw_hwp            llist_for_each_entry_safe raw_hwp_list
> > > >       llist_del_all                 ..
> > > >     folio_test_clear_hwpoison
> > > >
> > >
> > > Thanks Miaohe for raising this concern.
> > >
> > > > But __is_raw_hwp_subpage is used in hugetlbfs, unpoison_memory couldn't reach here because there's a
> > > > folio_mapping == NULL check before folio_free_raw_hwp.
> > >
> > > I agree. But in near future I do want to make __is_raw_hwp_subpage
> > > work for shared-mapping hugetlb, so it would be nice to work with
> > > unpoison_memory. It doesn't seem to me that holding mf_mutex in
> > > __is_raw_hwp_subpage is nice or even absolutely correct. Let me think
> > > if I can come up with something in v4.
> >
> > At my 2nd thought, if __is_raw_hwp_subpage simply takes mf_mutex
> > before llist_for_each_entry, it will introduce a deadlock:
> >
> > unpoison_memory                       __is_raw_hwp_subpage
> >   held mf_mutex                         held hugetlb_lock
> >   get_hwpoison_hugetlb_folio            attempts mf_mutex
> >     attempts hugetlb lock
> >
> > Not for this patch series, but for future, is it a good idea to make
> > mf_mutex available to hugetlb code? Then enforce the order of locking
> > to be mf_mutex first, hugetlb_lock second? I believe this is the
> > current locking pattern / order for try_memory_failure_hugetlb.
>
> I think only holding mf_mutex in __is_raw_hwp_subpage would be sufficient
> to prevent races with unpoison_memory.  memory failure code needs to take
> both mf_mutex and hugetlb_lock.  The hugetlb lock is to prevent hugetlb
> page state changes.  IIUC, __is_raw_hwp_subpage is only taking hugetlb_lock
> to prevent races with memory failure code.

Thanks Mike, I think mf_mutex is a simple and correct solution. I will
drop hugetlb_lock from __is_raw_hwp_subpage in v4.

>
> Of course, I could be missing something as there are subtle issues with
> locking in the memory failure code.
> --
> Mike Kravetz