Re: [PATCH v2 2/2] mm/ksm: fix advisor_min_pages_to_scan description

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Mon, Aug 17, 2026 at 10:54:52AM +0800, Jinjiang Tu wrote:
>
> 在 2026/8/13 22:32, Lorenzo Stoakes (ARM) 写道:
> > Please show me the common courtesy of responding to _all_ the review given,
> > not only the one point you want to respond to.
> >
> > I'm not happy with this patch, sorry, for the reasons stated that you chose
> > to ignore.
> >
> > On Thu, Aug 13, 2026 at 07:17:05PM +0800, Jinjiang Tu wrote:
> > > 在 2026/8/13 18:56, Lorenzo Stoakes (ARM) 写道:
> > > > On Thu, Aug 13, 2026 at 11:17:22AM +0800, Jinjiang Tu wrote:
> > > > > Both Documentation/admin-guide/mm/ksm.rst and the comment next to the
> > > > > variable definition in mm/ksm.c describe advisor_min_pages_to_scan as a
> > > > > lower limit of the pages_to_scan parameter, but that is not how the
> > > > > scan-time advisor actually uses it. commit 4e5fa4f5eff6 ("mm/ksm: add ksm
> > > > > advisor") only uses it to initialize ksm_thread_pages_to_scan when the
> > > > > scan-time advisor is enabled. This will mislead the users.
> > > > >
> > > > > The semantics of advisor_min_pages_to_scan was updated in the v2 patchset
> > > > > [1], but the documentation wasn't updated.
> > > > >
> > > > > Update the documentation and comment to match the semantics of
> > > > > advisor_min_pages_to_scan.
> > > > Umm, firstly if this were wrong you'd need to change the name of the variable
> > > > too instad of documenting it as something completely distinct.
> > > >
> > > > But secondly AFAICT the logic in scan_time_advisor() suggests that it truly does
> > > > act as a minimum bound as well as being the initial value for
> > > > ksm_thread_pages_to_scan.
> > > As the cover letter[1] said, 'The initial value and the max value for the pages_to_scan parameter can
> > > be limited with:', ksm_advisor_min_pages is used as the initial value, not the min value of pages_to_scan.
> > (Wrap your lines properly please, it's a common courtesy to learn how to
> > send mail according to community conventions.)
>
> Thanks for the reminder, I will follow this in the future.
>
> > One of the part of my review you ignored:
> >
> > 	But in any case, you are making a claim here yet have provided no
> > 	evidence for it. 'It is just the initial assignment' means nothing
> > 	- if the logic can then only increase it up to the maximum value
> > 	then it still acts as a minimum.
>
> ksm_thread_pages_to_scan is adjusted by scan_time_advisor() after a full
> scan finishes. ksm_thread_pages_to_scan could increase or decrease depend
> on the real scan time is longer or shorter than the target scan time.
> The min value of ksm_thread_pages_to_scan is only limited by
> KSM_ADVISOR_MIN_CPU, so ksm_thread_pages_to_scan could be smaller
> than ksm_advisor_min_pages_to_scan.

OK thanks :)

With this explanation put in the commit msg somewhere on respin then we should
be good.

Probably we want to rename the variable too but can save that for another change
since the attr is called min still (ugh).


--
Cheers, Lorenzo