| 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 |
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>
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!
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) > {
[...] >> 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!
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 --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) {
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(-)