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 <[email protected]>
---
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);
+}
+
+/* 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;
--
2.39.0
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 <[email protected]> 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 [email protected].
>
On Wed, 11 Jan 2023, Suren Baghdasaryan wrote:
>On Wed, Jan 11, 2023 at 8:13 AM Davidlohr Bueso <[email protected]> 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 <[email protected]> wrote:
>
> On Wed, 11 Jan 2023, Suren Baghdasaryan wrote:
>
> >On Wed, Jan 11, 2023 at 8:13 AM Davidlohr Bueso <[email protected]> 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 [email protected].
>
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 <[email protected]>
> ---
> 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.
--
Michal Hocko
SUSE Labs
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 <[email protected]>
> > ---
> > 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.
--
Michal Hocko
SUSE Labs
On Tue, Jan 17, 2023 at 7:15 AM 'Michal Hocko' via kernel-team
<[email protected]> 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 <[email protected]>
> > > ---
> > > 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/[email protected]/
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 [email protected].
>