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

在 2026/8/18 0:55, Lorenzo Stoakes (ARM) 写道:
> 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.

I will update the commit message in v3.

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

Yes, I have considered to rename the variable, but the sysfs knob name is
advisor_min_pages_to_scan and couldn't be renamed to keep compatibility IIUC.
so I keep the variable name unchanged too, to keep the variable name and sysfs
knob name consistent.

>
>
> --
> Cheers, Lorenzo