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 <[email protected]>
---
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;
- 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)
{
--
2.39.2
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 <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
>> ---
>> 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 <[email protected]>
Thanks!
--
Thanks,
David / dhildenb
David Hildenbrand <[email protected]> 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 <[email protected]>
> ---
> 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)
> {
[...]
>> 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,
David / dhildenb
David Hildenbrand <[email protected]> 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 <[email protected]>