Re: [PATCH v3 1/4] mm/hwpoison: delete all entries before traversal in __folio_free_raw_hwp

[Miaohe Lin]

On 2023/7/8 4:19, Jiaqi Yan wrote:
> Traversal on llist (e.g. llist_for_each_safe) is only safe AFTER entries
> are deleted from the llist. Correct the way __folio_free_raw_hwp deletes
> and frees raw_hwp_page entries in raw_hwp_list: first llist_del_all, then
> kfree within llist_for_each_safe.
> 
> As of today, concurrent adding, deleting, and traversal on raw_hwp_list
> from hugetlb.c and/or memory-failure.c are fine with each other. Note

I think there's a race on freeing the raw_hwp_list between unpoison_memory and __update_and_free_hugetlb_folio:

  unpoison_memory		__update_and_free_hugetlb_folio
    				if (folio_test_hwpoison)
    	  			    folio_clear_hugetlb_hwpoison
    folio_free_raw_hwp		      folio_free_raw_hwp
    folio_test_clear_hwpoison

unpoison_memory and __update_and_free_hugetlb_folio can traverse and free the raw_hwp_list
at the same time. And I believe your patch will fix the problem. Thanks.

> this is guaranteed partly by the lock-free nature of llist, and partly
> by holding hugetlb_lock and/or mf_mutex. For example, as llist_del_all
> is lock-free with itself, folio_clear_hugetlb_hwpoison()s from
> __update_and_free_hugetlb_folio and memory_failure won't need explicit
> locking when freeing the raw_hwp_list. New code that manipulates
> raw_hwp_list must be careful to ensure the concurrency correctness.
> 
> Acked-by: Mike Kravetz <mike.kravetz@xxxxxxxxxx>
> Acked-by: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
> Signed-off-by: Jiaqi Yan <jiaqiyan@xxxxxxxxxx>

Anyway, this patch looks good to me.

Reviewed-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
Thanks.