On 1/31/22 23:43, John Hubbard wrote:
pin_user_pages
__get_user_pages_locked
follow_page_mask
follow_page_pte
try_grab_page
!is_pinnable_page(page)
return NULL;
return ERR_PTR(-ENOMEM);
return -ENOMEM without faultin_page
Yes, that's all clear.
...oh, but I guess you're pointing out that it's always going to be
page-at-a-time this deep in the pin_user_pages() call path. Which is true.
I hadn't worked through how to fix this yet, my initial reaction was
that allowing single refs to go through, while prohibiting multiple refs,
was clearly *not* the way to go.
OK, so after looking at this some more, I think that the real problem
with commit 54d516b1d62f ("mm/gup: small refactoring: simplify
try_grab_page()") is that it funnels fast and slow gup through the same
routine (try_grab_compound_head()). And the problem with *that*, is that
try_grab_compound_head() is coded up with that assumption that it is
being called *only* by fast-gup:
/*
* Can't do FOLL_LONGTERM + FOLL_PIN gup fast path if not in a
* right zone, so fail and let the caller fall back to the slow
* path.
*/
if (unlikely((flags & FOLL_LONGTERM) &&
!is_pinnable_page(page)))
return NULL;
Now, to fix that, I'd really rather not conflate "refs == 1" with "on
the slow path", because that's a conceptual mismatch.
So, other ideas:
a) Remove the above check, and fail fast gup at a different point for
the (FOLL_LONGTERM && !is_pinnable) case. Haven't looked closely at this
yet.
b) Pass in FOLL_FAST_ONLY from all call sites *except* try_grag_page().
Skip the above check in slow-gup (!FOLL_FAST_ONLY) cases.
This makes the code ugly, though, and I'm just listing it here for
completeness.
c) Just call the entire refactoring idea a mistake, and roll it back
either entirely, or enough to keep fast and slow gup separate.
Unless a better idea shows up, (c) is probably the way to go, I think...
thanks,
--
John Hubbard
NVIDIA
