On 8/9/19 1:23 AM, Michal Hocko wrote:
On Fri 09-08-19 10:12:48, Vlastimil Babka wrote:
On 8/9/19 12:59 AM, John Hubbard wrote:
That's true. However, I'm not sure munlocking is where the
put_user_page() machinery is intended to be used anyway? These are
short-term pins for struct page manipulation, not e.g. dirtying of page
contents. Reading commit fc1d8e7cca2d I don't think this case falls
within the reasoning there. Perhaps not all GUP users should be
converted to the planned separate GUP tracking, and instead we should
have a GUP/follow_page_mask() variant that keeps using get_page/put_page?
Interesting. So far, the approach has been to get all the gup callers to
release via put_user_page(), but if we add in Jan's and Ira's vaddr_pin_pages()
wrapper, then maybe we could leave some sites unconverted.
However, in order to do so, we would have to change things so that we have
one set of APIs (gup) that do *not* increment a pin count, and another set
(vaddr_pin_pages) that do.
Is that where we want to go...?
We already have a FOLL_LONGTERM flag, isn't that somehow related? And if
it's not exactly the same thing, perhaps a new gup flag to distinguish
which kind of pinning to use?
Agreed. This is a shiny example how forcing all existing gup users into
the new scheme is subotimal at best. Not the mention the overal
fragility mention elsewhere. I dislike the conversion even more now.
Sorry if this was already discussed already but why the new pinning is
not bound to FOLL_LONGTERM (ideally hidden by an interface so that users
do not have to care about the flag) only?
Oh, it's been discussed alright, but given how some of the discussions have gone,
I certainly am not surprised that there are still questions and criticisms!
Especially since I may have misunderstood some of the points, along the way.
It's been quite a merry go round. :)
Anyway, what I'm hearing now is: for gup(FOLL_LONGTERM), apply the pinned tracking.
And therefore only do put_user_page() on pages that were pinned with
FOLL_LONGTERM. For short term pins, let the locking do what it will:
things can briefly block and all will be well.
Also, that may or may not come with a wrapper function, courtesy of Jan
and Ira.
Is that about right? It's late here, but I don't immediately recall any
problems with doing it that way...
thanks,
--
John Hubbard
NVIDIA
