On Thu, Apr 28, 2022 at 4:21 PM Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> 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@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.
>
> 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?
>
So after continuing to play around with your suggestion for a bit
(thank you again), I found that with minimal effort, we could just
pipe the scan_result back through function return values.
The only point where this becomes awkward is for khugepaged_scan_pmd()
where the return value already has a specific meaning.
Solving both problems at once, we can both (a) remove another member
from struct collapse_control (result) by passing this value along as
the return value of functions, and (b) move this "we dropped the lock"
return value into a "bool *mmap_locked" arg to khugepaged_scan_pmd().
The resulting code rebased on this change is much cleaner overall.
> --
> Peter Xu
>
