Re: [PATCH v12 mm-new 06/15] khugepaged: introduce collapse_max_ptes_none helper function

[David Hildenbrand <david@redhat.com>]

>>> Hey Lorenzo,
>>>
>>>> I mean not to beat a dead horse re: v11 commentary, but I thought we were going
>>>> to implement David's idea re: the new 'eagerness' tunable, and again we're now just
>>>> implementing the capping at HPAGE_PMD_NR/2 - 1 thing again?
>>>
>>> I spoke to David and he said to continue forward with this series; the
>>> "eagerness" tunable will take some time, and may require further
>>> considerations/discussion.
>>
>> Right, after talking to Johannes it got clearer that what we envisioned with
> 
> I'm not sure that you meant to say go ahead with the series as-is with this
> silent capping?

No, "go ahead" as in "let's find some way forward that works for all and 
is not too crazy".

[...]

>> "eagerness" would not be like swappiness, and we will really have to be
>> careful here. I don't know yet when I will have time to look into that.
> 
> I guess I missed this part of the converastion, what do you mean?

Johannes raised issues with that on the list and afterwards we had an 
offline discussion about some of the details and why something 
unpredictable is not good.

> 
> The whole concept is that we have a paramaeter whose value is _abstracted_ and
> which we control what it means.
> 
> I'm not sure exactly why that would now be problematic? The fundamental concept
> seems sound no? Last I remember of the conversation this was the case.

The basic idea was to do something abstracted as swappiness. Turns out 
"swappiness" is really something predictable, not something we can 
randomly change how it behaves under the hood.

So we'd have to find something similar for "eagerness", and that's where 
it stops being easy.

> 
>>
>> If we want to avoid the implicit capping, I think there are the following
>> possible approaches
>>
>> (1) Tolerate creep for now, maybe warning if the user configures it.
> 
> I mean this seems a viable option if there is pressure to land this series
> before we have a viable uAPI for configuring this.
> 
> A part of me thinks we shouldn't rush series in for that reason though and
> should require that we have a proper control here.
> 
> But I guess this approach is the least-worst as it leaves us with the most
> options moving forwards.

Yes. There is also the alternative of respecting only 0 / 511 for mTHP 
collapse for now as discussed in the other thread.

> 
>> (2) Avoid creep by counting zero-filled pages towards none_or_zero.
> 
> Would this really make all that much difference?

It solves the creep problem I think, but it's a bit nasty IMHO.

> 
>> (3) Have separate toggles for each THP size. Doesn't quite solve the
>>      problem, only shifts it.
> 
> Yeah I did wonder about this as an alternative solution. But of course it then
> makes it vague what the parent values means in respect of the individual levels,
> unless we have an 'inherit' mode there too (possible).
> 
> It's going to be confusing though as max_ptes_none sits at the root khugepaged/
> level and I don't think any other parameter from khugepaged/ is exposed at
> individual page size levels.
> 
> And of course doing this means we
> 
>>
>> Anything else?
> 
> Err... I mean I'm not sure if you missed it but I suggested an approach in the
> sub-thread - exposing mthp_max_ptes_none as a _READ-ONLY_ field at:
> 
> /sys/kernel/mm/transparent_hugepage/khugepaged/max_mthp_ptes_none
> 
> Then we allow the capping, but simply document that we specify what the capped
> value will be here for mTHP.

I did not have time to read the details on that so far.

It would be one solution forward. I dislike it because I think the whole 
capping is an intermediate thing that can be (and likely must be, when 
considering mTHP underused shrinking I think) solved in the future 
differently. That's why I would prefer adding this only if there is no 
other, simpler, way forward.

> 
> That struck me as the simplest way of getting this series landed without
> necessarily violating any future eagerness which:
> 
> a. Must still support khugepaged/max_ptes_none - we aren't getting away from
>     this, it's uAPI.
> 
> b. Surely must want to do different things for mTHP in eagerness, so if we're
>     exposing some PTE value in max_ptes_none doing so in
>     khugepaged/mthp_max_ptes_none wouldn't be problematic (note again - it's
>     readonly so unlike max_ptes_none we don't have to worry about the other
>     direction).
> 
> HOWEVER, eagerness might want want to change this behaviour per-mTHP size, in
> which case perhaps mthp_max_ptes_none would be problematic in that it is some
> kind of average.
> 
> Then again we could always revert to putting this parameter as in (3) in that
> case, ugly but kinda viable.
> 
>>
>> IIUC, creep is less of a problem when we have the underused shrinker
>> enabled: whatever we over-allocated can (unless longterm-pinned etc) get
>> reclaimed again.
>>
>> So maybe having underused-shrinker support for mTHP as well would be a
>> solution to tackle (1) later?
> 
> How viable is this in the short term?

I once started looking into it, but it will require quite some work, 
because the lists will essentially include each and every (m)THP in the 
system ... so i think we will need some redesign.

> 
> Another possible solution:
> 
> If mthp_max_ptes_none is not workable, we could have a toggle at, e.g.:
> 
> /sys/kernel/mm/transparent_hugepage/khugepaged/mthp_cap_collapse_none
> 
> As a simple boolean. If switched on then we document that it caps mTHP as
> per Nico's suggestion.
> 
> That way we avoid the 'silent' issue I have with all this and it's an
> explicit setting.

Right, but it's another toggle I wish we wouldn't need. We could of 
course also make it some compile-time option, but not sure if that's 
really any better.

I'd hope we find an easy way forward that doesn't require new toggles, 
at least for now ...

-- 
Cheers

David / dhildenb