On Tue, Apr 26, 2022 at 07:44:04AM -0700, Zach O'Keefe wrote:
> +/* Gather information from one khugepaged_scan_[pmd|file]() request */
> +struct collapse_result {
> +	enum scan_result result;
> +
> +	/* Was mmap_lock dropped during request? */
> +	bool dropped_mmap_lock;
> +};

IMHO this new dropped_mmap_lock makes things a bit more complicated..

To me, the old code actually is easy to read on when the lock is dropped:

  - For file, we always drop it
  - For anon, when khugepaged_scan_pmd() returns 1

That's fairly clear to me.

But I understand what you need, probably the "result" hidden in the old
world but you'd need that for MADV_COLLAPSE?

I also noticed major chunks of this patch is converting "result = XXX" into
"cr->result = XXX".  IMHO that's really not necessary for touching up all
the places around.

I'm also wondering whether we could simply pass over the result into your
newly introduced collapse_control*.

In summary, this is what's in my mind..

  - Add collapse_control.result
  - Make sure both khugepaged_scan_file() and khugepaged_scan_pmd() to
    simply remember to update cc->result before it returns

I've attached a pesudo code patch (note!  it's pesudo not even compile but
to show what I meant..), so far I don't see a problem with it.

-- 
Peter Xu