Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i84614614:Fastmail
References: <20230418051342.1919757-1-shr@devkernel.io>
 <20230418152849.505124-1-david@redhat.com>
 <20230418152849.505124-2-david@redhat.com>
User-agent: mu4e 1.10.1; emacs 28.2.50
From:   Stefan Roesch <shr@devkernel.io>
To:     David Hildenbrand <david@redhat.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        linux-s390@vger.kernel.org, linux-kselftest@vger.kernel.org,
        Andrew Morton <akpm@linux-foundation.org>,
        Rik van Riel <riel@surriel.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Michal Hocko <mhocko@suse.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Sven Schnelle <svens@linux.ibm.com>,
        Shuah Khan <shuah@kernel.org>
Subject: Re: [PATCH v1 1/3] mm/ksm: unmerge and clear VM_MERGEABLE when
 setting PR_SET_MEMORY_MERGE=0
Date:   Thu, 20 Apr 2023 14:21:42 -0700
In-reply-to: <20230418152849.505124-2-david@redhat.com>
Message-ID: <qvqwr0sei6sl.fsf@devbig1114.prn1.facebook.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


David Hildenbrand <david@redhat.com> writes:

> Let's unmerge any KSM pages when setting PR_SET_MEMORY_MERGE=0, and clear
> the VM_MERGEABLE flag from all VMAs -- just like KSM would. Of course,
> only do that if we previously set PR_SET_MEMORY_MERGE=1.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  include/linux/ksm.h |  1 +
>  kernel/sys.c        |  7 +------
>  mm/ksm.c            | 47 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 49 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/ksm.h b/include/linux/ksm.h
> index 590934bdddcf..7108bc65dc2a 100644
> --- a/include/linux/ksm.h
> +++ b/include/linux/ksm.h
> @@ -21,6 +21,7 @@ int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
>
>  void ksm_add_vma(struct vm_area_struct *vma);
>  int ksm_enable_merge_any(struct mm_struct *mm);
> +int ksm_disable_merge_any(struct mm_struct *mm);
>
>  int __ksm_enter(struct mm_struct *mm);
>  void __ksm_exit(struct mm_struct *mm);
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 72cdb16e2636..3436376667d7 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2698,12 +2698,7 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>  		if (arg2) {
>  			error = ksm_enable_merge_any(me->mm);
>  		} else {
> -			/*
> -			 * TODO: we might want disable KSM on all VMAs and
> -			 * trigger unsharing to completely disable KSM.
> -			 */
> -			clear_bit(MMF_VM_MERGE_ANY, &me->mm->flags);
> -			error = 0;
> +			error = ksm_disable_merge_any(me->mm);
>  		}
>
nit:
can we do:

    if (arg2)
   	error = ksm_enable_merge_any(me->mm);
	else
   	error = ksm_disable_merge_any(me->mm);
	mmap_write_unlock(me->mm);
	break;

> diff --git a/mm/ksm.c b/mm/ksm.c
> index a959e8925413..813f7fbc1832 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2520,6 +2520,22 @@ static void __ksm_add_vma(struct vm_area_struct *vma)
>  		vm_flags_set(vma, VM_MERGEABLE);
>  }
>
> +static int __ksm_del_vma(struct vm_area_struct *vma)
> +{
> +	int err;
> +
> +	if (!(vma->vm_flags & VM_MERGEABLE))
> +		return 0;
> +
> +	if (vma->anon_vma) {
> +		err = unmerge_ksm_pages(vma, vma->vm_start, vma->vm_end);
> +		if (err)
> +			return err;
> +	}
> +
> +	vm_flags_clear(vma, VM_MERGEABLE);
> +	return 0;
> +}
>  /**
>   * ksm_add_vma - Mark vma as mergeable if compatible
>   *
> @@ -2542,6 +2558,20 @@ static void ksm_add_vmas(struct mm_struct *mm)
>  		__ksm_add_vma(vma);
>  }
>
> +static int ksm_del_vmas(struct mm_struct *mm)
> +{
> +	struct vm_area_struct *vma;
> +	int err;
> +
> +	VMA_ITERATOR(vmi, mm, 0);
> +	for_each_vma(vmi, vma) {
> +		err = __ksm_del_vma(vma);
> +		if (err)
> +			return err;
> +	}
> +	return 0;
> +}
> +
>  /**
>   * ksm_enable_merge_any - Add mm to mm ksm list and enable merging on all
>   *                        compatible VMA's
> @@ -2569,6 +2599,23 @@ int ksm_enable_merge_any(struct mm_struct *mm)
>  	return 0;
>  }
>
> +int ksm_disable_merge_any(struct mm_struct *mm)
>

I understand we want to keep the name "symmetric" with
ksm_enable_merge_any, but it also unmerges the ksm pages. Do we want to
reflect that in the function name?

Can we add a comment for the function?

> +{
> +	int err;
> +
> +	if (!test_bit(MMF_VM_MERGE_ANY, &mm->flags))
> +		return 0;
> +
> +	err = ksm_del_vmas(mm);
> +	if (err) {
> +		ksm_add_vmas(mm);
> +		return err;
> +	}
> +
> +	clear_bit(MMF_VM_MERGE_ANY, &mm->flags);
>

We only clear the MMF_VM_MERGE_ANY flag if there are no errors. Is this
what we want? This means that if the process creates new memory regions
they would still be marked as mergeable.

> +	return 0;
> +}
> +
>  int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
>  		unsigned long end, int advice, unsigned long *vm_flags)
>  {