Messages in this thread | | | Date | Tue, 1 Mar 2022 00:40:47 -0800 | Subject | Re: [RFC PATCH 1/7] mm/gup: introduce pin_user_page() | From | John Hubbard <> |
| |
On 3/1/22 00:11, David Hildenbrand wrote: >> ... >>>> +EXPORT_SYMBOL(pin_user_page); >>>> + >>>> /* >>>> * pin_user_pages_unlocked() is the FOLL_PIN variant of >>>> * get_user_pages_unlocked(). Behavior is the same, except that this one sets >>> >>> I assume that function will only get called on a page that has been >>> obtained by a previous pin_user_pages_fast(), correct? >>> >> >> Well, no. This is meant to be used in place of get_page(), for code that >> knows that the pages will be released via unpin_user_page(). So there is >> no special prerequisite there. > > That might be problematic and possibly the wrong approach, depending on > *what* we're actually pinning and what we're intending to do with that. > > My assumption would have been that this interface is to duplicate a pin
I see that I need to put more documentation here, so people don't have to assume things... :)
> on a page, which would be perfectly fine, because the page actually saw > a FOLL_PIN previously. > > We're taking a pin on a page that we haven't obtained via FOLL_PIN if I > understand correctly. Which raises the questions, how do we end up with > the pages here, and what are we doing to do with them (use them like we > obtained them via FOLL_PIN?)? > > > If it's converting FOLL_GET -> FOLL_PIN manually, then we're bypassing > FOLL_PIN special handling in GUP code: > > page = get_user_pages(FOLL_GET) > pin_user_page(page) > put_page(page)
No, that's not where this is going at all. The idea, which I now see needs better documentation, is to handle file-backed pages. Only.
We're not converting from one type to another, nor are we doubling up. We're just keeping the pin type consistent so that the vast block- processing machinery can take pages in and handle them, then release them at the end with bio_release_pages(), which will call unpin_user_pages().
> > > For anonymous pages, we'll bail out for example once we have > > https://lkml.kernel.org/r/20220224122614.94921-14-david@redhat.com > > Because the conditions for pinned anonymous pages might no longer hold. > > If we won't call pin_user_page() on anonymous pages, it would be fine.
We won't, and in fact, I should add WARN_ON_ONCE(PageAnon(page)) to this function.
> But then, I still wonder how we come up the "struct page" here. >
From the file system. For example, the NFS-direct and fuse conversions in the last patches show how that works.
Thanks for this feedback, this is very helpful.
thanks, -- John Hubbard NVIDIA
| |