On Thu, Jun 27, 2024 at 04:32:42PM -0700, Andrew Morton wrote:
> On Thu, 27 Jun 2024 19:19:40 -0400 Peter Xu <peterx@xxxxxxxxxx> wrote:
>
> > Yang,
> >
> > On Thu, Jun 27, 2024 at 03:14:13PM -0700, Yang Shi wrote:
> > > The try_grab_folio() is supposed to be used in fast path and it elevates
> > > folio refcount by using add ref unless zero. We are guaranteed to have
> > > at least one stable reference in slow path, so the simple atomic add
> > > could be used. The performance difference should be trivial, but the
> > > misuse may be confusing and misleading.
> >
> > This first paragraph is IMHO misleading itself..
> >
> > I think we should mention upfront the important bit, on the user impact.
> >
> > Here IMO the user impact should be: Linux may fail longterm pin in some
> > releavnt paths when applied over CMA reserved blocks. And if to extend a
> > bit, that include not only slow-gup but also the new memfd pinning, because
> > both of them used try_grab_folio() which used to be only for fast-gup.
>
> It's still unclear how users will be affected. What do the *users*
> see? If it's a slight slowdown, do we need to backport this at all?
The user will see the pin fails, for gpu-slow it further triggers the WARN
right below that failure (as in the original report):
folio = try_grab_folio(page, page_increm - 1,
foll_flags);
if (WARN_ON_ONCE(!folio)) { <------------------------ here
/*
* Release the 1st page ref if the
* folio is problematic, fail hard.
*/
gup_put_folio(page_folio(page), 1,
foll_flags);
ret = -EFAULT;
goto out;
}
For memfd pin and hugepd paths, they should just observe GUP failure on
those longterm pins, and it'll be the caller context to decide what user
can see, I think.
>
> >
> > The patch itself looks mostly ok to me.
> >
> > There's still some "cleanup" part mangled together, e.g., the real meat
> > should be avoiding the folio_is_longterm_pinnable() check in relevant
> > paths. The rest (e.g. switch slow-gup / memfd pin to use folio_ref_add()
> > not try_get_folio(), and renames) could be good cleanups.
> >
> > So a smaller fix might be doable, but again I don't have a strong opinion
> > here.
>
> The smaller the better for backporting, of course.
I think a smaller version might be yangge's patch, plus Yang's hugepd
"fast" parameter for the hugepd stack, then hugepd can also use
try_grab_page(). memfd-pin change can be a separate small patch perhaps
squashed.
I'll leave how to move on to Yang.
Thanks,
--
Peter Xu
