Re: [PATCH v2 5/6] mm: introduce mod_vm_flags_nolock and use it in untrack_pfn
On Wed, Jan 25, 2023 at 12:38:50AM -0800, Suren Baghdasaryan wrote: > In cases when VMA flags are modified after VMA was isolated and mmap_lock > was downgraded, flags modifications would result in an assertion because > mmap write lock is not held. > Introduce mod_vm_flags_nolock to be used in such situation. vm_flags_mod_nolock? > Pass a hint to untrack_pfn to conditionally use mod_vm_flags_nolock for > flags modification and to avoid assertion. > > Signed-off-by: Suren Baghdasaryan > --- > arch/x86/mm/pat/memtype.c | 10 +++--- > include/linux/mm.h| 12 +--- > include/linux/pgtable.h | 5 +++-- > mm/memory.c | 13 +++-- > mm/memremap.c | 4 ++-- > mm/mmap.c | 16 ++-- > 6 files changed, 38 insertions(+), 22 deletions(-) > > diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c > index ae9645c900fa..d8adc0b42cf2 100644 > --- a/arch/x86/mm/pat/memtype.c > +++ b/arch/x86/mm/pat/memtype.c > @@ -1046,7 +1046,7 @@ void track_pfn_insert(struct vm_area_struct *vma, > pgprot_t *prot, pfn_t pfn) > * can be for the entire vma (in which case pfn, size are zero). > */ > void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn, > - unsigned long size) > + unsigned long size, bool mm_wr_locked) > { > resource_size_t paddr; > unsigned long prot; > @@ -1065,8 +1065,12 @@ void untrack_pfn(struct vm_area_struct *vma, unsigned > long pfn, > size = vma->vm_end - vma->vm_start; > } > free_pfn_range(paddr, size); > - if (vma) > - clear_vm_flags(vma, VM_PAT); > + if (vma) { > + if (mm_wr_locked) > + clear_vm_flags(vma, VM_PAT); > + else > + mod_vm_flags_nolock(vma, 0, VM_PAT); > + } > } > > /* > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 55335edd1373..48d49930c411 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -656,12 +656,18 @@ static inline void clear_vm_flags(struct vm_area_struct > *vma, > vma->vm_flags &= ~flags; > } > > +static inline void mod_vm_flags_nolock(struct vm_area_struct *vma, > +unsigned long set, unsigned long clear) > +{ > + vma->vm_flags |= set; > + vma->vm_flags &= ~clear; > +} > + > static inline void mod_vm_flags(struct vm_area_struct *vma, > unsigned long set, unsigned long clear) > { > mmap_assert_write_locked(vma->vm_mm); > - vma->vm_flags |= set; > - vma->vm_flags &= ~clear; > + mod_vm_flags_nolock(vma, set, clear); > } > > static inline void vma_set_anonymous(struct vm_area_struct *vma) > @@ -2087,7 +2093,7 @@ static inline void zap_vma_pages(struct vm_area_struct > *vma) > } > void unmap_vmas(struct mmu_gather *tlb, struct maple_tree *mt, > struct vm_area_struct *start_vma, unsigned long start, > - unsigned long end); > + unsigned long end, bool mm_wr_locked); > > struct mmu_notifier_range; > > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h > index 5fd45454c073..c63cd44777ec 100644 > --- a/include/linux/pgtable.h > +++ b/include/linux/pgtable.h > @@ -1185,7 +1185,8 @@ static inline int track_pfn_copy(struct vm_area_struct > *vma) > * can be for the entire vma (in which case pfn, size are zero). > */ > static inline void untrack_pfn(struct vm_area_struct *vma, > -unsigned long pfn, unsigned long size) > +unsigned long pfn, unsigned long size, > +bool mm_wr_locked) > { > } > > @@ -1203,7 +1204,7 @@ extern void track_pfn_insert(struct vm_area_struct > *vma, pgprot_t *prot, >pfn_t pfn); > extern int track_pfn_copy(struct vm_area_struct *vma); > extern void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn, > - unsigned long size); > + unsigned long size, bool mm_wr_locked); > extern void untrack_pfn_moved(struct vm_area_struct *vma); > #endif > > diff --git a/mm/memory.c b/mm/memory.c > index d6902065e558..5b11b50e2c4a 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -1613,7 +1613,7 @@ void unmap_page_range(struct mmu_gather *tlb, > static void unmap_single_vma(struct mmu_gather *tlb, > struct vm_area_struct *vma, unsigned long start_addr, > unsigned long end_addr, > - struct zap_details *details) > + struct zap_details *details, bool mm_wr_locked) > { > unsigned long start = max(vma->vm_start, start_addr); > unsigned long end; > @@ -1628,7 +1628,7 @@ static void unmap_single_vma(struct mmu_gather *tlb, > uprobe_munmap(vma, start, end); > > if (unlikely(vma->vm_flags & VM_PFNMAP)) > - untrack_pfn(vma, 0, 0); > + untrack_pfn(vma, 0, 0,
Re: [PATCH v2 5/6] mm: introduce mod_vm_flags_nolock and use it in untrack_pfn
On Wed, Jan 25, 2023 at 1:42 AM Michal Hocko wrote: > > On Wed 25-01-23 00:38:50, Suren Baghdasaryan wrote: > > In cases when VMA flags are modified after VMA was isolated and mmap_lock > > was downgraded, flags modifications would result in an assertion because > > mmap write lock is not held. > > Introduce mod_vm_flags_nolock to be used in such situation. > > Pass a hint to untrack_pfn to conditionally use mod_vm_flags_nolock for > > flags modification and to avoid assertion. > > The changelog nor the documentation of mod_vm_flags_nolock > really explain when it is safe to use it. This is really important for > future potential users. True. I'll add clarification in the comments and in the changelog. Thanks! > > > Signed-off-by: Suren Baghdasaryan > > --- > > arch/x86/mm/pat/memtype.c | 10 +++--- > > include/linux/mm.h| 12 +--- > > include/linux/pgtable.h | 5 +++-- > > mm/memory.c | 13 +++-- > > mm/memremap.c | 4 ++-- > > mm/mmap.c | 16 ++-- > > 6 files changed, 38 insertions(+), 22 deletions(-) > > > > diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c > > index ae9645c900fa..d8adc0b42cf2 100644 > > --- a/arch/x86/mm/pat/memtype.c > > +++ b/arch/x86/mm/pat/memtype.c > > @@ -1046,7 +1046,7 @@ void track_pfn_insert(struct vm_area_struct *vma, > > pgprot_t *prot, pfn_t pfn) > > * can be for the entire vma (in which case pfn, size are zero). > > */ > > void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn, > > - unsigned long size) > > + unsigned long size, bool mm_wr_locked) > > { > > resource_size_t paddr; > > unsigned long prot; > > @@ -1065,8 +1065,12 @@ void untrack_pfn(struct vm_area_struct *vma, > > unsigned long pfn, > > size = vma->vm_end - vma->vm_start; > > } > > free_pfn_range(paddr, size); > > - if (vma) > > - clear_vm_flags(vma, VM_PAT); > > + if (vma) { > > + if (mm_wr_locked) > > + clear_vm_flags(vma, VM_PAT); > > + else > > + mod_vm_flags_nolock(vma, 0, VM_PAT); > > + } > > } > > > > /* > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 55335edd1373..48d49930c411 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -656,12 +656,18 @@ static inline void clear_vm_flags(struct > > vm_area_struct *vma, > > vma->vm_flags &= ~flags; > > } > > > > +static inline void mod_vm_flags_nolock(struct vm_area_struct *vma, > > +unsigned long set, unsigned long clear) > > +{ > > + vma->vm_flags |= set; > > + vma->vm_flags &= ~clear; > > +} > > + > > static inline void mod_vm_flags(struct vm_area_struct *vma, > > unsigned long set, unsigned long clear) > > { > > mmap_assert_write_locked(vma->vm_mm); > > - vma->vm_flags |= set; > > - vma->vm_flags &= ~clear; > > + mod_vm_flags_nolock(vma, set, clear); > > } > > > > static inline void vma_set_anonymous(struct vm_area_struct *vma) > > @@ -2087,7 +2093,7 @@ static inline void zap_vma_pages(struct > > vm_area_struct *vma) > > } > > void unmap_vmas(struct mmu_gather *tlb, struct maple_tree *mt, > > struct vm_area_struct *start_vma, unsigned long start, > > - unsigned long end); > > + unsigned long end, bool mm_wr_locked); > > > > struct mmu_notifier_range; > > > > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h > > index 5fd45454c073..c63cd44777ec 100644 > > --- a/include/linux/pgtable.h > > +++ b/include/linux/pgtable.h > > @@ -1185,7 +1185,8 @@ static inline int track_pfn_copy(struct > > vm_area_struct *vma) > > * can be for the entire vma (in which case pfn, size are zero). > > */ > > static inline void untrack_pfn(struct vm_area_struct *vma, > > -unsigned long pfn, unsigned long size) > > +unsigned long pfn, unsigned long size, > > +bool mm_wr_locked) > > { > > } > > > > @@ -1203,7 +1204,7 @@ extern void track_pfn_insert(struct vm_area_struct > > *vma, pgprot_t *prot, > >pfn_t pfn); > > extern int track_pfn_copy(struct vm_area_struct *vma); > > extern void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn, > > - unsigned long size); > > + unsigned long size, bool mm_wr_locked); > > extern void untrack_pfn_moved(struct vm_area_struct *vma); > > #endif > > > > diff --git a/mm/memory.c b/mm/memory.c > > index d6902065e558..5b11b50e2c4a 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -1613,7 +1613,7 @@ void unmap_page_range(struct mmu_gather *tlb, > > static void unmap_single_vma(struct mmu_gather *tlb, > > struct vm_area_struct *vma, unsigned long start_addr, > > unsigned long
Re: [PATCH v2 5/6] mm: introduce mod_vm_flags_nolock and use it in untrack_pfn
On Wed 25-01-23 00:38:50, Suren Baghdasaryan wrote: > In cases when VMA flags are modified after VMA was isolated and mmap_lock > was downgraded, flags modifications would result in an assertion because > mmap write lock is not held. > Introduce mod_vm_flags_nolock to be used in such situation. > Pass a hint to untrack_pfn to conditionally use mod_vm_flags_nolock for > flags modification and to avoid assertion. The changelog nor the documentation of mod_vm_flags_nolock really explain when it is safe to use it. This is really important for future potential users. > Signed-off-by: Suren Baghdasaryan > --- > arch/x86/mm/pat/memtype.c | 10 +++--- > include/linux/mm.h| 12 +--- > include/linux/pgtable.h | 5 +++-- > mm/memory.c | 13 +++-- > mm/memremap.c | 4 ++-- > mm/mmap.c | 16 ++-- > 6 files changed, 38 insertions(+), 22 deletions(-) > > diff --git a/arch/x86/mm/pat/memtype.c b/arch/x86/mm/pat/memtype.c > index ae9645c900fa..d8adc0b42cf2 100644 > --- a/arch/x86/mm/pat/memtype.c > +++ b/arch/x86/mm/pat/memtype.c > @@ -1046,7 +1046,7 @@ void track_pfn_insert(struct vm_area_struct *vma, > pgprot_t *prot, pfn_t pfn) > * can be for the entire vma (in which case pfn, size are zero). > */ > void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn, > - unsigned long size) > + unsigned long size, bool mm_wr_locked) > { > resource_size_t paddr; > unsigned long prot; > @@ -1065,8 +1065,12 @@ void untrack_pfn(struct vm_area_struct *vma, unsigned > long pfn, > size = vma->vm_end - vma->vm_start; > } > free_pfn_range(paddr, size); > - if (vma) > - clear_vm_flags(vma, VM_PAT); > + if (vma) { > + if (mm_wr_locked) > + clear_vm_flags(vma, VM_PAT); > + else > + mod_vm_flags_nolock(vma, 0, VM_PAT); > + } > } > > /* > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 55335edd1373..48d49930c411 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -656,12 +656,18 @@ static inline void clear_vm_flags(struct vm_area_struct > *vma, > vma->vm_flags &= ~flags; > } > > +static inline void mod_vm_flags_nolock(struct vm_area_struct *vma, > +unsigned long set, unsigned long clear) > +{ > + vma->vm_flags |= set; > + vma->vm_flags &= ~clear; > +} > + > static inline void mod_vm_flags(struct vm_area_struct *vma, > unsigned long set, unsigned long clear) > { > mmap_assert_write_locked(vma->vm_mm); > - vma->vm_flags |= set; > - vma->vm_flags &= ~clear; > + mod_vm_flags_nolock(vma, set, clear); > } > > static inline void vma_set_anonymous(struct vm_area_struct *vma) > @@ -2087,7 +2093,7 @@ static inline void zap_vma_pages(struct vm_area_struct > *vma) > } > void unmap_vmas(struct mmu_gather *tlb, struct maple_tree *mt, > struct vm_area_struct *start_vma, unsigned long start, > - unsigned long end); > + unsigned long end, bool mm_wr_locked); > > struct mmu_notifier_range; > > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h > index 5fd45454c073..c63cd44777ec 100644 > --- a/include/linux/pgtable.h > +++ b/include/linux/pgtable.h > @@ -1185,7 +1185,8 @@ static inline int track_pfn_copy(struct vm_area_struct > *vma) > * can be for the entire vma (in which case pfn, size are zero). > */ > static inline void untrack_pfn(struct vm_area_struct *vma, > -unsigned long pfn, unsigned long size) > +unsigned long pfn, unsigned long size, > +bool mm_wr_locked) > { > } > > @@ -1203,7 +1204,7 @@ extern void track_pfn_insert(struct vm_area_struct > *vma, pgprot_t *prot, >pfn_t pfn); > extern int track_pfn_copy(struct vm_area_struct *vma); > extern void untrack_pfn(struct vm_area_struct *vma, unsigned long pfn, > - unsigned long size); > + unsigned long size, bool mm_wr_locked); > extern void untrack_pfn_moved(struct vm_area_struct *vma); > #endif > > diff --git a/mm/memory.c b/mm/memory.c > index d6902065e558..5b11b50e2c4a 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -1613,7 +1613,7 @@ void unmap_page_range(struct mmu_gather *tlb, > static void unmap_single_vma(struct mmu_gather *tlb, > struct vm_area_struct *vma, unsigned long start_addr, > unsigned long end_addr, > - struct zap_details *details) > + struct zap_details *details, bool mm_wr_locked) > { > unsigned long start = max(vma->vm_start, start_addr); > unsigned long end; > @@ -1628,7 +1628,7 @@ static void unmap_single_vma(struct mmu_gather *tlb, > uprobe_munmap(vma, start, end); > > if