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?
--
Peter Xu
