| Message ID | 20230123173007.325544-4-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On 1/23/23 09:30, David Howells wrote: > Provide a helper in the get_user_pages code to drop a pin or a ref on a > page based on being given FOLL_GET or FOLL_PIN in its flags argument or do > nothing if neither is set. > > Signed-off-by: David Howells <dhowells@redhat.com> > cc: Al Viro <viro@zeniv.linux.org.uk> > cc: Christoph Hellwig <hch@lst.de> > cc: Matthew Wilcox <willy@infradead.org> > cc: linux-fsdevel@vger.kernel.org > cc: linux-block@vger.kernel.org > cc: linux-mm@kvack.org > --- > include/linux/mm.h | 3 +++ > mm/gup.c | 22 ++++++++++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 8f857163ac89..3de9d88f8524 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1367,6 +1367,9 @@ static inline bool is_cow_mapping(vm_flags_t flags) > #define SECTION_IN_PAGE_FLAGS > #endif > > +void folio_put_unpin(struct folio *folio, unsigned int flags); > +void page_put_unpin(struct page *page, unsigned int flags); How about these names instead: folio_put_or_unpin() page_put_or_unpin() ? Also, could we please change the name of the flags argument, to gup_flags? > + > /* > * The identification function is mainly used by the buddy allocator for > * determining if two pages could be buddies. We are not really identifying > diff --git a/mm/gup.c b/mm/gup.c > index f45a3a5be53a..3ee4b4c7e0cb 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -191,6 +191,28 @@ static void gup_put_folio(struct folio *folio, int refs, unsigned int flags) > folio_put_refs(folio, refs); > } > > +/** > + * folio_put_unpin - Unpin/put a folio as appropriate > + * @folio: The folio to release > + * @flags: gup flags indicating the mode of release (FOLL_*) > + * > + * Release a folio according to the flags. If FOLL_GET is set, the folio has a > + * ref dropped; if FOLL_PIN is set, it is unpinned; otherwise it is left > + * unaltered. > + */ > +void folio_put_unpin(struct folio *folio, unsigned int flags) > +{ > + if (flags & (FOLL_GET | FOLL_PIN)) Another minor complication is that FOLL_PIN is supposed to be an internal-to-mm flag. But here (and in another part of the series), it has leaked into the public API. One approach would be to give up and just admit that, like FOLL_GET, FOLL_PIN has escaped into the wild. Another approach would be to use a new set of flags, such as USE_FOLL_GET and USE_FOLL_PIN. But I'm starting to lean toward the first approach: just let FOLL_PIN be used in this way, treat it as part of the external API at least for this area (not for gup/pup calls, though). So after all that thinking out loud, I think this is OK to use FOLL_PIN. +Cc Jason Gunthorpe, because he is about to split up FOLL_* into public and internal sets. thanks,
On 23.01.23 18:30, David Howells wrote: > Provide a helper in the get_user_pages code to drop a pin or a ref on a > page based on being given FOLL_GET or FOLL_PIN in its flags argument or do > nothing if neither is set. Does the FOLL_GET part still apply to this patch set?
David Hildenbrand <david@redhat.com> wrote: > > Provide a helper in the get_user_pages code to drop a pin or a ref on a > > page based on being given FOLL_GET or FOLL_PIN in its flags argument or do > > nothing if neither is set. > > Does the FOLL_GET part still apply to this patch set? Yes. Christoph insisted that the bio conversion patch be split up. That means there's an interval where you can get FOLL_GET from that. However, since Jason wants to hide FOLL_PUT, this is going to have to be removed and the switching done in the bio code for the bio case (until that reduces to just pinning) and the skbuff cleanup code (when that is eventually done - that will have the possibility of skbuffs comprising a mix of ref'd, pinned and unpinned data, albeit in separate fragments; I've posted patches that illustrate this[1]). David https://lore.kernel.org/all/167391047703.2311931.8115712773222260073.stgit@warthog.procyon.org.uk/ [1] Patches 33-34
On Tue, Jan 24, 2023 at 02:41:33PM +0000, David Howells wrote: > Yes. Christoph insisted that the bio conversion patch be split up. That > means there's an interval where you can get FOLL_GET from that. The only place where we have both is in the block layer. It never gets set by bio_set_cleanup_mode. Instead we can just keep using put_page dirctly for the BIO_PAGE_REFFED case in the callers of bio_release_page and in bio_release_pages itself, and then do away with bio_to_gup_flags and bio_release_page entirely.
On 24.01.23 15:52, Christoph Hellwig wrote: > On Tue, Jan 24, 2023 at 02:41:33PM +0000, David Howells wrote: >> Yes. Christoph insisted that the bio conversion patch be split up. That >> means there's an interval where you can get FOLL_GET from that. > > The only place where we have both is in the block layer. It never gets > set by bio_set_cleanup_mode. > > Instead we can just keep using put_page dirctly for the BIO_PAGE_REFFED > case in the callers of bio_release_page and in bio_release_pages itself, > and then do away with bio_to_gup_flags and bio_release_page entirely. > Thank you, just what I had in mind ...
Christoph Hellwig <hch@infradead.org> wrote: > The only place where we have both is in the block layer. It never gets > set by bio_set_cleanup_mode. It gets set directly by patch 6. David
diff --git a/include/linux/mm.h b/include/linux/mm.h index 8f857163ac89..3de9d88f8524 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1367,6 +1367,9 @@ static inline bool is_cow_mapping(vm_flags_t flags) #define SECTION_IN_PAGE_FLAGS #endif +void folio_put_unpin(struct folio *folio, unsigned int flags); +void page_put_unpin(struct page *page, unsigned int flags); + /* * The identification function is mainly used by the buddy allocator for * determining if two pages could be buddies. We are not really identifying diff --git a/mm/gup.c b/mm/gup.c index f45a3a5be53a..3ee4b4c7e0cb 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -191,6 +191,28 @@ static void gup_put_folio(struct folio *folio, int refs, unsigned int flags) folio_put_refs(folio, refs); } +/** + * folio_put_unpin - Unpin/put a folio as appropriate + * @folio: The folio to release + * @flags: gup flags indicating the mode of release (FOLL_*) + * + * Release a folio according to the flags. If FOLL_GET is set, the folio has a + * ref dropped; if FOLL_PIN is set, it is unpinned; otherwise it is left + * unaltered. + */ +void folio_put_unpin(struct folio *folio, unsigned int flags) +{ + if (flags & (FOLL_GET | FOLL_PIN)) + gup_put_folio(folio, 1, flags); +} +EXPORT_SYMBOL_GPL(folio_put_unpin); + +void page_put_unpin(struct page *page, unsigned int flags) +{ + folio_put_unpin(page_folio(page), flags); +} +EXPORT_SYMBOL_GPL(page_put_unpin); + /** * try_grab_page() - elevate a page's refcount by a flag-dependent amount * @page: pointer to page to be grabbed
Provide a helper in the get_user_pages code to drop a pin or a ref on a page based on being given FOLL_GET or FOLL_PIN in its flags argument or do nothing if neither is set. Signed-off-by: David Howells <dhowells@redhat.com> cc: Al Viro <viro@zeniv.linux.org.uk> cc: Christoph Hellwig <hch@lst.de> cc: Matthew Wilcox <willy@infradead.org> cc: linux-fsdevel@vger.kernel.org cc: linux-block@vger.kernel.org cc: linux-mm@kvack.org --- include/linux/mm.h | 3 +++ mm/gup.c | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+)