Re: [PATCH v3 04/12] mm/khugepaged: add struct collapse_result

["Zach O'Keefe" <zokeefe@google.com>]

On Wed, Apr 27, 2022 at 1:47 PM Peter Xu <peterx@redhat.com> wrote:
>
> 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.
>

Agreed - but I know when I first read the code it wasn't obvious to me
what the return value of khugepaged_scan_pmd() was saying. I thought
giving it a name ("dropped_mmap_lock") would help future readers.

That said, I'm fine reverting this and going back to the old way of
things. It's less members crowding struct collapse_control as well.

> 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.
>

Thanks for going so far as to throw this patch together! I've gone
ahead and used it with minimal changes. I agree that it's cleaner now.

> --
> Peter Xu