Re: [RFC PATCH 18/18] mm: Add pfn_range_put()

From: Matthew Wilcox <willy@infradead.org>
To: Ryan Roberts <ryan.roberts@arm.com>
Cc: linux-mm@kvack.org
Subject: Re: [RFC PATCH 18/18] mm: Add pfn_range_put()
Date: Fri, 1 Sep 2023 05:27:01 +0100	[thread overview]
Message-ID: <ZPFoFbin9pRq+Z/V@casper.infradead.org> (raw)
In-Reply-To: <cc148408-5935-4d1f-bbbd-f25af7ec90ae@arm.com>

On Thu, Aug 31, 2023 at 08:03:14PM +0100, Ryan Roberts wrote:
> > +++ b/include/linux/mm.h
> > @@ -1515,6 +1515,13 @@ typedef union {
> >  void release_pages(release_pages_arg, int nr);
> >  void folios_put(struct folio_batch *folios);
> >  
> > +struct pfn_range {
> > +	unsigned long start;
> > +	unsigned long end;
> > +};
> 
> As mentioned in the other thread, I think we need to better convey that a
> pfn_range must be fully contained within a single folio. Perhaps its better to
> call it `struct folio_region`?

Yep, I'm not taking a strong position on that.  We can call it whatever
you like, subject to usual nitpicking later.

> > +
> > +void pfn_range_put(struct pfn_range *range, unsigned int nr);
> 
> If going with `struct folio_region`, we could call this folios_put_refs()?
> 
> Or if you hate that idea and want to stick with pfn, then at least call it
> pfn_ranges_put() (with s on ranges).

folio_regions_put()?  I don't know at this point; nothing's speaking
to me.

> > +void pfn_range_put(struct pfn_range *range, unsigned int nr)
> > +{
> > +	struct folio_batch folios;
> > +	unsigned int i;
> > +	struct lruvec *lruvec = NULL;
> > +	unsigned long flags = 0;
> > +
> > +	folio_batch_init(&folios);
> > +	for (i = 0; i < nr; i++) {
> > +		struct folio *folio = pfn_folio(range[i].start);
> > +		unsigned int refs = range[i].end - range[i].start;
> 
> Don't you need to check for huge zero page like in folios_put()?
> 
> 		if (is_huge_zero_page(&folio->page))
> 			continue;

Maybe?  Can we put the huge zero page in here, or would we filter it
out earlier?

> > +	if (lruvec)
> > +		unlock_page_lruvec_irqrestore(lruvec, flags);
> > +
> > +	if (folios.nr) {
> > +		mem_cgroup_uncharge_folios(&folios);
> > +		free_unref_folios(&folios);
> > +	}
> > +}
> 
> This still duplicates a lot of the logic in folios_put(), but I see you have an
> idea in the other thread for improving this situation - I'll reply in the
> context of that thread.
> 
> But overall this looks great - thanks for taking the time to put this together -
> it will integrate nicely with my mmugather series.
> 

Thanks!  One thing I was wondering; intended to look at today, but didn't
have time for it -- can we use mmugather to solve how vmscan wants to
handle batched TLB IPIs for mapped pages, instead of having its own thing?
72b252aed506 is the commit to look at.