Re: [PATCH v1 1/3] mm/ksm: unmerge and clear VM_MERGEABLE when setting PR_SET_MEMORY_MERGE=0

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) {