Re: [PATCH] mm/ksm: mark migration stores with WRITE_ONCE()

[xu.xin16]

> Use WRITE_ONCE() for both stores to pair them with the existing lockless
> reads.  This preserves the existing smp_wmb()/smp_rmb() migration protocol
> and control flow while preventing compiler transformations of the shared
> accesses.
> 
> Signed-off-by: Chengfeng Ye <nicoyip.dev@xxxxxxxxx>
> ---
>  mm/ksm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/ksm.c b/mm/ksm.c
> index b4142746777e..bec6fea0fdb4 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -1116,7 +1116,8 @@ static inline void folio_set_stable_node(struct folio *folio,
>  					 struct ksm_stable_node *stable_node)
>  {
>  	VM_WARN_ON_FOLIO(folio_test_anon(folio) && PageAnonExclusive(&folio->page), folio);
> -	folio->mapping = (void *)((unsigned long)stable_node | FOLIO_MAPPING_KSM);
> +	WRITE_ONCE(folio->mapping,
> +		   (void *)((unsigned long)stable_node | FOLIO_MAPPING_KSM));
>  }

This is good.

>  
>  #ifdef CONFIG_SYSFS
> @@ -3318,7 +3319,7 @@ void folio_migrate_ksm(struct folio *newfolio, struct folio *folio)
>  	stable_node = folio_stable_node(folio);
>  	if (stable_node) {
>  		VM_BUG_ON_FOLIO(stable_node->kpfn != folio_pfn(folio), folio);
> -		stable_node->kpfn = folio_pfn(newfolio);
> +		WRITE_ONCE(stable_node->kpfn, folio_pfn(newfolio));
>  		/*
>  		 * newfolio->mapping was set in advance; now we need smp_wmb()
>  		 * to make sure that the new stable_node->kpfn is visible
> -- 
> 2.43.0
> 

There are other places where READ_ONCE is not used like in ksm_check_stable_tree() and
stable_node_dup_remove_range(), but these two function belong to MEM_OFFLINE, and
I think there should be no races between migrate and ksm_memory_callback of MEM_OFFLINE.

So Basically it looks good to me.

Acked-by: Xu Xin <xu.xin16@xxxxxxxxxx>