Re: [PATCH mm-unstable v1] mm: add a total mapcount for large folios

[David Hildenbrand <david@redhat.com>]

On 10.08.23 19:15, Peter Xu wrote:
> On Thu, Aug 10, 2023 at 11:48:27AM +0100, Ryan Roberts wrote:
>>> For PTE-mapped THP, it might be a bit bigger noise, although I doubt it is
>>> really significant (judging from my experience on managing PageAnonExclusive
>>> using set_bit/test_bit/clear_bit when (un)mapping anon pages).
>>>
>>> As folio_add_file_rmap_range() indicates, for PTE-mapped THPs we should be
>>> batching where possible (and Ryan is working on some more rmap batching).
>>
>> Yes, I've just posted [1] which batches the rmap removal. That would allow you
>> to convert the per-page atomic_dec() into a (usually) single per-large-folio
>> atomic_sub().
>>
>> [1] https://lore.kernel.org/linux-mm/20230810103332.3062143-1-ryan.roberts@arm.com/
> 
> Right, that'll definitely make more sense, thanks for the link; I'd be very
> happy to read more later (finally I got some free time recently..).  But
> then does it mean David's patch can be attached at the end instead of
> proposed separately and early?

Not in my opinion. Batching rmap makes sense even without this change, 
and this change makes sense even without batching.

> 
> I was asking mostly because I read it as a standalone patch first, and
> honestly I don't know the effect.  It's based on not only the added atomic
> ops itself, but also the field changes.
> 
> For example, this patch moves Hugh's _nr_pages_mapped into the 2nd tail
> page, I think it means for any rmap change of any small page of a huge one
> we'll need to start touching one more 64B cacheline on x86.  I really have
> no idea what does it mean for especially a large SMP: see 292648ac5cf1 on
> why I had an impression of that.  But I've no enough experience or clue to
> prove it a problem either, maybe would be interesting to measure the time
> needed for some pte-mapped loops?  E.g., something like faulting in a thp,

Okay, so your speculation right now is:

1) The change in cacheline might be problematic.

2) The additional atomic operation might be problematic.

> then measure the split (by e.g. mprotect() at offset 1M on a 4K?) time it
> takes before/after this patch.

I can certainly try getting some numbers on that. If you're aware of 
other micro-benchmarks that would likely notice slower pte-mapping of 
THPs, please let me know.

> 
> When looking at this, I actually found one thing that is slightly
> confusing, not directly relevant to your patch, but regarding the reuse of
> tail page 1 on offset 24 bytes.  Current it's Hugh's _nr_pages_mapped,
> and you're proposing to replace it with the total mapcount:
> 
>          atomic_t   _nr_pages_mapped;     /*    88     4 */
> 
> Now my question is.. isn't byte 24 of tail page 1 used for keeping a
> poisoned mapping?  See prep_compound_tail() where it has:
> 
> 	p->mapping = TAIL_MAPPING;
> 
> While here mapping is, afaict, also using offset 24 of the tail page 1:
> 
>          struct address_space * mapping;  /*    24     8 */
> 
> I hope I did a wrong math somewhere, though.
> 

I think your math is correct.

prep_compound_head() is called after prep_compound_tail(), so 
prep_compound_head() wins.

In __split_huge_page_tail() there is a VM_BUG_ON_PAGE() that explains 
the situation:

/* ->mapping in first and second tail page is replaced by other uses */
VM_BUG_ON_PAGE(tail > 2 && page_tail->mapping != TAIL_MAPPING,
	       page_tail);

Thanks for raising that, I had to look into that myself.

-- 
Cheers,

David / dhildenb