Re: [PATCH v2 1/1] mm/madvise: enhance lazyfreeing with mTHP in madvise_free

[David Hildenbrand]

On 07.03.24 09:10, Barry Song wrote:
> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang <ioworker0@xxxxxxxxx> wrote:
> > Hey Barry,
> >
> > Thanks for taking time to review!
> >
> > On Thu, Mar 7, 2024 at 3:00 PM Barry Song <21cnbao@xxxxxxxxx> wrote:
> > > On Thu, Mar 7, 2024 at 7:15 PM Lance Yang <ioworker0@xxxxxxxxx> wrote:
> > > >
> > [...]
> > > > +static inline bool can_mark_large_folio_lazyfree(unsigned long addr,
> > > > +                                                struct folio *folio, pte_t *start_pte)
> > > > +{
> > > > +       int nr_pages = folio_nr_pages(folio);
> > > > +       fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY;
> > > > +
> > > > +       for (int i = 0; i < nr_pages; i++)
> > > > +               if (page_mapcount(folio_page(folio, i)) != 1)

No new direct subpage mapcount users please if avoidable. Also, without 
holding the page lock this might be racy (did not stare at the bigger code).

> > > > +                       return false;
> > >
> > > we have moved to folio_estimated_sharers though it is not precise, so
> > > we don't do
> > > this check with lots of loops and depending on the subpage's mapcount.
> >
> > If we don't check the subpage’s mapcount, and there is a cow folio associated
> > with this folio and the cow folio has smaller size than this folio,
> > should we still
> > mark this folio as lazyfree?
>
> I agree, this is true. However, we've somehow accepted the fact that
> folio_likely_mapped_shared
> can result in false negatives or false positives to balance the
> overhead.  So I really don't know :-)
>
> Maybe David and Vishal can give some comments here.

Well, I did not accept the fact yet that folio_likely_mapped_shared() 
can give false negatives :)

So I've been working on a replacement [1], also in the context of 
removing the subpage mapcounts. But there is still a lot to resolve 
around that, first step being to introduce the total mapcount.

But independent of that, MADV_FREE is special (compared to 
MADV_DONTNEED), because we really have to make sure all page table 
mappings that map the folio will be covered here.

What could work for large folios is making sure that #ptes that map the 
folio here correspond to the folio_mapcount(). And folio_mapcount() 
should be called under folio lock, to avoid racing with swapout/migration.

So instead of checking each page_mapcount(), see how many PTEs you could 
batch, try taking the folio lock, and compare the number of batched PTEs 
against folio_mapcount(). If they match, we're good. if not, there is 
some other folio mapping somewhere else (other process, mremap).

folio_likely_mapped_shared() should still be used as an early exit point.

[1] 
https://lore.kernel.org/all/20231124132626.235350-1-david@xxxxxxxxxx/T/#u

--
Cheers,

David / dhildenb