Re: [PATCH mm-unstable v18 11/14] mm/khugepaged: Introduce mTHP collapse support

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

>>
>> Reading this, it is unclear why exactly do we need the stack.
> 
> So I looked into your items below. It seems logical, and I think it
> works the same way; however, your method seems slightly harder to
> understand due to all the edge cases and more error-prone to future
> changes (the stack holds implicit knowledge of the offset/order that
> must now be tracked in the edge cases).
> 
> Given the stack is 24 bytes, I'm not sure if the extra complexity is
> worth saving that small amount of memory. Although we would also be
> getting rid of (3?) functions, so both approaches have pros and cons.

I consider a simple forward loop over the offset ... less complexity compared to
a stack structure :)

> 
> I will implement a patch comparing your solution against mine and send
> it here, then we can decide which approach is better.

Right, throw it over the fence and I'll see how to improve it further.

[...]

>>> +     bitmap_zero(cc->mthp_bitmap, MAX_PTRS_PER_PTE);
>>>       memset(cc->node_load, 0, sizeof(cc->node_load));
>>>       nodes_clear(cc->alloc_nmask);
>>> +
>>> +     enabled_orders = collapse_allowable_orders(vma, vma->vm_flags, tva_flags);
>>> +
>>> +     /*
>>> +      * If PMD is the only enabled order, enforce max_ptes_none, otherwise
>>> +      * scan all pages to populate the bitmap for mTHP collapse.
>>> +      */
>>
>> You should note here, that we re-verify in mthp_collapse().
>>
>> But the question is, whether we should relocate the check completely into
>> mthp_collapse(), instead of conditionally duplicating it.
>>
>> What speaks against always populating the bitmap and making the decision in
>> mthp_collapse()?
>>
>> Sure, we might scan a page table a bit longer, but the code gets clearer ... and
>> I am not sure if scanning some more page table entries is really that critical here.
> 
> Someone asked me to preserve the legacy behavior (PMD only). Although
> rather trivial, if you set max_ptes_none=0 for example, we'd still
> have to do 511 iterations for no reason if PMD collapse is the only
> enabled order rather than bailing immediately.
> 
> I'm ok with dropping it, but I think its the correct approach (despite
> the extra complexity). @Usama Arif brought up this point here
> https://lore.kernel.org/all/f8f7bb71-ca31-46ee-a62d-7ddfd83e0ead@gmail.com/

We talk about regressions, but I am not sure if we care about scanning speed
within a page table that much?

After all, we locked it and already read some entries.

Having the same check at two places to optimize for PMD order might right now
feel like a good optimization, but likely an irrelevant one in a near future?

Anyhow, won't push back, as long as we document why we are special casing things
here.

-- 
Cheers,

David