| Message ID | 20230125233554.153109-3-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | introduce vm_flags modifier functions | expand |
On Wed, 25 Jan 2023 15:35:49 -0800 Suren Baghdasaryan <surenb@google.com> 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. > > ... > > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > +static inline void init_vm_flags(struct vm_area_struct *vma, > +static inline void reset_vm_flags(struct vm_area_struct *vma, > +static inline void set_vm_flags(struct vm_area_struct *vma, > +static inline void clear_vm_flags(struct vm_area_struct *vma, > +static inline void mod_vm_flags(struct vm_area_struct *vma, vm_flags_init(), vm_flags_reset(), etc? This would be more idiomatic and I do think the most-significant-first naming style is preferable.
On Wed, 25 Jan 2023 15:35:49 -0800 Suren Baghdasaryan <surenb@google.com> wrote: > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -491,7 +491,15 @@ 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. > + * To modify use {init|reset|set|clear|mod}_vm_flags() functions. > + */ > + union { > + const vm_flags_t vm_flags; > + vm_flags_t __private __vm_flags; > + }; Typically when making a change like this we'll rename the affected field/variable/function/etc. This will reliably and deliberately break unconverted usage sites. This const trick will get us partway there, by breaking setters. But renaming it will break both setters and getters.
On Wed, Jan 25, 2023 at 4:24 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Wed, 25 Jan 2023 15:35:49 -0800 Suren Baghdasaryan <surenb@google.com> 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. > > > > ... > > > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > +static inline void init_vm_flags(struct vm_area_struct *vma, > > +static inline void reset_vm_flags(struct vm_area_struct *vma, > > +static inline void set_vm_flags(struct vm_area_struct *vma, > > +static inline void clear_vm_flags(struct vm_area_struct *vma, > > +static inline void mod_vm_flags(struct vm_area_struct *vma, > > vm_flags_init(), vm_flags_reset(), etc? > > This would be more idiomatic and I do think the most-significant-first > naming style is preferable. Thanks for the suggestion! I will rename them in the next version. > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
On Wed, Jan 25, 2023 at 4:28 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Wed, 25 Jan 2023 15:35:49 -0800 Suren Baghdasaryan <surenb@google.com> wrote: > > > --- a/include/linux/mm_types.h > > +++ b/include/linux/mm_types.h > > @@ -491,7 +491,15 @@ 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. > > + * To modify use {init|reset|set|clear|mod}_vm_flags() functions. > > + */ > > + union { > > + const vm_flags_t vm_flags; > > + vm_flags_t __private __vm_flags; > > + }; > > Typically when making a change like this we'll rename the affected > field/variable/function/etc. This will reliably and deliberately break > unconverted usage sites. > > This const trick will get us partway there, by breaking setters. But > renaming it will break both setters and getters. My intent here is to break setters but to allow getters to keep reading vma->vm_flags directly. We could provide get_vm_flags() and convert all getters as well but it would introduce a huge additional churn (800+ hits) with no obvious benefits, I think. Does that clarify the intent of this trick? > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
On Wed 25-01-23 16:56:17, Suren Baghdasaryan wrote: > On Wed, Jan 25, 2023 at 4:28 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > On Wed, 25 Jan 2023 15:35:49 -0800 Suren Baghdasaryan <surenb@google.com> wrote: > > > > > --- a/include/linux/mm_types.h > > > +++ b/include/linux/mm_types.h > > > @@ -491,7 +491,15 @@ 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. > > > + * To modify use {init|reset|set|clear|mod}_vm_flags() functions. > > > + */ > > > + union { > > > + const vm_flags_t vm_flags; > > > + vm_flags_t __private __vm_flags; > > > + }; > > > > Typically when making a change like this we'll rename the affected > > field/variable/function/etc. This will reliably and deliberately break > > unconverted usage sites. > > > > This const trick will get us partway there, by breaking setters. But > > renaming it will break both setters and getters. > > My intent here is to break setters but to allow getters to keep > reading vma->vm_flags directly. We could provide get_vm_flags() and > convert all getters as well but it would introduce a huge additional > churn (800+ hits) with no obvious benefits, I think. Does that clarify > the intent of this trick? I think that makes sense at this stage. The conversion patch is quite large already. Maybe the final renaming could be done on top of everything and patch generated by coccinele. The const union is a neat trick but a potential lockdep assert is a nice plus as well. I wouldn't see it as a top priority though.
On Wed 25-01-23 15:35:49, 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 <surenb@google.com> with or without the proposed renaming (it seems we are not consistent on that much even in the core kernel - e.g. init_rwsem vs. mutex_init) Acked-by: Michal Hocko <mhocko@suse.com> > --- > include/linux/mm.h | 37 +++++++++++++++++++++++++++++++++++++ > include/linux/mm_types.h | 10 +++++++++- > 2 files changed, 46 insertions(+), 1 deletion(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index c2f62bdce134..bf16ddd544a5 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, > + vm_flags_t flags) > +{ > + ACCESS_PRIVATE(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, > + vm_flags_t flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + init_vm_flags(vma, flags); > +} > + > +static inline void set_vm_flags(struct vm_area_struct *vma, > + vm_flags_t flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + ACCESS_PRIVATE(vma, __vm_flags) |= flags; > +} > + > +static inline void clear_vm_flags(struct vm_area_struct *vma, > + vm_flags_t flags) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + ACCESS_PRIVATE(vma, __vm_flags) &= ~flags; > +} > + > +static inline void mod_vm_flags(struct vm_area_struct *vma, > + vm_flags_t set, vm_flags_t clear) > +{ > + mmap_assert_write_locked(vma->vm_mm); > + ACCESS_PRIVATE(vma, __vm_flags) |= set; > + ACCESS_PRIVATE(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..bccbd5896850 100644 > --- a/include/linux/mm_types.h > +++ b/include/linux/mm_types.h > @@ -491,7 +491,15 @@ 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. > + * To modify use {init|reset|set|clear|mod}_vm_flags() functions. > + */ > + union { > + const vm_flags_t vm_flags; > + vm_flags_t __private __vm_flags; > + }; > > /* > * For areas with an address space and backing store, > -- > 2.39.1
On Wed, Jan 25, 2023 at 03:35:49PM -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 <surenb@google.com> With or without the suggested rename; Acked-by: Mel Gorman <mgorman@techsingularity.net>
On Thu, Jan 26, 2023 at 5:58 AM Mel Gorman <mgorman@techsingularity.net> wrote: > > On Wed, Jan 25, 2023 at 03:35:49PM -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 <surenb@google.com> > > With or without the suggested rename; > > Acked-by: Mel Gorman <mgorman@techsingularity.net> Thanks! I'll make the renames and repost the patchset. vm_flags_init(), vm_flags_reset(), etc. sounds like a good naming for this. > > -- > Mel Gorman > SUSE Labs
On Wed, 25 Jan 2023, Andrew Morton wrote: >On Wed, 25 Jan 2023 15:35:49 -0800 Suren Baghdasaryan <surenb@google.com> 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. >> >> ... >> >> --- a/include/linux/mm.h >> +++ b/include/linux/mm.h >> +static inline void init_vm_flags(struct vm_area_struct *vma, >> +static inline void reset_vm_flags(struct vm_area_struct *vma, >> +static inline void set_vm_flags(struct vm_area_struct *vma, >> +static inline void clear_vm_flags(struct vm_area_struct *vma, >> +static inline void mod_vm_flags(struct vm_area_struct *vma, > >vm_flags_init(), vm_flags_reset(), etc? > >This would be more idiomatic and I do think the most-significant-first >naming style is preferable. I tend to prefer this naming yes, but lgtm regardless. Reviewed-by: Davidlohr Bueso <dave@stgolabs.net>
diff --git a/include/linux/mm.h b/include/linux/mm.h index c2f62bdce134..bf16ddd544a5 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, + vm_flags_t flags) +{ + ACCESS_PRIVATE(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, + vm_flags_t flags) +{ + mmap_assert_write_locked(vma->vm_mm); + init_vm_flags(vma, flags); +} + +static inline void set_vm_flags(struct vm_area_struct *vma, + vm_flags_t flags) +{ + mmap_assert_write_locked(vma->vm_mm); + ACCESS_PRIVATE(vma, __vm_flags) |= flags; +} + +static inline void clear_vm_flags(struct vm_area_struct *vma, + vm_flags_t flags) +{ + mmap_assert_write_locked(vma->vm_mm); + ACCESS_PRIVATE(vma, __vm_flags) &= ~flags; +} + +static inline void mod_vm_flags(struct vm_area_struct *vma, + vm_flags_t set, vm_flags_t clear) +{ + mmap_assert_write_locked(vma->vm_mm); + ACCESS_PRIVATE(vma, __vm_flags) |= set; + ACCESS_PRIVATE(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..bccbd5896850 100644 --- a/include/linux/mm_types.h +++ b/include/linux/mm_types.h @@ -491,7 +491,15 @@ 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. + * To modify use {init|reset|set|clear|mod}_vm_flags() functions. + */ + union { + const vm_flags_t vm_flags; + vm_flags_t __private __vm_flags; + }; /* * For areas with an address space and backing store,
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 <surenb@google.com> --- include/linux/mm.h | 37 +++++++++++++++++++++++++++++++++++++ include/linux/mm_types.h | 10 +++++++++- 2 files changed, 46 insertions(+), 1 deletion(-)