Re: [PATCH] mm/madvise: keep the hardware dirty bit in MADV_COLD's THP path

[Lorenzo Stoakes (ARM)]

+cc Zi

On Mon, Oct 12, 2026 at 11:33:48AM +0100, Lorenzo Stoakes (ARM) wrote:
> -cc slopper.
>
> On Mon, Oct 12, 2026 at 12:09:07PM +0200, David Hildenbrand (Arm) wrote:
> > On 10/11/26 15:22, samarth.colleges@xxxxxxxxx wrote:
>
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -426,8 +426,14 @@
> > >  		}
> > >
> > >  		if (!pageout && pmd_young(orig_pmd)) {
> > > -			pmdp_invalidate(vma, addr, pmd);
> > > -			orig_pmd = pmd_mkold(orig_pmd);
> > > +			/*
> > > +			 * Rebuild from the entry pmdp_invalidate() exchanged
> > > +			 * out, not from orig_pmd: hardware may have set the
> > > +			 * dirty bit since orig_pmd was read, and a MADV_FREE'd
> > > +			 * (lazyfree) THP whose dirty bit is lost is discarded
> > > +			 * by reclaim, silently dropping the store.
> > > +			 */
> > > +			orig_pmd = pmd_mkold(pmdp_invalidate(vma, addr, pmd));
> >
> > Okay, you are saying we can lose the dirty bit that could get set concurrently.
> > And if we did an MADV_FREE before, we might not detect that the content was changed.
>
> Yeah, lazyfree -> write means you don't actually free it.
>
> As per the manpage:
>
> 	For each of the pages that has been marked to be freed but has not yet
> 	been freed, the free operation will be canceled if the caller writes
> 	into the page.
>
> This is anon memory only:
>
> 	The MADV_FREE operation can be applied only to private anonymous
> 	pages (see mmap(2)).
>
> >
> >
> > I suspect this found through the help of an LLM and not an actual use case in
> > practice?
>
> Seems very likely.
>
> >
> > Using MADV_COLD following MADV_FREE sounds odd at thirst. In particular, when
> > having MADV_COLD race with actual access.
>
> It's odd to MADV_FREE (lazyfree) then MADV_COLD (deactivate) yes,
> especially with racing accesses.
>
> I wonder if there are other cases where you could get weird races like
> this...
>
> In any case it seems a really hard thing to hit in reality. But I think we
> probably need to fix this because of the stated contract of MADV_FREE.
>
> I think probably we shouldn't have added that contract...
>
> >
> > On the PTE path we use clear_young_dirty_ptes(CYDP_CLEAR_YOUNG) which achieves
> > the same through ptep_test_and_clear_young().
> >
> >
> > ... where we also just do a
> >
> > pte = ptep_get(ptep);
> > set_pte_at(vma->vm_mm, address, ptep, pte_mkold(pte));
> >
> > in the generic variant and rely on the per-arch variant to do the right thing.
> >
> > Which makes me wonder whether we should actually have per-architecture PMD
> > helpers for that purpose?
>
> Is it achieving the same thing? There's an invalidation here also.
>
> pmdp_establish() is used to install a pmd_mkinvalid() entry and there's a
> flush_pmd_tlb_range().
>
> --
> Cheers, Lorenzo

OK, isn't this all moot with Zi's patch? [0]

[0]: https://lore.kernel.org/linux-mm/20261009-lazyfree-stable-fix-v1-1-70997958ef8d@xxxxxxxxxx

--
Cheers, Lorenzo