Re: [PATCH v4 10/13] mm/collapse: open-code collapse_single_pmd() in its two callers

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

Finally following up after recovering from last week :)

> 
> Right on both.
> 
> As I mentioned before, virtual address scanning might not be the best
> way to collapse page cache.
> 
> We might want to start from inodes on superblocks that have large folios
> enabled.
> 
> But it is out of scope for the patchset.

Ack.

> 
>> Also, I guess we can convert some of the scanning to use per-vma locks in the
>> future, whereby we would actually want to scan with the per-vma lock held.
> 
> That is the reason the caller owns the lock here.
> 
> On top of this series I have khugepaged scanning under the per-VMA lock:
> the scan asserts whatever lock the caller holds, the caller does
> vma_end_read() before the run, and the run takes the locks it needs
> itself.
> 
> Neither the scan nor the run changed for it.
> 
> With collapse_single_pmd() dropping the lock, it has to know which lock
> the caller took. process_madvise() on another process's mm cannot move
> to the VMA lock: untagged_addr_remote() asserts mmap_lock, so a remote
> MADV_COLLAPSE keeps it while a local one and khugepaged move on.
> 
> The single call would need a flag saying which lock to drop. That is
> the lock_dropped bool again, pointing the other way.
> 
>> I do wonder about one thing: should we really care so much about keeping the
>> mmap lock locked? Meaning, why not provide a single collapse_single_pmd() that
>>
>> * Is always called without the mmap lock (as is)
>> * Always returns with the mmap lock unlocked (change)
> 
> Yes, we should.
> 
> Walking the tables under one hold and giving the lock up only to
> collapse is how khugepaged has worked since ba76149f47d8 ("thp:
> khugepaged").
> 
> The bool is just how that signal got plumbed when MADV_COLLAPSE arrived
> in 50ad2f24b3b4, and it has already cost one bug, 5a62019807da
> ("mm/khugepaged: fix issue with tracking lock").
> 
> This series removes the bool and keeps the behaviour.
> 
>> Sure, we drop+re-acquire the mmap lock a couple of times and lookup the vma, but
>> isn't that actually being nice to the other parts of the system? In the future
>>  it would simply get called with the per-vma lock and would return with it unlocked.
>>
>> In the good old days, looking up VMAs was expensive, but nowadays ... not sure
>> if it still matters?
> 
> The rwsem and the VMA lookup are not the only cost.
> 
> In your version every table is a visit to the mm: unlock,
> khugepaged_mm_lock, back through khugepaged_do_scan(),
> khugepaged_mm_lock again, a trylock and a VMA walk, for a scan that on
> memory already huge is one pmd read.

I removed the inner loop. We could have kept it, but the numbers below suggest
something different I guess.

> 
> I measured your prototype against patch 9 as posted, both on a production
> config, 8G of anonymous memory already collapsed, scan_sleep_millisecs 0,
> pages_to_scan 65536, PMD order only, five runs each, medians:
> 
>                                      as posted    yours
>   khugepaged, nothing to collapse:
>     full passes over 8G in 20s         251,180    55,477
>     khugepaged CPU                        100%      100%
>     cost per refused table               19 ns     88 ns
>   MADV_COLLAPSE, already huge:
>     512M per call, p50                  3.8 us   17.4 us
>     2M per call, p50                    703 ns    703 ns
>   tables walked in full then refused
>     (max_ptes_none 0), passes in 20s       803       836
> 
> On being nice to the rest of the system: the scan is already bounded.
> The read hold ends after pages_to_scan worth of progress. We already
> have properly sized scan batching in place.
> 
>> khugepaged? Not sure if this matters. madvise? I suspect many real users operate
>> on a single PMD only (e.g., tcmalloc, jemalloc). For the other ones, not sure if
>> dropping the lock every PMD is really a problem?
> 
> khugepaged matters. We run it across the fleet, and its scan cost is
> what decides how aggressive we can afford to be with it. I am not
> giving up scan rate to keep one entry point into the engine.

I'm not sure whether the numbers above area really *that* relevant. But I get
your point: scanning must be as cheap as possible.

> 
> For a single PMD there is no difference either way. For a range, today a
> MADV_COLLAPSE over memory that is already huge walks it without letting
> go of the lock at all; in your version it relocks and revalidates for
> every table, and tells madvise_walk_vmas() to look the VMA up again.
> 
>> IOW, how bad would the following simplification be (prototype that needs more
>> work and thought):
>>
> 
> ...
> 
>>
>> Based on that, I'd rather want to see collapse_single_pmd() to just inline the file
>> and anon paths, and see how we can further optimize the locking internally (e.g., perform
>> the pagecache scanning without the mmap lock).
> 
> I see the appeal of the single entry point for the collapse engine. I do.
> 
> But it costs us on both the locking picture and the scan rate. That
> does not work for me.

So this is in mm-unstable and I want at least the first 9 patches to go to
mm-stable rather sooner than later.

As raised in person, I don't like the open-coding here; it just results in weird
API. I wonder whether we can come up with a better interface, while keeping the
locking as per your requirement cheap (scan performance).

While we figure that out, does it make sense to send out revised patch 1..9 to
move them to mm-stable while we figure out the remainder of the new API?


Regarding API, I think we have two possible paths forward:

a) An API based on ranges without a VMA. The internal handling would take care
of VMA lookup, re-validation, and managing the mmap lock.

b) Some better state tracking that abstracts the locking details.


For example, something along the lines of an init/next/finish sequence with
some additional state:

void collapse_init(struct collapse_state *state,
	struct mm_struct *mm, unsigned long start,
	unsigned long end, struct collapse_control *cc);

enum scan_result collapse_next(struct collapse_state *state);

void collapse_finish(struct collapse_walk *state);


Maybe that could give you some idea how we could provide a cleaner API? We have
various options I think that don't require this split in the caller.

Ideally, I'd wish we could get rid of the vma revalidation in the callers entirely.

If it helps, I can play with it to prototype what have in mind.

-- 
Cheers,

David