Re: [PATCHv7 2/3] mm/gup: fix omission of check on FOLL_LONGTERM in gup fast path

[Pingfan Liu]

On Fri, Mar 20, 2020 at 6:17 AM John Hubbard <jhubbard@xxxxxxxxxx> wrote:
>
> On 3/17/20 4:47 AM, Pingfan Liu wrote:
> > FOLL_LONGTERM is a special case of FOLL_PIN. It suggests a pin which is
> > going to be given to hardware and can't move. It would truncate CMA
> > permanently and should be excluded.
> >
> > In gup slow path, slow path, where
>
>
> s/slow path, slow path/slow path/
Yeah.
[...]
> >
> >               /*
> > +              * Huge page's subpages have the same migrate type due to either
> > +              * allocation from a free_list[] or alloc_contig_range() with
> > +              * param MIGRATE_MOVABLE. So it is enough to check on a subpage.
> > +              */
>
> Urggh, this comment is fine in the commit description, but at this location in the
> code it is completely incomprehensible! Instead of an extremely far-removed tidbit about
> interactions between CMA and huge pages, this comment should be explaining why we bail
> out early in the specific case of FOLL_PIN + FOLL_LONGTERM. And we don't bail out for
> FOLL_GET + FOLL_LONGTERM...
>
>
> I'm expect it is something like:
>
>                 /*
>                  * We can't do FOLL_LONGTERM + FOLL_PIN with CMA in the gup fast
>                  * path, so fail and let the caller fall back to the slow path.
>                  */
>
>
> ...approximately. Right?
Yes, right. And I think it is better to drop "We".

Thanks,
Pingfan