Re: [PATCH 3/4] mm/ksm: make break_ksm() more scalable

[Lorenzo Stoakes (ARM)]

On Fri, Sep 11, 2026 at 04:12:10PM +0800, xu.xin16@xxxxxxxxxx wrote:
> From: Xu Xin (ZTE) <xu.xin@xxxxxxxxx>
>
> Currently the last argument 'walk_lock' of break_ksm() is used to
> indicate whether the page_walk is protected by mmap_read_lock or
> mmap_write_lock. If 'walk_lock' is true, we suppose its context to
> be under mmap_write_lock() protection, then mark it PGWALK_WRLOCK and
> make its vma be write-locked during the walk; If 'walk_lock' is
> false, we suppose its context to be mmap_read_lock(), then mark it
> PGWALK_RDLOCK.

I thnk this whole block is unnecessary. You're basically writing what the code
does in English

>
> This change is prepared for the latter patch to enable VMA

Latter -> later.

And it's the patch I'm not cc'd on so I don't see unless I go do a bunch of
stuff to try to download it... great :)

> read-locking where break_ksm() might be under the third new proctecion
> way: VMA read-locking, so we have to replace the boolean variable to
> the enum 'page_walk_lock', but without any function changed.

You don't, this is just horrible.

>
> No functional change intended.
>
> Signed-off-by: Xu Xin (ZTE) <xu.xin@xxxxxxxxx>
> ---
>  mm/ksm.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 8df66b4e5de0..dda105681d7f 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -660,16 +660,11 @@ static int break_ksm_pmd_entry(pmd_t *pmdp, unsigned long addr, unsigned long en
>  	return found;
>  }
>
> -static const struct mm_walk_ops break_ksm_ops = {
> +static struct mm_walk_ops break_ksm_ops = {
>  	.pmd_entry = break_ksm_pmd_entry,
>  	.walk_lock = PGWALK_RDLOCK,
>  };
>
> -static const struct mm_walk_ops break_ksm_lock_vma_ops = {
> -	.pmd_entry = break_ksm_pmd_entry,
> -	.walk_lock = PGWALK_WRLOCK,
> -};
> -
>  /*
>   * Though it's very tempting to unmerge rmap_items from stable tree rather
>   * than check every pte of a given vma, the locking doesn't quite work for
> @@ -696,11 +691,11 @@ static const struct mm_walk_ops break_ksm_lock_vma_ops = {
>   * protection keys here anyway.
>   */
>  static int break_ksm(struct vm_area_struct *vma, unsigned long addr,
> -		unsigned long end, bool lock_vma)
> +		unsigned long end, enum page_walk_lock walk_lock)

Ugh yuck this is horrible, you're exposing internal page walker state here as a
parameter...?

And then this commit makes it possible for any walk_lock to be passed but then
you change none of the code to handle it?

>  {
>  	vm_fault_t ret = 0;
> -	const struct mm_walk_ops *ops = lock_vma ?
> -				&break_ksm_lock_vma_ops : &break_ksm_ops;
> +	struct mm_walk_ops *ops = &break_ksm_ops;
> +	ops->walk_lock = walk_lock;

Are you sure this can't be run concurrently by two walkers?

I didn't see any arguments about that in the commit message. Having a single,
static, struct where you change the walk_lock is gross.

What would be better is to have your own enum that lists ksm lock state or
express it some other way, then if possible have it on the stack otherwise
ensure that state can't be corrupted.

Again, if you'd sent me 4/4 too I could see the overall structure and give
advice but...

>
>  	do {
>  		int ksm_page;
> @@ -807,7 +802,7 @@ static void break_cow(struct ksm_rmap_item *rmap_item)
>  	mmap_read_lock(mm);
>  	vma = find_mergeable_vma(mm, addr);
>  	if (vma)
> -		break_ksm(vma, addr, addr + PAGE_SIZE, false);
> +		break_ksm(vma, addr, addr + PAGE_SIZE, PGWALK_RDLOCK);
>  	mmap_read_unlock(mm);
>  }
>
> @@ -1245,7 +1240,7 @@ static int unmerge_and_remove_all_rmap_items(void)
>  		for_each_vma(vmi, vma) {
>  			if (!(vma->vm_flags & VM_MERGEABLE) || !vma->anon_vma)
>  				continue;
> -			err = break_ksm(vma, vma->vm_start, vma->vm_end, false);
> +			err = break_ksm(vma, vma->vm_start, vma->vm_end, PGWALK_RDLOCK);
>  			if (err)
>  				goto error;
>  		}
> @@ -2885,7 +2880,7 @@ static int __ksm_del_vma(struct vm_area_struct *vma)
>  		return 0;
>
>  	if (vma->anon_vma) {
> -		err = break_ksm(vma, vma->vm_start, vma->vm_end, true);
> +		err = break_ksm(vma, vma->vm_start, vma->vm_end, PGWALK_WRLOCK);
>  		if (err)
>  			return err;
>  	}
> @@ -3037,7 +3032,7 @@ int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
>  			return 0;		/* just ignore the advice */
>
>  		if (vma->anon_vma) {
> -			err = break_ksm(vma, start, end, true);
> +			err = break_ksm(vma, start, end, PGWALK_WRLOCK);
>  			if (err)
>  				return err;
>  		}
> --
> 2.25.1

--
Cheers, Lorenzo