Re: [PATCH v3 2/7] mm: introduce vma->vm_flags wrapper functions

[Michal Hocko <mhocko@suse.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.
-- 
Michal Hocko
SUSE Labs