Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Thu, Jan 26, 2023 at 7:09 AM Matthew Wilcox wrote: > > On Thu, Jan 26, 2023 at 04:50:59PM +0200, Mike Rapoport wrote: > > On Thu, Jan 26, 2023 at 11:17:09AM +0200, Mike Rapoport wrote: > > > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > > > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > > > > +static inline void init_vm_flags(struct vm_area_struct *vma, > > > > + unsigned long flags) > > > > > > I'd suggest to make it vm_flags_init() etc. > > > > Thinking more about it, it will be even clearer to name these > > vma_flags_xyz() > > Perhaps vma_VERB_flags()? > > vma_init_flags() > vma_reset_flags() > vma_set_flags() > vma_clear_flags() > vma_mod_flags() Due to excessive email bouncing I posted the v3 of this patchset using the original per-VMA patchset's distribution list. That might have dropped Mike from the list. Sorry about that Mike, I'll add you to my usual list of suspects :) The v3 is here: https://lore.kernel.org/all/20230125233554.153109-1-sur...@google.com/ and Andrew did suggest the same renames, so I'll be posting v4 with those changes later today. Thanks for the feedback! >
Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Thu, Jan 26, 2023 at 11:17:09AM +0200, Mike Rapoport wrote: > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > > vm_flags are among VMA attributes which affect decisions like VMA merging > > and splitting. Therefore all vm_flags modifications are performed after > > taking exclusive mmap_lock to prevent vm_flags updates racing with such > > operations. Introduce modifier functions for vm_flags to be used whenever > > flags are updated. This way we can better check and control correct > > locking behavior during these updates. > > > > Signed-off-by: Suren Baghdasaryan > > --- > > include/linux/mm.h | 37 + > > include/linux/mm_types.h | 8 +++- > > 2 files changed, 44 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index c2f62bdce134..b71f2809caac 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -627,6 +627,43 @@ static inline void vma_init(struct vm_area_struct > > *vma, struct mm_struct *mm) > > INIT_LIST_HEAD(&vma->anon_vma_chain); > > } > > > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > > +static inline void init_vm_flags(struct vm_area_struct *vma, > > +unsigned long flags) > > I'd suggest to make it vm_flags_init() etc. Thinking more about it, it will be even clearer to name these vma_flags_xyz() > Except that > > Acked-by: Mike Rapoport (IBM) > -- Sincerely yours, Mike.
Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Thu, Jan 26, 2023 at 04:50:59PM +0200, Mike Rapoport wrote: > On Thu, Jan 26, 2023 at 11:17:09AM +0200, Mike Rapoport wrote: > > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > > > +static inline void init_vm_flags(struct vm_area_struct *vma, > > > + unsigned long flags) > > > > I'd suggest to make it vm_flags_init() etc. > > Thinking more about it, it will be even clearer to name these vma_flags_xyz() Perhaps vma_VERB_flags()? vma_init_flags() vma_reset_flags() vma_set_flags() vma_clear_flags() vma_mod_flags()
Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > vm_flags are among VMA attributes which affect decisions like VMA merging > and splitting. Therefore all vm_flags modifications are performed after > taking exclusive mmap_lock to prevent vm_flags updates racing with such > operations. Introduce modifier functions for vm_flags to be used whenever > flags are updated. This way we can better check and control correct > locking behavior during these updates. > > Signed-off-by: Suren Baghdasaryan > --- > include/linux/mm.h | 37 + > include/linux/mm_types.h | 8 +++- > 2 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index c2f62bdce134..b71f2809caac 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -627,6 +627,43 @@ static inline void vma_init(struct vm_area_struct *vma, > struct mm_struct *mm) > INIT_LIST_HEAD(&vma->anon_vma_chain); > } > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > +static inline void init_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) I'd suggest to make it vm_flags_init() etc. Except that Acked-by: Mike Rapoport (IBM) > +{ > + vma->vm_flags = flags; > +} > + > +/* Use when VMA is part of the VMA tree and modifications need coordination > */ > +static inline void reset_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + init_vm_flags(vma, flags); > +} > + > +static inline void set_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + vma->vm_flags |= flags; > +} > + > +static inline void clear_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + vma->vm_flags &= ~flags; > +} > + > +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; > +} > + > static inline void vma_set_anonymous(struct vm_area_struct *vma) > { > vma->vm_ops = NULL; > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > index 2d6d790d9bed..6c7c70bf50dd 100644 > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -491,7 +491,13 @@ struct vm_area_struct { >* See vmf_insert_mixed_prot() for discussion. >*/ > pgprot_t vm_page_prot; > - unsigned long vm_flags; /* Flags, see mm.h. */ > + > + /* > + * Flags, see mm.h. > + * WARNING! Do not modify directly. > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > + */ > + unsigned long vm_flags; > > /* >* For areas with an address space and backing store, > -- > 2.39.1 > >
Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > index 2d6d790d9bed..6c7c70bf50dd 100644 > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -491,7 +491,13 @@ struct vm_area_struct { >* See vmf_insert_mixed_prot() for discussion. >*/ > pgprot_t vm_page_prot; > - unsigned long vm_flags; /* Flags, see mm.h. */ > + > + /* > + * Flags, see mm.h. > + * WARNING! Do not modify directly. > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > + */ > + unsigned long vm_flags; We have __private and ACCESS_PRIVATE() to help with enforcing this.
Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > +/* Use when VMA is not part of the VMA tree and needs no locking */ > +static inline void init_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + vma->vm_flags = flags; vm_flags are supposed to have type vm_flags_t. That's not been fully realised yet, but perhaps we could avoid making it worse? > pgprot_t vm_page_prot; > - unsigned long vm_flags; /* Flags, see mm.h. */ > + > + /* > + * Flags, see mm.h. > + * WARNING! Do not modify directly. > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > + */ > + unsigned long vm_flags; Including changing this line to vm_flags_t
Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Wed, Jan 25, 2023 at 10:37 AM Matthew Wilcox wrote: > > On Wed, Jan 25, 2023 at 08:49:50AM -0800, Suren Baghdasaryan wrote: > > On Wed, Jan 25, 2023 at 1:10 AM Peter Zijlstra wrote: > > > > + /* > > > > + * Flags, see mm.h. > > > > + * WARNING! Do not modify directly. > > > > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > > > > + */ > > > > + unsigned long vm_flags; > > > > > > We have __private and ACCESS_PRIVATE() to help with enforcing this. > > > > Thanks for pointing this out, Peter! I guess for that I'll need to > > convert all read accesses and provide get_vm_flags() too? That will > > cause some additional churt (a quick search shows 801 hits over 248 > > files) but maybe it's worth it? I think Michal suggested that too in > > another patch. Should I do that while we are at it? > > Here's a trick I saw somewhere in the VFS: > > union { > const vm_flags_t vm_flags; > vm_flags_t __private __vm_flags; > }; > > Now it can be read by anybody but written only by those using > ACCESS_PRIVATE. Huh, this is quite nice! I think it does not save us from the cases when vma->vm_flags is passed by a reference and modified indirectly, like in ksm_madvise()? Though maybe such usecases are so rare (I found only 2 cases) that we can ignore this?
Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Wed, Jan 25, 2023 at 10:33 AM Matthew Wilcox wrote: > > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > > +static inline void init_vm_flags(struct vm_area_struct *vma, > > + unsigned long flags) > > +{ > > + vma->vm_flags = flags; > > vm_flags are supposed to have type vm_flags_t. That's not been > fully realised yet, but perhaps we could avoid making it worse? > > > pgprot_t vm_page_prot; > > - unsigned long vm_flags; /* Flags, see mm.h. */ > > + > > + /* > > + * Flags, see mm.h. > > + * WARNING! Do not modify directly. > > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > > + */ > > + unsigned long vm_flags; > > Including changing this line to vm_flags_t Good point. Will make the change. Thanks!
Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Wed, Jan 25, 2023 at 1:10 AM Peter Zijlstra wrote: > > On Wed, Jan 25, 2023 at 12:38:46AM -0800, Suren Baghdasaryan wrote: > > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > > index 2d6d790d9bed..6c7c70bf50dd 100644 > > --- a/include/linux/mm_types.h > > +++ b/include/linux/mm_types.h > > @@ -491,7 +491,13 @@ struct vm_area_struct { > >* See vmf_insert_mixed_prot() for discussion. > >*/ > > pgprot_t vm_page_prot; > > - unsigned long vm_flags; /* Flags, see mm.h. */ > > + > > + /* > > + * Flags, see mm.h. > > + * WARNING! Do not modify directly. > > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > > + */ > > + unsigned long vm_flags; > > We have __private and ACCESS_PRIVATE() to help with enforcing this. Thanks for pointing this out, Peter! I guess for that I'll need to convert all read accesses and provide get_vm_flags() too? That will cause some additional churt (a quick search shows 801 hits over 248 files) but maybe it's worth it? I think Michal suggested that too in another patch. Should I do that while we are at it? >
Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Wed, Jan 25, 2023 at 08:49:50AM -0800, Suren Baghdasaryan wrote: > On Wed, Jan 25, 2023 at 1:10 AM Peter Zijlstra wrote: > > > + /* > > > + * Flags, see mm.h. > > > + * WARNING! Do not modify directly. > > > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > > > + */ > > > + unsigned long vm_flags; > > > > We have __private and ACCESS_PRIVATE() to help with enforcing this. > > Thanks for pointing this out, Peter! I guess for that I'll need to > convert all read accesses and provide get_vm_flags() too? That will > cause some additional churt (a quick search shows 801 hits over 248 > files) but maybe it's worth it? I think Michal suggested that too in > another patch. Should I do that while we are at it? Here's a trick I saw somewhere in the VFS: union { const vm_flags_t vm_flags; vm_flags_t __private __vm_flags; }; Now it can be read by anybody but written only by those using ACCESS_PRIVATE.
Re: [PATCH v2 1/6] mm: introduce vma->vm_flags modifier functions
On Wed 25-01-23 00:38:46, Suren Baghdasaryan wrote: > vm_flags are among VMA attributes which affect decisions like VMA merging > and splitting. Therefore all vm_flags modifications are performed after > taking exclusive mmap_lock to prevent vm_flags updates racing with such > operations. Introduce modifier functions for vm_flags to be used whenever > flags are updated. This way we can better check and control correct > locking behavior during these updates. > > Signed-off-by: Suren Baghdasaryan Acked-by: Michal Hocko > --- > include/linux/mm.h | 37 + > include/linux/mm_types.h | 8 +++- > 2 files changed, 44 insertions(+), 1 deletion(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index c2f62bdce134..b71f2809caac 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -627,6 +627,43 @@ static inline void vma_init(struct vm_area_struct *vma, > struct mm_struct *mm) > INIT_LIST_HEAD(&vma->anon_vma_chain); > } > > +/* Use when VMA is not part of the VMA tree and needs no locking */ > +static inline void init_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + vma->vm_flags = flags; > +} > + > +/* Use when VMA is part of the VMA tree and modifications need coordination > */ > +static inline void reset_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + init_vm_flags(vma, flags); > +} > + > +static inline void set_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + vma->vm_flags |= flags; > +} > + > +static inline void clear_vm_flags(struct vm_area_struct *vma, > + unsigned long flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + vma->vm_flags &= ~flags; > +} > + > +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; > +} > + > static inline void vma_set_anonymous(struct vm_area_struct *vma) > { > vma->vm_ops = NULL; > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h > index 2d6d790d9bed..6c7c70bf50dd 100644 > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -491,7 +491,13 @@ struct vm_area_struct { >* See vmf_insert_mixed_prot() for discussion. >*/ > pgprot_t vm_page_prot; > - unsigned long vm_flags; /* Flags, see mm.h. */ > + > + /* > + * Flags, see mm.h. > + * WARNING! Do not modify directly. > + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. > + */ > + unsigned long vm_flags; > > /* >* For areas with an address space and backing store, > -- > 2.39.1 -- Michal Hocko SUSE Labs