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

["David Hildenbrand (Arm)" <david@kernel.org>]

On 11/12/26 15:06, Pedro Falcato wrote:
> 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!
>>
>>>
>>>
>>> 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.
> 
>>
>> [...]
>>
>>>
>>> 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.
>>
>> [...]
>>
>>>
>>> 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.
> 
>>
>>>
>>>
>>> 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.

Thinking again, folio_get() should not be affected, it rightfully only checks
for a refcount underflow (when we exceed the limit + headroom). So only
try_get_page() is affected, which OOT very likely doesn't use.

-- 
Cheers,

David