Re: [PATCH v7 06/13] hugetlb: Use the has_hwpoisoned flag

[Matthew Wilcox <willy@infradead.org>]

On Tue, Jul 28, 2026 at 09:36:37PM -0400, Gregory Price wrote:
> On Tue, Jul 28, 2026 at 09:43:58PM +0100, Matthew Wilcox (Oracle) wrote:
> > +
> > +#ifdef CONFIG_MEMORY_FAILURE
> > +static inline bool folio_test_huge_poison(const struct folio *folio)
> > +{
> > +	return (READ_ONCE(folio->page.page_type) >> 23) ==
> > +		((PGTY_hugetlb << 1) | 1);
> > +}
> > +
> > +static inline void folio_set_huge_poison(struct folio *folio)
> > +{
> > +	folio->page.page_type |= (1 << 23);
> > +}
> > +
> > +static inline void folio_clear_huge_poison(struct folio *folio)
> > +{
> > +	folio->page.page_type &= ~(1 << 23);
> > +}
> > +#else
> 
> Took me a second to realize why this was open-coded instead of using
> the generator macros.  The reasoning is buried in this comment:

I believe hugetlb is the first user of the low 24 bits of page_type.
As such, there aren't any generator macros yet ... and I'd probably want
there to be at least three before creating any.

> /*
>  * Check if a page is currently marked HWPoisoned.  This check is best
>  * effort only and inherently racy: there is no way to synchronize with
>  * failing hardware.  The caller may not have a refcount on the folio
>  * containing the page, so we must be careful to not trip any assertions.
>  */
>  static inline bool is_page_hwpoison(const struct page *page)
>  ...
> 
> which i presume is referring to VM_BUG_ON_FOLIO() in the set/clear in
> the macros.  Might be worth just note here if only so no one comes along
> and "fixes" this or writes more of these without the same reasoning.

It's even in the test ...

FOLIO_FLAG(has_hwpoisoned, FOLIO_SECOND_PAGE)
#define FOLIO_SECOND_PAGE       1
#define FOLIO_FLAG(name, page)                                          \
FOLIO_TEST_FLAG(name, page)                                             \
#define FOLIO_TEST_FLAG(name, page)                                     \
static __always_inline bool folio_test_##name(const struct folio *folio) \
{ return test_bit(PG_##name, const_folio_flags(folio, page)); }

static const unsigned long *const_folio_flags(const struct folio *folio,
                unsigned n)
{
        const struct page *page = &folio->page;

        VM_BUG_ON_PGFLAGS(page->compound_info & 1, page);
        VM_BUG_ON_PGFLAGS(n > 0 && !test_bit(PG_head, &page->flags.f), page);
        return &page[n].flags.f;
}

So there's two asserts that might trip; the first if the folio is freed
then reallocated into a larger folio and the former folio becomes a tail
page.  That's pretty unlikely for a hugetlb folio, but the second is
more likely.  That'll happen just by freeing the folio.

> > @@ -2725,13 +2748,19 @@ int unpoison_memory(unsigned long pfn)
> >  
> >  	ghp = get_hwpoison_page(p, MF_UNPOISON);
> >  	if (!ghp) {
> > +		spin_lock_irq(&hugetlb_lock);
> >  		if (folio_test_hugetlb(folio)) {
> >  			huge = true;
> >  			count = folio_free_raw_hwp(folio, false);
> > -			if (count == 0)
> > +			if (count == 0) {
> > +				spin_unlock_irq(&hugetlb_lock);
> >  				goto unlock_mutex;
> > +			}
> > +			ret = hugetlb_clear_poison(folio);
> > +		} else {
> > +			ret = TestClearPageHWPoison(p) ? 0 : -EBUSY;
> >  		}
> > -		ret = folio_test_clear_hwpoison(folio) ? 0 : -EBUSY;
> > +		spin_unlock_irq(&hugetlb_lock);
> 
> Adding new synchronization here without noting why - it's not
> immediately clear what this synchronizes.
> 
> Is this just fixing an unrelated bug / should it be a separate patch?

This prevents a hugetlb folio (which in this arm we don't have a
reference on) from being freed.  I think it ended up in this patch
because Sashiko pointed out the race during review of this patch.
It could probably go in its own patch