Re: [RFC PATCH v1 1/1] linux: mm_types: allow to modified the vm_flags in vm_area_struct

[Suren Baghdasaryan]

On Mon, Mar 13, 2023 at 2:25 AM Vincenzo Palazzo
<vincenzopalazzodev@xxxxxxxxx> wrote:
>
> On Mon Mar 13, 2023 at 1:57 AM CET, Andrew Morton wrote:
> > On Sun, 12 Mar 2023 23:42:50 +0100 Vincenzo Palazzo <vincenzopalazzodev@xxxxxxxxx> wrote:
> >
> > > With 6.3-rc1 is not possible to build the following drivers
> >
> > Well, let's cc Suren who actually wrote bc292ab00f6c ("mm: introduce
> > vma->vm_flags wrapper functions").
> >
> > > - nvidia, that the compilation returns the following errors
> > >
> > > - vboxhost/7.0.6_OSE where the build fails with the following errors
> > >
> > > ...
> > >
> > > --- a/include/linux/mm_types.h
> > > +++ b/include/linux/mm_types.h
> > > @@ -497,7 +497,7 @@ struct vm_area_struct {
> > >      * To modify use vm_flags_{init|reset|set|clear|mod} functions.
> > >      */
> > >     union {
> > > -           const vm_flags_t vm_flags;
> > > +           vm_flags_t vm_flags;
> > >             vm_flags_t __private __vm_flags;
> > >     };
> >
> > These out-of-tree drivers should be converted to use the wrappers which
> > bc292ab00f6c added.  Their maintainers have a couple of months to do
> > this before bc292ab00f6c is released in 6.3.
>
> Yeah I thought about that! nvidia update as soon as the 6.3 is out, not sure
> about vboxhost. But let the driver update to this new feature make sense.
>
> >
> > It's unfortunate.  I guess it would be better if we were to find a way
> > to permit these drivers to compile OK but to generate warnings, as an
> > interim thing for six months or so.  Perhaps there's some clever way.
>
> Yeah it is! Regarding giving the possiblity to compile ok (and
> preserving the previous behavior) look like an interesting thought for
> kernel API, but am I think that it this is difficult to do?
>
> The real things that I'm missing with the new wrapper API
> is the meaning of the new `vm_flags_t vm_flags`, before the
> field was only one and it was getting modified, but now the real
> value is the private one `__vm_flags`, so what is the meaning of
> the new `vm_flags`?

Sorry for the delay. I'm on vacation until Wednesday and have limited
network access.
vm_flags is used as a read-only property so that readers of the flags
don't have to change, however flag modifiers should be using new
wrapper functions. Fixing the errors in this report should be trivial.
For example:
vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND;
should be replaced with
vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND);
and
vma->vm_flags &= ~VM_MAYWRITE;
with
vm_flags_clear(vma, VM_MAYWRITE);

Thanks,
Suren.

>
> I feel that this question is stupid but I can not avoid do it, otherwise
> my mind think about it till I will not find an answer :)
>
> P.S: regaring the warning at compile time the __deprecated looks a good
> fit to generate error message, but to me in this particular case do not.
>
> Thanks,
>
> Vincent.
>