On 2024/4/9 2:31, Jane Chu wrote:
> On 4/8/2024 8:36 AM, Matthew Wilcox wrote:
>
>> On Wed, Mar 06, 2024 at 05:31:19PM +0800, Miaohe Lin wrote:
>>>> -void shake_page(struct page *p)
>>>> +void shake_folio(struct folio *folio)
>>> It might not be a good idea to convert shake_page() into shake_folio(). shake_page() can
>>> be called without holding page refcnt. So the below race could happen:
>>>
>>> hwpoison_inject
>>> folio = page_folio(p); --> assume p is 4k page [1]
>>> shake_folio(folio)
>>> folio_test_slab(folio)
>>> test_bit(PG_slab, folio_flags(folio, FOLIO_PF_NO_TAIL) [2]
>>> VM_BUG_ON_PGFLAGS(PageTail(page), page) in folio_flags
>>>
>>> Between [1] and [2], page can become PageTail of a THP. So the VM_BUG_ON_PGFLAGS will trigger.
>>> Or am I miss something?
>> No, you're not missing anything. This race can happen. However, I've
>> removed the VM_BUG_ON for folio_test_slab() with "mm: free up PG_slab".
>> Now it goes through:
>>
>> static inline bool PageSlab(const struct page *page)
>> {
>> return folio_test_slab(page_folio(page));
>> }
>>
>> static __always_inline bool folio_test_##fname(const struct folio *folio)\
>> { \
>> return folio_test_type(folio, PG_##lname); \
>> } \
>>
>> #define folio_test_type(folio, flag) \
>> ((folio->page.page_type & (PAGE_TYPE_BASE | flag)) == PAGE_TYPE_BASE)
>>
>> which has no assertion that the folio is not a tail page. Maybe it
>> should, but until then we'll silently get the wrong result ;-)
>>
> In this case (4k page -> THP), the act of drain will be triggered with perhaps nothing to do, so looks like a harmless 'wrong result' to me.
>
> thanks,
I tend to agree that with "free up PG_slab", this race should be harmless.
Thanks both.
.
>
> -jane
>
> .
