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

[Peter Xu <peterx@redhat.com>]

On Thu, Apr 28, 2022 at 02:59:44PM -0700, Zach O'Keefe wrote:
> 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.

Yes that's a fair point.  It's not that obvious to me too when I first read
it, but I am just worried hiding that too deep could make it complicated in
another form.

Maybe we can rename "ret" in khugepaged_scan_mm_slot() to "lock_dropped",
or we can comment above khugepaged_scan_pmd() explaining the retval.
Or.. both?

-- 
Peter Xu