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