Re: [PATCH v1 1/1] mm/gup: skip pinnable check for refs==1
From: John Hubbard
Date: Tue Feb 01 2022 - 02:43:37 EST
On 1/31/22 23:35, John Hubbard wrote:
On 1/31/22 23:28, Minchan Kim wrote:
...
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -135,7 +135,7 @@ struct page *try_grab_compound_head(struct page *page,
* right zone, so fail and let the caller fall back to the slow
* path.
*/
- if (unlikely((flags & FOLL_LONGTERM) &&
+ if (refs > 1 && unlikely((flags & FOLL_LONGTERM) &&
!is_pinnable_page(page)))
return NULL;
...but are you really sure that this is the best way to "fix" the
problem? This trades correctness for "bug-for-bug compatibility" with
the previous code. It says, "it's OK to violate the pinnable and
longterm checks, as long as you do it one page at a time, rather than in
larger chunks.
Wouldn't it be better to try to fix up the calling code so that it's
not in violation of these zone rules?
I think the problem is before pin_user_pages can work with CMA pages
in the fallback path but now it doesn't work with CMA page. Driver
Actually, it "worked" only if the caller did it one page at a time.
(See how the above attempted fix restores the "make it work for
refs == 1.)
couldn't know whether it will work with CMA page or not in advance.
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.
thanks,
--
John Hubbard
NVIDIA