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

Pingfan Liu
2020-03-20T09:20:00Z

On Fri, Mar 20, 2020 at 6:17 AM John Hubbard <jhubbard@nvidia.com> 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