Re: [RFC] mm/gup.c: Use gup_flags as parameter instead of passing write flag

[John Hubbard]

On 2020-05-18 13:44, Souptick Joarder wrote:
> On Tue 19 May, 2020, 1:47 AM Matthew Wilcox, <willy@xxxxxxxxxxxxx> wrote:
> > On Tue, May 19, 2020 at 01:28:23AM +0530, Souptick Joarder wrote:
> > > The idea is to get rid of write parameter. Instead caller will pass
> > > FOLL_WRITE to __get_user_pages_fast(). This will not change any
> > > functionality of the API. Once it is upstream all the callers will
> > > be changed to pass FOLL_WRITE.
> >
> > Uhh ... until you change all the callers, haven't you just broken all
> > the callers?
>
> All the callers have called the API with either 1 or 0.  I think, it's
> not going to break
> any of the callers.

Maybe so, but it's still "wrong" to do that, because it only works more
or less accidentally. That is, it works in spite of a type safety
violation. So we don't want to do that sort of thing unless there is
a compelling reason.

In addition to that, I am at the exact moment putting together a minor
refactoring of this function, because I need a FOLL_PIN variant:
__pin_user_pages_fast(), as part of my work to change over a few dozen
gup call sites to pin_user_pages*().

(In fact, I was wondering whether to stick with the "write" parameter, for
the new __pin_user_pages_fast(), or go with gup_flags. That could go either
way: gup_flags provides a nicer API, but "write" matches the existing
callers.)

So in other words, if you do go out and change all the call sites (there only
seem to be about 7, outside of gup.c, actually), that's going to conflict
a little bit with what I'm doing here.

So, how would you like proceed? If you want to do the full conversion
(which really should include the call sites), it would be easier for me
if you based it on my upcoming small patchset, which I expect to post
shortly (later today).

thanks,
--
John Hubbard
NVIDIA

> > > -int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> > > -                       struct page **pages)
> > > +int __get_user_pages_fast(unsigned long start, int nr_pages,
> > > +                     unsigned int gup_flags, struct page **pages)
> > >   {
> > >        unsigned long len, end;
> > >        unsigned long flags;
> > > @@ -2685,10 +2692,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> > >         * Internally (within mm/gup.c), gup fast variants must set FOLL_GET,
> > >         * because gup fast is always a "pin with a +1 page refcount" request.
> > >         */
> > > -     unsigned int gup_flags = FOLL_GET;
> > > -
> > > -     if (write)
> > > -             gup_flags |= FOLL_WRITE;
> > > +     gup_flags |= FOLL_GET;