Re: [PATCH v3 1/9] mm: limit pagecache folio refcount when mapping pages

[Pedro Falcato <pfalcato@suse.de>]

On Thu, Nov 12, 2026 at 02:16:33PM +0100, David Hildenbrand (Arm) wrote:
> > Mostly LGTM, with some comments:
> > 
> > Reviewed-by: Pedro Falcato <pfalcato@suse.de>
> 
> Thanks for the review, Pedro!
> 
> > 
> >> ---
> >>  include/linux/mm.h | 35 +++++++++++++++++++++++++++++++----
> >>  mm/filemap.c       |  6 ++++++
> >>  mm/huge_memory.c   | 29 +++++++++++++++++++++++++++--
> >>  mm/memory.c        | 33 +++++++++++++++++++++++++++++----
> >>  mm/migrate.c       | 11 +++++++++++
> >>  mm/userfaultfd.c   |  4 ++++
> >>  mm/util.c          | 18 ++++++++++++++++++
> >>  7 files changed, 126 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> >> index 4608540e40d46..439cfe446cea4 100644
> >> --- a/mm/huge_memory.c
> >> +++ b/mm/huge_memory.c
> >> @@ -1626,6 +1626,7 @@ static vm_fault_t insert_pmd(struct vm_area_struct *vma, unsigned long addr,
> >>  {
> >>  	struct mm_struct *mm = vma->vm_mm;
> >>  	pgtable_t pgtable = NULL;
> >> +	vm_fault_t ret = VM_FAULT_NOPAGE;
> >>  	spinlock_t *ptl;
> >>  	pmd_t entry;
> >>  
> >> @@ -1662,6 +1663,10 @@ static vm_fault_t insert_pmd(struct vm_area_struct *vma, unsigned long addr,
> >>  		if (is_huge_zero_folio(fop.folio)) {
> >>  			entry = pmd_mkspecial(entry);
> >>  		} else {
> >> +			if (unlikely(folio_ref_count_over_limit(fop.folio))) {
> >> +				ret = VM_FAULT_SIGBUS;
> >> +				goto out_unlock;
> >> +			}
> >>  			folio_get(fop.folio);
> > 
> > I see you use this pattern often. Would it make sense to have a
> > bool folio_get_unless_overflow()?
> 
> I commented somewhere (in v2 I think) that I played with using
> folio_get_checked() from #2, but didn't quite like it so I discarded it as
> 
> (a) it cannot be used everywhere here, as I'd need more functions to add
> multiple references. next_uptodate_folio() also could use it as I would need yet
> another temporary helper for a try_get().
> 
> (b) these changes here are going to get reverted fairly soon once I change the
> way how mappings are refcounted (which is, unfortunately, not suitable for
> stable, otherwise I wouldn't be throwing in this temporary change in the first
> place)
> 
> So everything that is expected to stay in later patches uses the proper helper.

Alright.

> 
> [...]
> 
> >>  
> >> +		if (unlikely(folio_ref_count_over_limit(folio))) {
> >> +			/* Skip this PUD; it can be faulted in later. */
> >> +			ret = (dst_vma->vm_flags & VM_MIXEDMAP) ? -ENOMEM : 0;
> > 
> > A comment as to what's going on here with VM_MIXEDMAP would be useful. I guess
> > the idea is that faulting on a mixedmap later will not necessarily hand you the
> > exact same page fault result?
> 
> I can throw in a comment, yes.
> 
> [...]
> 
> >> +bool __folio_ref_count_over_limit(unsigned int ref_count)
> >> +{
> >> +	if (ref_count >= FOLIO_REF_COUNT_ADMIN_LIMIT)
> >> +		return true;
> >> +	/* Background threads must not consume admin reserves. */
> >> +	if (current->flags & PF_KTHREAD)
> >> +		return true;
> >> +	/* Neither must interrupt contexts on behalf of the interrupted task. */
> >> +	if (!in_task())
> >> +		return true;
> >> +	/* Capability checks are not safe with IRQs disabled. */
> >> +	if (irqs_disabled())
> >> +		return true;
> >> +	/* Use the task's credentials, which filesystem overrides cannot change. */
> >> +	return !has_capability_noaudit(current, CAP_SYS_RESOURCE);
> >> +}
> > 
> > In case this function ever returns over limit, we probably want to log this
> > in the kernel log. If this function gets the folio you could even print
> > the problematic file offset/inode/filesystem/etc. Ratelimited of course.
> 
> You mean, if this function returns "true", right? Is printing always fine?
> (e.g., irqs disabled?)

Yes. As far as I know, it's always fine, even under NMI. But definitely fine
with IRQs disabled.

> 
> > 
> >> +EXPORT_SYMBOL_GPL(__folio_ref_count_over_limit);
> > 
> > I'm fairly sure this will break the build for a number of non-GPL drivers
> > when calling folio_get() with CONFIG_DEBUG_VM=y.
> 
> You mean, breaking OOT drivers, right? I can switch to EXPORT_SYMBOL if that's a
> concern.

I had the impression we had some stuff in-tree that isn't marked as GPL. But a quick
grep shows that everything is either GPL or dual-licensed.

In any case, yes, this will quite possibly break debug builds for people OOT.
I think folio_get() might be a bit too ubiquitous to do this to.

-- 
Pedro