diff mbox series

[v1,3/3] mm/ksm: move disabling KSM from s390/gmap code to KSM code

Message ID 20230418152849.505124-4-david@redhat.com
State New
Headers show
Series mm/ksm: improve PR_SET_MEMORY_MERGE=0 handling and cleanup disabling KSM | expand

Commit Message

David Hildenbrand April 18, 2023, 3:28 p.m. UTC
Let's factor out actual disabling of KSM. The existing
"mm->def_flags &= ~VM_MERGEABLE;" was essentially a NOP and can be dropped,
because def_flags should never include VM_MERGEABLE. Note that we don't
currently prevent re-enabling KSM.

This should now be faster in case KSM was never enabled, because we only
conditionally iterate all VMAs. Further, it certainly looks cleaner.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 arch/s390/mm/gmap.c | 20 +-------------------
 include/linux/ksm.h |  6 ++++++
 mm/ksm.c            | 11 +++++++++++
 3 files changed, 18 insertions(+), 19 deletions(-)

Comments

Janosch Frank April 19, 2023, 11:39 a.m. UTC | #1
On 4/18/23 17:28, David Hildenbrand wrote:
> Let's factor out actual disabling of KSM. The existing
> "mm->def_flags &= ~VM_MERGEABLE;" was essentially a NOP and can be dropped,
> because def_flags should never include VM_MERGEABLE. Note that we don't
> currently prevent re-enabling KSM.
> 
> This should now be faster in case KSM was never enabled, because we only
> conditionally iterate all VMAs. Further, it certainly looks cleaner.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   arch/s390/mm/gmap.c | 20 +-------------------
>   include/linux/ksm.h |  6 ++++++
>   mm/ksm.c            | 11 +++++++++++
>   3 files changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index 0949811761e6..dfe905c7bd8e 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2585,30 +2585,12 @@ EXPORT_SYMBOL_GPL(s390_enable_sie);
>   
>   int gmap_mark_unmergeable(void)
>   {
> -	struct mm_struct *mm = current->mm;
> -	struct vm_area_struct *vma;
> -	unsigned long vm_flags;
> -	int ret;
> -	VMA_ITERATOR(vmi, mm, 0);
> -
>   	/*
>   	 * Make sure to disable KSM (if enabled for the whole process or
>   	 * individual VMAs). Note that nothing currently hinders user space
>   	 * from re-enabling it.
>   	 */

Is that still true?

My KSM knowledge is nearly zero but from what I can see the patch looks 
ok to me:
Acked-by: Janosch Frank <frankja@linux.ibm.net>
David Hildenbrand April 19, 2023, 11:40 a.m. UTC | #2
On 19.04.23 13:39, Janosch Frank wrote:
> On 4/18/23 17:28, David Hildenbrand wrote:
>> Let's factor out actual disabling of KSM. The existing
>> "mm->def_flags &= ~VM_MERGEABLE;" was essentially a NOP and can be dropped,
>> because def_flags should never include VM_MERGEABLE. Note that we don't
>> currently prevent re-enabling KSM.
>>
>> This should now be faster in case KSM was never enabled, because we only
>> conditionally iterate all VMAs. Further, it certainly looks cleaner.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>    arch/s390/mm/gmap.c | 20 +-------------------
>>    include/linux/ksm.h |  6 ++++++
>>    mm/ksm.c            | 11 +++++++++++
>>    3 files changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index 0949811761e6..dfe905c7bd8e 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -2585,30 +2585,12 @@ EXPORT_SYMBOL_GPL(s390_enable_sie);
>>    
>>    int gmap_mark_unmergeable(void)
>>    {
>> -	struct mm_struct *mm = current->mm;
>> -	struct vm_area_struct *vma;
>> -	unsigned long vm_flags;
>> -	int ret;
>> -	VMA_ITERATOR(vmi, mm, 0);
>> -
>>    	/*
>>    	 * Make sure to disable KSM (if enabled for the whole process or
>>    	 * individual VMAs). Note that nothing currently hinders user space
>>    	 * from re-enabling it.
>>    	 */
> 
> Is that still true?

Yes. We'd need another per-MM bit to stop it from getting re-enabled.

> 
> My KSM knowledge is nearly zero but from what I can see the patch looks
> ok to me:
> Acked-by: Janosch Frank <frankja@linux.ibm.net>


Thanks!
Stefan Roesch April 20, 2023, 9:41 p.m. UTC | #3
David Hildenbrand <david@redhat.com> writes:

> Let's factor out actual disabling of KSM. The existing
> "mm->def_flags &= ~VM_MERGEABLE;" was essentially a NOP and can be dropped,
> because def_flags should never include VM_MERGEABLE. Note that we don't
> currently prevent re-enabling KSM.
>
> This should now be faster in case KSM was never enabled, because we only
> conditionally iterate all VMAs. Further, it certainly looks cleaner.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  arch/s390/mm/gmap.c | 20 +-------------------
>  include/linux/ksm.h |  6 ++++++
>  mm/ksm.c            | 11 +++++++++++
>  3 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index 0949811761e6..dfe905c7bd8e 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2585,30 +2585,12 @@ EXPORT_SYMBOL_GPL(s390_enable_sie);
>
>  int gmap_mark_unmergeable(void)
>  {
> -	struct mm_struct *mm = current->mm;
> -	struct vm_area_struct *vma;
> -	unsigned long vm_flags;
> -	int ret;
> -	VMA_ITERATOR(vmi, mm, 0);
> -
>  	/*
>  	 * Make sure to disable KSM (if enabled for the whole process or
>  	 * individual VMAs). Note that nothing currently hinders user space
>  	 * from re-enabling it.
>  	 */
> -	clear_bit(MMF_VM_MERGE_ANY, &mm->flags);
> -
> -	for_each_vma(vmi, vma) {
> -		/* Copy vm_flags to avoid partial modifications in ksm_madvise */
> -		vm_flags = vma->vm_flags;
> -		ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
> -				  MADV_UNMERGEABLE, &vm_flags);
> -		if (ret)
> -			return ret;
> -		vm_flags_reset(vma, vm_flags);
> -	}
> -	mm->def_flags &= ~VM_MERGEABLE;
>

This clears the def_flags struct member, however, in ksm_disable() we
clear the __flags struct member. Is this a problem?

> -	return 0;
> +	return ksm_disable(current->mm);
>  }
>  EXPORT_SYMBOL_GPL(gmap_mark_unmergeable);
>
> diff --git a/include/linux/ksm.h b/include/linux/ksm.h
> index 7108bc65dc2a..b3d8b7849e18 100644
> --- a/include/linux/ksm.h
> +++ b/include/linux/ksm.h
> @@ -22,6 +22,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_disable(struct mm_struct *mm);
>
>  int __ksm_enter(struct mm_struct *mm);
>  void __ksm_exit(struct mm_struct *mm);
> @@ -75,6 +76,11 @@ static inline void ksm_add_vma(struct vm_area_struct *vma)
>  {
>  }
>
> +static inline int ksm_disable(struct mm_struct *mm)
> +{
> +	return 0;
> +}
> +
>  static inline int ksm_fork(struct mm_struct *mm, struct mm_struct *oldmm)
>  {
>  	return 0;
> diff --git a/mm/ksm.c b/mm/ksm.c
> index 813f7fbc1832..208311cbb019 100644
> --- a/mm/ksm.c
> +++ b/mm/ksm.c
> @@ -2616,6 +2616,17 @@ int ksm_disable_merge_any(struct mm_struct *mm)
>  	return 0;
>  }
>
> +int ksm_disable(struct mm_struct *mm)
> +{
> +	mmap_assert_write_locked(mm);
> +
> +	if (!test_bit(MMF_VM_MERGEABLE, &mm->flags))
> +		return 0;
> +	if (test_bit(MMF_VM_MERGE_ANY, &mm->flags))
> +		return ksm_disable_merge_any(mm);
> +	return ksm_del_vmas(mm);
> +}
> +
>  int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
>  		unsigned long end, int advice, unsigned long *vm_flags)
>  {
David Hildenbrand April 21, 2023, 4:52 p.m. UTC | #4
[...]

>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>> index 0949811761e6..dfe905c7bd8e 100644
>> --- a/arch/s390/mm/gmap.c
>> +++ b/arch/s390/mm/gmap.c
>> @@ -2585,30 +2585,12 @@ EXPORT_SYMBOL_GPL(s390_enable_sie);
>>
>>   int gmap_mark_unmergeable(void)
>>   {
>> -	struct mm_struct *mm = current->mm;
>> -	struct vm_area_struct *vma;
>> -	unsigned long vm_flags;
>> -	int ret;
>> -	VMA_ITERATOR(vmi, mm, 0);
>> -
>>   	/*
>>   	 * Make sure to disable KSM (if enabled for the whole process or
>>   	 * individual VMAs). Note that nothing currently hinders user space
>>   	 * from re-enabling it.
>>   	 */
>> -	clear_bit(MMF_VM_MERGE_ANY, &mm->flags);
>> -
>> -	for_each_vma(vmi, vma) {
>> -		/* Copy vm_flags to avoid partial modifications in ksm_madvise */
>> -		vm_flags = vma->vm_flags;
>> -		ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
>> -				  MADV_UNMERGEABLE, &vm_flags);
>> -		if (ret)
>> -			return ret;
>> -		vm_flags_reset(vma, vm_flags);
>> -	}
>> -	mm->def_flags &= ~VM_MERGEABLE;
>>
> 

Hi Stefan,

> This clears the def_flags struct member, however, in ksm_disable() we
> clear the __flags struct member. Is this a problem?

The patch description contains a comment regarding def_flags: "The 
existing "mm->def_flags &= ~VM_MERGEABLE;" was essentially a NOP and can 
be dropped, because def_flags should never include VM_MERGEABLE."

We keep clearing the MADV_UNMERGEABLE flag from MADV_UNMERGEABLE. In the 
old code, ksm_madvise() would have cleared it from local vm_flags and 
vm_flags_reset() would have modified vma->vm_flags. Now we clear it 
directly via vm_flags_clear(vma, VM_MERGEABLE);


Long story short, the mm->def_flags code as wrong and most probably 
copied from thp_split_mm() where we do:
	mm->def_flags |= VM_NOHUGEPAGE;
Which makes more sense.

Thanks!
Stefan Roesch April 21, 2023, 6:27 p.m. UTC | #5
David Hildenbrand <david@redhat.com> writes:

> [...]
>
>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>>> index 0949811761e6..dfe905c7bd8e 100644
>>> --- a/arch/s390/mm/gmap.c
>>> +++ b/arch/s390/mm/gmap.c
>>> @@ -2585,30 +2585,12 @@ EXPORT_SYMBOL_GPL(s390_enable_sie);
>>>
>>>   int gmap_mark_unmergeable(void)
>>>   {
>>> -	struct mm_struct *mm = current->mm;
>>> -	struct vm_area_struct *vma;
>>> -	unsigned long vm_flags;
>>> -	int ret;
>>> -	VMA_ITERATOR(vmi, mm, 0);
>>> -
>>>   	/*
>>>   	 * Make sure to disable KSM (if enabled for the whole process or
>>>   	 * individual VMAs). Note that nothing currently hinders user space
>>>   	 * from re-enabling it.
>>>   	 */
>>> -	clear_bit(MMF_VM_MERGE_ANY, &mm->flags);
>>> -
>>> -	for_each_vma(vmi, vma) {
>>> -		/* Copy vm_flags to avoid partial modifications in ksm_madvise */
>>> -		vm_flags = vma->vm_flags;
>>> -		ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
>>> -				  MADV_UNMERGEABLE, &vm_flags);
>>> -		if (ret)
>>> -			return ret;
>>> -		vm_flags_reset(vma, vm_flags);
>>> -	}
>>> -	mm->def_flags &= ~VM_MERGEABLE;
>>>
>>
>
> Hi Stefan,
>
>> This clears the def_flags struct member, however, in ksm_disable() we
>> clear the __flags struct member. Is this a problem?
>
> The patch description contains a comment regarding def_flags: "The existing
> "mm->def_flags &= ~VM_MERGEABLE;" was essentially a NOP and can be dropped,
> because def_flags should never include VM_MERGEABLE."
>
> We keep clearing the MADV_UNMERGEABLE flag from MADV_UNMERGEABLE. In the old
> code, ksm_madvise() would have cleared it from local vm_flags and
> vm_flags_reset() would have modified vma->vm_flags. Now we clear it directly via
> vm_flags_clear(vma, VM_MERGEABLE);
>
>
> Long story short, the mm->def_flags code as wrong and most probably copied from
> thp_split_mm() where we do:
> 	mm->def_flags |= VM_NOHUGEPAGE;
> Which makes more sense.
>
> Thanks!

Thanks for the explanation.

Acked-by: Stefan Roesch <shr@devkernel.io>
diff mbox series

Patch

diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index 0949811761e6..dfe905c7bd8e 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -2585,30 +2585,12 @@  EXPORT_SYMBOL_GPL(s390_enable_sie);
 
 int gmap_mark_unmergeable(void)
 {
-	struct mm_struct *mm = current->mm;
-	struct vm_area_struct *vma;
-	unsigned long vm_flags;
-	int ret;
-	VMA_ITERATOR(vmi, mm, 0);
-
 	/*
 	 * Make sure to disable KSM (if enabled for the whole process or
 	 * individual VMAs). Note that nothing currently hinders user space
 	 * from re-enabling it.
 	 */
-	clear_bit(MMF_VM_MERGE_ANY, &mm->flags);
-
-	for_each_vma(vmi, vma) {
-		/* Copy vm_flags to avoid partial modifications in ksm_madvise */
-		vm_flags = vma->vm_flags;
-		ret = ksm_madvise(vma, vma->vm_start, vma->vm_end,
-				  MADV_UNMERGEABLE, &vm_flags);
-		if (ret)
-			return ret;
-		vm_flags_reset(vma, vm_flags);
-	}
-	mm->def_flags &= ~VM_MERGEABLE;
-	return 0;
+	return ksm_disable(current->mm);
 }
 EXPORT_SYMBOL_GPL(gmap_mark_unmergeable);
 
diff --git a/include/linux/ksm.h b/include/linux/ksm.h
index 7108bc65dc2a..b3d8b7849e18 100644
--- a/include/linux/ksm.h
+++ b/include/linux/ksm.h
@@ -22,6 +22,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_disable(struct mm_struct *mm);
 
 int __ksm_enter(struct mm_struct *mm);
 void __ksm_exit(struct mm_struct *mm);
@@ -75,6 +76,11 @@  static inline void ksm_add_vma(struct vm_area_struct *vma)
 {
 }
 
+static inline int ksm_disable(struct mm_struct *mm)
+{
+	return 0;
+}
+
 static inline int ksm_fork(struct mm_struct *mm, struct mm_struct *oldmm)
 {
 	return 0;
diff --git a/mm/ksm.c b/mm/ksm.c
index 813f7fbc1832..208311cbb019 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2616,6 +2616,17 @@  int ksm_disable_merge_any(struct mm_struct *mm)
 	return 0;
 }
 
+int ksm_disable(struct mm_struct *mm)
+{
+	mmap_assert_write_locked(mm);
+
+	if (!test_bit(MMF_VM_MERGEABLE, &mm->flags))
+		return 0;
+	if (test_bit(MMF_VM_MERGE_ANY, &mm->flags))
+		return ksm_disable_merge_any(mm);
+	return ksm_del_vmas(mm);
+}
+
 int ksm_madvise(struct vm_area_struct *vma, unsigned long start,
 		unsigned long end, int advice, unsigned long *vm_flags)
 {