| Message ID | 20230109205336.3665937-14-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Per-VMA locks | expand |
On Mon, 09 Jan 2023, Suren Baghdasaryan wrote: >To keep vma locking correctness when vm_flags are modified, add modifier >functions to be used whenever flags are updated. How about moving this patch and the ones that follow out of this series, into a preliminary patchset? It would reduce the amount of noise in the per-vma lock changes, which would then only be adding the needed vma_write_lock()ing. Thanks, Davidlohr
On Wed, Jan 11, 2023 at 8:13 AM Davidlohr Bueso <dave@stgolabs.net> wrote: > > On Mon, 09 Jan 2023, Suren Baghdasaryan wrote: > > >To keep vma locking correctness when vm_flags are modified, add modifier > >functions to be used whenever flags are updated. > > How about moving this patch and the ones that follow out of this series, > into a preliminary patchset? It would reduce the amount of noise in the > per-vma lock changes, which would then only be adding the needed > vma_write_lock()ing. How about moving those prerequisite patches to the beginning of the patchset (before maple_tree RCU changes)? I feel like they do belong in the patchset because as a standalone patchset it would be unclear why I'm adding all these accessor functions and introducing this churn. Would that be acceptable? > > Thanks, > Davidlohr > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
On Wed, 11 Jan 2023, Suren Baghdasaryan wrote: >On Wed, Jan 11, 2023 at 8:13 AM Davidlohr Bueso <dave@stgolabs.net> wrote: >> >> On Mon, 09 Jan 2023, Suren Baghdasaryan wrote: >> >> >To keep vma locking correctness when vm_flags are modified, add modifier >> >functions to be used whenever flags are updated. >> >> How about moving this patch and the ones that follow out of this series, >> into a preliminary patchset? It would reduce the amount of noise in the >> per-vma lock changes, which would then only be adding the needed >> vma_write_lock()ing. > >How about moving those prerequisite patches to the beginning of the >patchset (before maple_tree RCU changes)? I feel like they do belong >in the patchset because as a standalone patchset it would be unclear >why I'm adding all these accessor functions and introducing this >churn. Would that be acceptable? imo the abstraction of vm_flags handling is worth being standalone and is easier to be picked up before a more complex locking scheme change. But either way, it's up to you. Thanks, Davidlohr
On Wed, Jan 11, 2023 at 12:19 PM Davidlohr Bueso <dave@stgolabs.net> wrote: > > On Wed, 11 Jan 2023, Suren Baghdasaryan wrote: > > >On Wed, Jan 11, 2023 at 8:13 AM Davidlohr Bueso <dave@stgolabs.net> wrote: > >> > >> On Mon, 09 Jan 2023, Suren Baghdasaryan wrote: > >> > >> >To keep vma locking correctness when vm_flags are modified, add modifier > >> >functions to be used whenever flags are updated. > >> > >> How about moving this patch and the ones that follow out of this series, > >> into a preliminary patchset? It would reduce the amount of noise in the > >> per-vma lock changes, which would then only be adding the needed > >> vma_write_lock()ing. > > > >How about moving those prerequisite patches to the beginning of the > >patchset (before maple_tree RCU changes)? I feel like they do belong > >in the patchset because as a standalone patchset it would be unclear > >why I'm adding all these accessor functions and introducing this > >churn. Would that be acceptable? > > imo the abstraction of vm_flags handling is worth being standalone and is > easier to be picked up before a more complex locking scheme change. But > either way, it's up to you. I see your point. Ok, if you think it makes sense as a stand-alone patch I can post it separately in the next version. Thanks, Suren. > > Thanks, > Davidlohr > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
On Mon 09-01-23 12:53:08, Suren Baghdasaryan wrote: > To keep vma locking correctness when vm_flags are modified, add modifier > functions to be used whenever flags are updated. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > include/linux/mm.h | 38 ++++++++++++++++++++++++++++++++++++++ > include/linux/mm_types.h | 8 +++++++- > 2 files changed, 45 insertions(+), 1 deletion(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index ec2c4c227d51..35cf0a6cbcc2 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -702,6 +702,44 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm) > vma_init_lock(vma); > } > > +/* 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) > +{ > + WRITE_ONCE(vma->vm_flags, flags); > +} Why do we need WRITE_ONCE here? Isn't vma invisible during its initialization? > + > +/* Use when VMA is part of the VMA tree and needs appropriate locking */ > +static inline > +void reset_vm_flags(struct vm_area_struct *vma, unsigned long flags) > +{ > + vma_write_lock(vma); > + init_vm_flags(vma, flags); > +} > + > +static inline > +void set_vm_flags(struct vm_area_struct *vma, unsigned long flags) > +{ > + vma_write_lock(vma); > + vma->vm_flags |= flags; > +} > + > +static inline > +void clear_vm_flags(struct vm_area_struct *vma, unsigned long flags) > +{ > + vma_write_lock(vma); > + vma->vm_flags &= ~flags; > +} > + > +static inline > +void mod_vm_flags(struct vm_area_struct *vma, > + unsigned long set, unsigned long clear) > +{ > + vma_write_lock(vma); > + vma->vm_flags |= set; > + vma->vm_flags &= ~clear; > +} > + This is rather unusual pattern. There is no note about locking involved in the naming and also why is the locking part of this interface in the first place? I can see reason for access functions to actually check for lock asserts.
On Tue 17-01-23 16:09:03, Michal Hocko wrote: > On Mon 09-01-23 12:53:08, Suren Baghdasaryan wrote: > > To keep vma locking correctness when vm_flags are modified, add modifier > > functions to be used whenever flags are updated. > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > include/linux/mm.h | 38 ++++++++++++++++++++++++++++++++++++++ > > include/linux/mm_types.h | 8 +++++++- > > 2 files changed, 45 insertions(+), 1 deletion(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index ec2c4c227d51..35cf0a6cbcc2 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -702,6 +702,44 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm) > > vma_init_lock(vma); > > } > > > > +/* 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) > > +{ > > + WRITE_ONCE(vma->vm_flags, flags); > > +} > > Why do we need WRITE_ONCE here? Isn't vma invisible during its > initialization? > > > + > > +/* Use when VMA is part of the VMA tree and needs appropriate locking */ > > +static inline > > +void reset_vm_flags(struct vm_area_struct *vma, unsigned long flags) > > +{ > > + vma_write_lock(vma); > > + init_vm_flags(vma, flags); > > +} > > + > > +static inline > > +void set_vm_flags(struct vm_area_struct *vma, unsigned long flags) > > +{ > > + vma_write_lock(vma); > > + vma->vm_flags |= flags; > > +} > > + > > +static inline > > +void clear_vm_flags(struct vm_area_struct *vma, unsigned long flags) > > +{ > > + vma_write_lock(vma); > > + vma->vm_flags &= ~flags; > > +} > > + > > +static inline > > +void mod_vm_flags(struct vm_area_struct *vma, > > + unsigned long set, unsigned long clear) > > +{ > > + vma_write_lock(vma); > > + vma->vm_flags |= set; > > + vma->vm_flags &= ~clear; > > +} > > + > > This is rather unusual pattern. There is no note about locking involved > in the naming and also why is the locking part of this interface in the > first place? I can see reason for access functions to actually check for > lock asserts. OK, it took me a while but it is clear to me now. The confusion comes from the naming vma_write_lock is no a lock in its usual terms. It is more of a vma_mark_modified with side effects to read locking which is a real lock. With that it makes more sense to have this done in these helpers rather than requiring all users to keep this subtletly in mind.
On Tue, Jan 17, 2023 at 7:15 AM 'Michal Hocko' via kernel-team <kernel-team@android.com> wrote: > > On Tue 17-01-23 16:09:03, Michal Hocko wrote: > > On Mon 09-01-23 12:53:08, Suren Baghdasaryan wrote: > > > To keep vma locking correctness when vm_flags are modified, add modifier > > > functions to be used whenever flags are updated. > > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > > --- > > > include/linux/mm.h | 38 ++++++++++++++++++++++++++++++++++++++ > > > include/linux/mm_types.h | 8 +++++++- > > > 2 files changed, 45 insertions(+), 1 deletion(-) > > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > index ec2c4c227d51..35cf0a6cbcc2 100644 > > > --- a/include/linux/mm.h > > > +++ b/include/linux/mm.h > > > @@ -702,6 +702,44 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm) > > > vma_init_lock(vma); > > > } > > > > > > +/* 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) > > > +{ > > > + WRITE_ONCE(vma->vm_flags, flags); > > > +} > > > > Why do we need WRITE_ONCE here? Isn't vma invisible during its > > initialization? Ack. Will change to a simple assignment. > > > > > + > > > +/* Use when VMA is part of the VMA tree and needs appropriate locking */ > > > +static inline > > > +void reset_vm_flags(struct vm_area_struct *vma, unsigned long flags) > > > +{ > > > + vma_write_lock(vma); > > > + init_vm_flags(vma, flags); > > > +} > > > + > > > +static inline > > > +void set_vm_flags(struct vm_area_struct *vma, unsigned long flags) > > > +{ > > > + vma_write_lock(vma); > > > + vma->vm_flags |= flags; > > > +} > > > + > > > +static inline > > > +void clear_vm_flags(struct vm_area_struct *vma, unsigned long flags) > > > +{ > > > + vma_write_lock(vma); > > > + vma->vm_flags &= ~flags; > > > +} > > > + > > > +static inline > > > +void mod_vm_flags(struct vm_area_struct *vma, > > > + unsigned long set, unsigned long clear) > > > +{ > > > + vma_write_lock(vma); > > > + vma->vm_flags |= set; > > > + vma->vm_flags &= ~clear; > > > +} > > > + > > > > This is rather unusual pattern. There is no note about locking involved > > in the naming and also why is the locking part of this interface in the > > first place? I can see reason for access functions to actually check for > > lock asserts. > > OK, it took me a while but it is clear to me now. The confusion comes > from the naming vma_write_lock is no a lock in its usual terms. It is > more of a vma_mark_modified with side effects to read locking which is a > real lock. With that it makes more sense to have this done in these > helpers rather than requiring all users to keep this subtletly in mind. If renaming vma-locking primitives the way Matthew suggested in https://lore.kernel.org/all/Y8cZMt01Z1FvVFXh@casper.infradead.org/ makes it easier to read/understand, I'm all for it. Let's discuss the naming in that email thread because that's where these functions are introduced. > > -- > Michal Hocko > SUSE Labs > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
diff --git a/include/linux/mm.h b/include/linux/mm.h index ec2c4c227d51..35cf0a6cbcc2 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -702,6 +702,44 @@ static inline void vma_init(struct vm_area_struct *vma, struct mm_struct *mm) vma_init_lock(vma); } +/* 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) +{ + WRITE_ONCE(vma->vm_flags, flags); +} + +/* Use when VMA is part of the VMA tree and needs appropriate locking */ +static inline +void reset_vm_flags(struct vm_area_struct *vma, unsigned long flags) +{ + vma_write_lock(vma); + init_vm_flags(vma, flags); +} + +static inline +void set_vm_flags(struct vm_area_struct *vma, unsigned long flags) +{ + vma_write_lock(vma); + vma->vm_flags |= flags; +} + +static inline +void clear_vm_flags(struct vm_area_struct *vma, unsigned long flags) +{ + vma_write_lock(vma); + vma->vm_flags &= ~flags; +} + +static inline +void mod_vm_flags(struct vm_area_struct *vma, + unsigned long set, unsigned long clear) +{ + vma_write_lock(vma); + 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 5f7c5ca89931..0d27edd3e63a 100644 --- a/include/linux/mm_types.h +++ b/include/linux/mm_types.h @@ -553,7 +553,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 to keep correct VMA locking. + * Use {init|reset|set|clear|mod}_vm_flags() functions instead. + */ + unsigned long vm_flags; #ifdef CONFIG_PER_VMA_LOCK int vm_lock_seq;
To keep vma locking correctness when vm_flags are modified, add modifier functions to be used whenever flags are updated. Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- include/linux/mm.h | 38 ++++++++++++++++++++++++++++++++++++++ include/linux/mm_types.h | 8 +++++++- 2 files changed, 45 insertions(+), 1 deletion(-)