| Message ID | 20230418152849.505124-2-david@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/ksm: improve PR_SET_MEMORY_MERGE=0 handling and cleanup disabling KSM | expand |
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) > {
[...] >> > 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; Indeed, thanks. > >> 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? ksm_disable_merge_any_umerge() is suboptimal. As ksm_disable_merge_any() now reverts what ksm_enable_merge_any() ended up doing, I think it's just fine. (it would be a different story if we'd be using "set" / "clear" terminology instead of "enable" / "disable"). We can describe that in the comment. > > Can we add a comment for the function? Can do for symmetry with ksm_enable_merge_any(). But note that I don't think documentation for functions is of any help when it takes longer to read the documentation than to read+understand the actual code. > >> +{ >> + 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 I think this is the behavior we want. We tried to disable KSM for the process (previously enabled via the prctl), but cannot disable KSM. So we rollback our changes and return an error. This is similar to trying to set MADV_UNMERGEABLE but failing. We leave the bit set. > what we want? This means that if the process creates new memory regions > they would still be marked as mergeable. Yes, we failed the operation so we keep everything unchanged.
>> 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? > > ksm_disable_merge_any_umerge() is suboptimal. > > As ksm_disable_merge_any() now reverts what ksm_enable_merge_any() ended > up doing, I think it's just fine. > > (it would be a different story if we'd be using "set" / "clear" > terminology instead of "enable" / "disable"). > > We can describe that in the comment. > >> >> Can we add a comment for the function? > > Can do for symmetry with ksm_enable_merge_any(). > +/** + * ksm_disable_merge_any - Disable merging on all compatible VMA's of the mm, + * previously enabled via ksm_enable_merge_any(). + * + * Disabling merging implies unmerging any merged pages, like setting + * MADV_UNMERGEABLE would. If unmerging fails, the whole operation fails and + * merging on all compatible VMA's remains enabled. + * + * @mm: Pointer to mm + * + * Returns 0 on success, otherwise error code + */
David Hildenbrand <david@redhat.com> writes: >>> 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? >> ksm_disable_merge_any_umerge() is suboptimal. >> As ksm_disable_merge_any() now reverts what ksm_enable_merge_any() ended >> up doing, I think it's just fine. >> (it would be a different story if we'd be using "set" / "clear" >> terminology instead of "enable" / "disable"). >> We can describe that in the comment. >> >>> >>> Can we add a comment for the function? >> Can do for symmetry with ksm_enable_merge_any(). >> > > +/** > + * ksm_disable_merge_any - Disable merging on all compatible VMA's of the mm, > + * previously enabled via ksm_enable_merge_any(). > + * > + * Disabling merging implies unmerging any merged pages, like setting > + * MADV_UNMERGEABLE would. If unmerging fails, the whole operation fails and > + * merging on all compatible VMA's remains enabled. > + * > + * @mm: Pointer to mm > + * > + * Returns 0 on success, otherwise error code > + */ LGTM Acked-by: Stefan Roesch <shr@devkernel.io>
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); } 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) +{ + 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); + return 0; +} + int ksm_madvise(struct vm_area_struct *vma, unsigned long start, unsigned long end, int advice, unsigned long *vm_flags) {
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(-)