Re: [PATCH 4/8] mm/hugetlb: introduce per-node sysfs interface "zeroable_hugepages"

["Li Zhe" <lizhe.67@bytedance.com>]

> On Mon, 29 Dec 2025 10:57:23 -0800, fvdl@google.com wrote:
> 
> On Mon, Dec 29, 2025 at 4:26 AM Li Zhe <lizhe.67@bytedance.com> wrote:
> >
> > On Fri, 26 Dec 2025 10:51:01 -0800, fvdl@google.com wrote:
> >
> > > > +static ssize_t zeroable_hugepages_show(struct kobject *kobj,
> > > > +                                       struct kobj_attribute *attr, char *buf)
> > > > +{
> > > > +       struct hstate *h;
> > > > +       unsigned long free_huge_pages_zero;
> > > > +       int nid;
> > > > +
> > > > +       h = kobj_to_hstate(kobj, &nid);
> > > > +       if (WARN_ON(nid == NUMA_NO_NODE))
> > > > +               return -EPERM;
> > > > +
> > > > +       free_huge_pages_zero = h->free_huge_pages_node[nid] -
> > > > +                              h->free_huge_pages_zero_node[nid];
> > > > +
> > > > +       return sprintf(buf, "%lu\n", free_huge_pages_zero);
> > > > +}
> > > > +
> > > > +static inline bool zero_should_abort(struct hstate *h, int nid)
> > > > +{
> > > > +       return (h->free_huge_pages_zero_node[nid] ==
> > > > +               h->free_huge_pages_node[nid]) ||
> > > > +               list_empty(&h->hugepage_freelists[nid]);
> > > > +}
> > > > +
> > > > +static void zero_free_hugepages_nid(struct hstate *h,
> > > > +                                  int nid, unsigned int nr_zero)
> > > > +{
> > > > +       struct list_head *freelist = &h->hugepage_freelists[nid];
> > > > +       unsigned int nr_zerod = 0;
> > > > +       struct folio *folio;
> > > > +
> > > > +       if (zero_should_abort(h, nid))
> > > > +               return;
> > > > +
> > > > +       spin_lock_irq(&hugetlb_lock);
> > > > +
> > > > +       while (nr_zerod < nr_zero) {
> > > > +
> > > > +               if (zero_should_abort(h, nid) || fatal_signal_pending(current))
> > > > +                       break;
> > > > +
> > > > +               freelist = freelist->prev;
> > > > +               if (unlikely(list_is_head(freelist, &h->hugepage_freelists[nid])))
> > > > +                       break;
> > > > +               folio = list_entry(freelist, struct folio, lru);
> > > > +
> > > > +               if (folio_test_hugetlb_zeroed(folio) ||
> > > > +                   folio_test_hugetlb_zeroing(folio))
> > > > +                       continue;
> > > > +
> > > > +               folio_set_hugetlb_zeroing(folio);
> > > > +
> > > > +               /*
> > > > +                * Incrementing this here is a bit of a fib, since
> > > > +                * the page hasn't been cleared yet (it will be done
> > > > +                * immediately after dropping the lock below). But
> > > > +                * it keeps the count consistent with the overall
> > > > +                * free count in case the page gets taken off the
> > > > +                * freelist while we're working on it.
> > > > +                */
> > > > +               h->free_huge_pages_zero_node[nid]++;
> > > > +               spin_unlock_irq(&hugetlb_lock);
> > > > +
> > > > +               /*
> > > > +                * HWPoison pages may show up on the freelist.
> > > > +                * Don't try to zero it out, but do set the flag
> > > > +                * and counts, so that we don't consider it again.
> > > > +                */
> > > > +               if (!folio_test_hwpoison(folio))
> > > > +                       folio_zero_user(folio, 0);
> > > > +
> > > > +               cond_resched();
> > > > +
> > > > +               spin_lock_irq(&hugetlb_lock);
> > > > +               folio_set_hugetlb_zeroed(folio);
> > > > +               folio_clear_hugetlb_zeroing(folio);
> > > > +
> > > > +               /*
> > > > +                * If the page is still on the free list, move
> > > > +                * it to the head.
> > > > +                */
> > > > +               if (folio_test_hugetlb_freed(folio))
> > > > +                       list_move(&folio->lru, &h->hugepage_freelists[nid]);
> > > > +
> > > > +               /*
> > > > +                * If someone was waiting for the zero to
> > > > +                * finish, wake them up.
> > > > +                */
> > > > +               if (waitqueue_active(&h->dqzero_wait[nid]))
> > > > +                       wake_up(&h->dqzero_wait[nid]);
> > > > +               nr_zerod++;
> > > > +               freelist = &h->hugepage_freelists[nid];
> > > > +       }
> > > > +       spin_unlock_irq(&hugetlb_lock);
> > > > +}
> > >
> > > Nit: s/nr_zerod/nr_zeroed/
> >
> > Thank you for the reminder. I will address this issue in v2.
> >
> > > Feels like the list logic can be cleaned up a bit here. Since the
> > > zeroed folios are at the head of the list, and the dirty ones at the
> > > tail, and you start walking from the tail, you don't need to check if
> > > you circled back to the head - just stop if you encounter a prezeroed
> > > folio. If you encounter a prezeroed folio while walking from the tail,
> > > that means that all other folios from that one to the head will also
> > > be prezeroed already.
> >
> > Thank you for the thoughtful suggestion. Your line of reasoning is,
> > in most situations, perfectly valid. Under extreme concurrency,
> > however, a corner case can still appear. Imagine two processes
> > simultaneously zeroing huge pages: Process A enters
> > zero_free_hugepages_nid(), completes the zeroing of one huge page,
> > and marks the folio in the list as pre-zeroed. Should Process B enter
> > the same function moments later and decide to exit as soon as it
> > meets a prezeroed folio, the intended parallel zeroing would quietly
> > fall back to a single-threaded pace.
> 
> Hm, setting the prezeroed bit and moving the folio to the front of the
> free list happens while holding hugetlb_lock. In other words, if you
> encounter a folio with the prezeroed bit set while holding
> hugetlb_lock, it will always be in a contiguous stretch of prezeroed
> folios at the head of the free list.
> 
> Since the check for 'is this already prezeroed' is done while holding
> hugetlb_lock, you know for sure that the folio is part of a list of
> prezeroed folios at the head, and you can stop, right?

Sorry for the confusion earlier. You're right, this does make
zero_free_hugepages_nid() simpler. I'll update it in v2.

Thanks,
Zhe