On 2022/10/7 9:07, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
>
> HWPoisoned page is not supposed to be accessed once marked, but currently
> such accesses can happen during memory hotremove because do_migrate_range()
> can be called before dissolve_free_huge_pages() is called.
>
> Clear HPageMigratable for hwpoisoned hugepages to prevent them from being
> migrated. This should be done in hugetlb_lock to avoid race against
> isolate_hugetlb().
>
> get_hwpoison_huge_page() needs to have a flag to show it's called from
> unpoison to take refcount of hwpoisoned hugepages, so add it.
>
> Reported-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
Sorry for late respond. I was spending a busy week. :) And thanks for your work, Naoya.
> ---
> ChangeLog v3 -> v6:
> - introduce migratable_cleared to remember that HPageMigratable is
> cleared in error handling. It's needed to cancel when an error event
> is filtered by hwpoison_filter(). (Thanks to Miaohe)
>
> ChangeLog v2 -> v3
> - move to the approach of clearing HPageMigratable instead of shifting
> dissolve_free_huge_pages.
> ---
> include/linux/hugetlb.h | 10 ++++++----
> include/linux/mm.h | 6 ++++--
> mm/hugetlb.c | 9 +++++----
> mm/memory-failure.c | 21 +++++++++++++++++----
> 4 files changed, 32 insertions(+), 14 deletions(-)
>
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 32d45e96a894..19b99ff7fea0 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -183,8 +183,9 @@ bool hugetlb_reserve_pages(struct inode *inode, long from, long to,
> long hugetlb_unreserve_pages(struct inode *inode, long start, long end,
> long freed);
> int isolate_hugetlb(struct page *page, struct list_head *list);
> -int get_hwpoison_huge_page(struct page *page, bool *hugetlb);
> -int get_huge_page_for_hwpoison(unsigned long pfn, int flags);
> +int get_hwpoison_huge_page(struct page *page, bool *hugetlb, bool unpoison);
> +int get_huge_page_for_hwpoison(unsigned long pfn, int flags,
> + bool *migratable_cleared);
> void putback_active_hugepage(struct page *page);
> void move_hugetlb_state(struct page *oldpage, struct page *newpage, int reason);
> void free_huge_page(struct page *page);
> @@ -391,12 +392,13 @@ static inline int isolate_hugetlb(struct page *page, struct list_head *list)
> return -EBUSY;
> }
>
> -static inline int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
> +static inline int get_hwpoison_huge_page(struct page *page, bool *hugetlb, bool unpoison)
> {
> return 0;
> }
>
> -static inline int get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +static inline int get_huge_page_for_hwpoison(unsigned long pfn, int flags,
> + bool *migratable_cleared)
> {
> return 0;
> }
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 8bbcccbc5565..3264bf993ad8 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3277,9 +3277,11 @@ extern void shake_page(struct page *p);
> extern atomic_long_t num_poisoned_pages __read_mostly;
> extern int soft_offline_page(unsigned long pfn, int flags);
> #ifdef CONFIG_MEMORY_FAILURE
> -extern int __get_huge_page_for_hwpoison(unsigned long pfn, int flags);
> +extern int __get_huge_page_for_hwpoison(unsigned long pfn, int flags,
> + bool *migratable_cleared);
> #else
> -static inline int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +static inline int __get_huge_page_for_hwpoison(unsigned long pfn, int flags,
> + bool *migratable_cleared)
> {
> return 0;
> }
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 63fe47a0240a..0e482dfaf92e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -7253,7 +7253,7 @@ int isolate_hugetlb(struct page *page, struct list_head *list)
> return ret;
> }
>
> -int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
> +int get_hwpoison_huge_page(struct page *page, bool *hugetlb, bool unpoison)
> {
> int ret = 0;
>
> @@ -7263,7 +7263,7 @@ int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
> *hugetlb = true;
> if (HPageFreed(page))
> ret = 0;
> - else if (HPageMigratable(page))
> + else if (HPageMigratable(page) || unpoison)
> ret = get_page_unless_zero(page);
> else
> ret = -EBUSY;
> @@ -7272,12 +7272,13 @@ int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
> return ret;
> }
>
> -int get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +int get_huge_page_for_hwpoison(unsigned long pfn, int flags,
> + bool *migratable_cleared)
> {
> int ret;
>
> spin_lock_irq(&hugetlb_lock);
> - ret = __get_huge_page_for_hwpoison(pfn, flags);
> + ret = __get_huge_page_for_hwpoison(pfn, flags, migratable_cleared);
> spin_unlock_irq(&hugetlb_lock);
> return ret;
> }
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 145bb561ddb3..d4fef56c0438 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1244,7 +1244,7 @@ static int __get_hwpoison_page(struct page *page, unsigned long flags)
> int ret = 0;
> bool hugetlb = false;
>
> - ret = get_hwpoison_huge_page(head, &hugetlb);
> + ret = get_hwpoison_huge_page(head, &hugetlb, false);
> if (hugetlb)
> return ret;
>
> @@ -1334,7 +1334,7 @@ static int __get_unpoison_page(struct page *page)
> int ret = 0;
> bool hugetlb = false;
>
> - ret = get_hwpoison_huge_page(head, &hugetlb);
> + ret = get_hwpoison_huge_page(head, &hugetlb, true);
> if (hugetlb)
> return ret;
>
> @@ -1785,7 +1785,8 @@ void hugetlb_clear_page_hwpoison(struct page *hpage)
> * -EBUSY - the hugepage is busy (try to retry)
> * -EHWPOISON - the hugepage is already hwpoisoned
> */
> -int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +int __get_huge_page_for_hwpoison(unsigned long pfn, int flags,
> + bool *migratable_cleared)
> {
> struct page *page = pfn_to_page(pfn);
> struct page *head = compound_head(page);
> @@ -1815,6 +1816,15 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> goto out;
> }
>
> + /*
> + * Clearing HPageMigratable for hwpoisoned hugepages to prevent them
> + * from being migrated by memory hotremove.
> + */
> + if (count_increased) {
> + *migratable_cleared = true;
> + ClearHPageMigratable(head);
I think I might be nitpicking... But it seems ClearHPageMigratable is not enough here.
1. In MF_COUNT_INCREASED case, we don't know whether HPageMigratable is set.
2. Even if HPageMigratable is set, there might be a race window before we clear HPageMigratable?
So "*migratable_cleared = TestClearHPageMigratable" might be better? But I might be wrong.
With above fixed (if it's really a problem), this patch looks good to me.
Reviewed-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
Thanks,
Miaohe Lin
