Re: [PATCH 38/46] mm/ksm: reset the unstable trees of all nodes

[David Hildenbrand (Arm)]

On 10/30/26 04:24, xu.xin@xxxxxxxxx wrote:
>>
>> Let's fix it by tracking in ksm_nr_node_ids the actual number of
>> allocated tree roots.
>>
>> remove_all_stable_nodes() and ksm_check_stable_tree() will now scan
>> more tree roots (skipping the empty ones), which is harmless.
>>
>> Fixes: ef53d16cded7 ("ksm: allocate roots when needed")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Reported-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>> ---
>>  mm/ksm.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/ksm.c b/mm/ksm.c
>> index dbfb8f98b1892..9c51925825cbf 100644
>> --- a/mm/ksm.c
>> +++ b/mm/ksm.c
>> @@ -475,6 +475,7 @@ static void advisor_stop_scan(void)
>>  #ifdef CONFIG_NUMA
>>  /* Zeroed when merging across nodes is not allowed */
>>  static unsigned int ksm_merge_across_nodes = 1;
>> +/* Number of allocated stable and unstable tree roots. */
>>  static int ksm_nr_node_ids = 1;
>>  #else
>>  #define ksm_merge_across_nodes	1U
>> @@ -3636,14 +3637,13 @@ static ssize_t merge_across_nodes_store(struct kobject *kobj,
>>  			else {
>>  				root_stable_tree = buf;
>>  				root_unstable_tree = buf + nr_node_ids;
>> +				ksm_nr_node_ids = nr_node_ids;
>>  				/* Stable tree is empty but not the unstable */
>>  				root_unstable_tree[0] = one_unstable_tree[0];
>>  			}
>>  		}
>> -		if (!err) {
>> +		if (!err)
>>  			ksm_merge_across_nodes = knob;
>> -			ksm_nr_node_ids = knob ? 1 : nr_node_ids;
>> -		}
>>  	}
>>  	mutex_unlock(&ksm_thread_mutex);
>>  
>>
>> -- 
>> 2.43.0
> 

Thanks for the review!

> Looks correct as far as it goes, but I think it is not quite sufficient
> on its own.

I think this fixes the problem described. What you describe below is a mismatch
between documentation and the implementation, but I don't think it is
problematic as in "responsible for BUGS". See below.

> 
> The documentation requires that merge_across_nodes can only be changed
> when there are no KSM shared pages in the system, but
> merge_across_nodes_store() does not enforce this at the sysfs level.
> 
> Should we also reject writes while ksmd is merging? At minimum:
> 
> +	if (ksm_run & KSM_RUN_MERGE)
> +		return -EBUSY;
> 
> Ideally, we should also check that ksm_pages_shared is zero, since
> KSM_RUN_STOP (0) does not guarantee that all pages have been unmerged.

We do that already in merge_across_nodes_store() I think:

if (ksm_pages_shared || remove_all_stable_nodes())
	err = -EBUSY;

Which is really required when swapping trees.

> 
> That would make the interface match the documented sequence and close
> the remaining race window.

Right, either we should adjust documentation or adjust the implementation. Not
sure if restricting the interface is really required. Personally, I'd simply
fixup the documentation (separately) unless there is a real problem with the
implementation when allowing KSM_RUN_MERGE.

(I think there is no problem as we are synchronized on the mutex).

-- 
Cheers,

David