search for: unpin_user_pag

Hi, It recently became clear to me that there are some get_user_pages*() callers that don't fit neatly into any of the four cases that are so far listed in pin_user_pages.rst. vhost.c is one of those. Add a Case 5 to the documentation, and refer to that when converting vhost.c. Thanks to Jan Kara for helping me (again) in understanding the interaction between get_user_pages() and page

...okes that pattern. In > +other words, if the code is neither Case 1 nor Case 2, it may still require > +FOLL_PIN, for patterns like this: > + > +Correct (uses FOLL_PIN calls): > + pin_user_pages() > + access the data within the pages > + set_page_dirty_lock() > + unpin_user_pages() > + > +INCORRECT (uses FOLL_GET calls): > + get_user_pages() > + access the data within the pages > + set_page_dirty_lock() > + put_page() Why does this case need to pin? Why can't it just do ... get_user_pages() lock_page(page); ... modify the data ......

This is based on Linux 5.7, plus one prerequisite patch: "mm/gup: update pin_user_pages.rst for "case 3" (mmu notifiers)" [1] Changes since v1: removed references to set_page_dirty*(), in response to Souptick Joarder's review (thanks!). Cover letter for v1, edited/updated slightly: It recently became clear to me that there are some get_user_pages*() callers that

..._user_pages > > > vhost_vdpa_map > > > iommu_map > > > > > > 3. kill QEMU > > > > > > 4. vhost_vdpa_release > > > vhost_vdpa_free_domain > > > > > > In this case, we have no opportunity to invoke unpin_user_pages or > > > iommu_unmap to free the memory. > > > > We do: > > > > vhost_vdpa_cleanup() > > vhost_vdpa_remove_as() > > vhost_vdpa_iotlb_unmap() > > vhost_vdpa_pa_unmap() > > unpin_user_pages() >...

...okes that pattern. In > +other words, if the code is neither Case 1 nor Case 2, it may still require > +FOLL_PIN, for patterns like this: > + > +Correct (uses FOLL_PIN calls): > + pin_user_pages() > + access the data within the pages > + set_page_dirty_lock() > + unpin_user_pages() > + > +INCORRECT (uses FOLL_GET calls): > + get_user_pages() > + access the data within the pages > + set_page_dirty_lock() > + put_page() > + > page_maybe_dma_pinned(): the whole point of pinning > =================================================== &gt...

...) > lock_limit) { > - ret = -ENOMEM; > - goto unlock; > - } > > - pinned = pin_user_pages(msg->uaddr & PAGE_MASK, npages, gup_flags, > - page_list, vmas); > - if (npages != pinned) { > - if (pinned < 0) { > - ret = pinned; > - } else { > - unpin_user_pages(page_list, pinned); > - ret = -ENOMEM; > - } > - goto unlock; > + if (locked > lock_limit) { > + ret = -ENOMEM; > + goto out; > } > > + cur_base = msg->uaddr & PAGE_MASK; > iova &= PAGE_MASK; > - map_pfn = page_to_pfn(page_list[0]); &g...

...t = -ENOMEM; - goto out; + goto unlock; } - cur_base = msg->uaddr & PAGE_MASK; - iova &= PAGE_MASK; + pinned = pin_user_pages(msg->uaddr & PAGE_MASK, npages, gup_flags, + page_list, vmas); + if (npages != pinned) { + if (pinned < 0) { + ret = pinned; + } else { + unpin_user_pages(page_list, pinned); + ret = -ENOMEM; + } + goto unlock; + } - while (npages) { - pinned = min_t(unsigned long, npages, list_size); - ret = pin_user_pages(cur_base, pinned, - gup_flags, page_list, NULL); - if (ret != pinned) - goto out; - - if (!last_pfn) - map_pfn = page_to_...

...t = -ENOMEM; - goto out; + goto unlock; } - cur_base = msg->uaddr & PAGE_MASK; - iova &= PAGE_MASK; + pinned = pin_user_pages(msg->uaddr & PAGE_MASK, npages, gup_flags, + page_list, vmas); + if (npages != pinned) { + if (pinned < 0) { + ret = pinned; + } else { + unpin_user_pages(page_list, pinned); + ret = -ENOMEM; + } + goto unlock; + } - while (npages) { - pinned = min_t(unsigned long, npages, list_size); - ret = pin_user_pages(cur_base, pinned, - gup_flags, page_list, NULL); - if (ret != pinned) - goto out; - - if (!last_pfn) - map_pfn = page_to_...

...e 1, plus Case 2, plus anything that invokes that pattern. In +other words, if the code is neither Case 1 nor Case 2, it may still require +FOLL_PIN, for patterns like this: + +Correct (uses FOLL_PIN calls): + pin_user_pages() + access the data within the pages + set_page_dirty_lock() + unpin_user_pages() + +INCORRECT (uses FOLL_GET calls): + get_user_pages() + access the data within the pages + set_page_dirty_lock() + put_page() + page_maybe_dma_pinned(): the whole point of pinning =================================================== -- 2.26.2

This code was using get_user_pages*(), in approximately a "Case 5" scenario (accessing the data within a page), using the categorization from [1]. That means that it's time to convert the get_user_pages*() + put_page() calls to pin_user_pages*() + unpin_user_pages() calls. There is some helpful background in [2]: basically, this is a small part of fixing a long-standing disconnect between pinning pages, and file systems' use of those pages. [1] Documentation/core-api/pin_user_pages.rst [2] "Explicit pinning of user-space pages": https:...

...idered a +superset of Case 1, plus Case 2, plus anything that invokes that pattern. In +other words, if the code is neither Case 1 nor Case 2, it may still require +FOLL_PIN, for patterns like this: + +Correct (uses FOLL_PIN calls): + pin_user_pages() + write to the data within the pages + unpin_user_pages() + +INCORRECT (uses FOLL_GET calls): + get_user_pages() + write to the data within the pages + put_page() + page_maybe_dma_pinned(): the whole point of pinning =================================================== -- 2.26.2

...nobody holds a reference on the page. - */ - if (count == 1) - free_devmap_managed_page(page); - else if (!count) - __put_page(page); - - return true; -} -#else -static bool __unpin_devmap_managed_user_page(struct page *page) -{ - return false; -} -#endif /* CONFIG_DEV_PAGEMAP_OPS */ - /** * unpin_user_page() - release a dma-pinned page * @page: pointer to page to be released @@ -227,15 +192,6 @@ void unpin_user_page(struct page *page) page = compound_head(page); - /* - * For devmap managed pages we need to catch refcount transition from - * GUP_PIN_COUNTING_BIAS to 1, when refco...

...nobody holds a reference on the page. - */ - if (count == 1) - free_devmap_managed_page(page); - else if (!count) - __put_page(page); - - return true; -} -#else -static bool __unpin_devmap_managed_user_page(struct page *page) -{ - return false; -} -#endif /* CONFIG_DEV_PAGEMAP_OPS */ - /** * unpin_user_page() - release a dma-pinned page * @page: pointer to page to be released @@ -227,15 +192,6 @@ void unpin_user_page(struct page *page) page = compound_head(page); - /* - * For devmap managed pages we need to catch refcount transition from - * GUP_PIN_COUNTING_BIAS to 1, when refco...

...he virtqueue. But reading Documentation/core-api/pin_user_pages.rst it seems that drivers should use "pin_user_pages*() for DMA-pinned pages", so I'm not sure what we should do. Additional advice would be great! Anyway, when we are done using the pages, we should call put_page() or unpin_user_page() depending on how we pin them. >+ >+ for (page_idx = 0; page_idx < pages_in_seg; page_idx++) { >+ int frag_len = PAGE_SIZE; >+ >+ if (page_idx == (pages_in_seg - 1)) >+ frag_len = last_frag_len; >+ >+ skb_fill_page_desc(skb, frag_idx++, >+ user_page...

Matthew Wilcox, Ira Weiny, and others have complained that ZONE_DEVICE struct page reference counting is ugly because they are "free" when the reference count is one instead of zero. This leads to explicit checks for ZONE_DEVICE pages in places like put_page(), GUP, THP splitting, and page migration which have to adjust the expected reference count when determining if the page is

This is still an RFC because after looking at the pmem/dax code some more, I realized that the ZONE_DEVICE struct pages are being inserted into the process' page tables with vmf_insert_mixed() and a zero refcount on the ZONE_DEVICE struct page. This is sort of OK because insert_pfn() increments the reference count on the pgmap which is what prevents memunmap_pages() from freeing the struct

...nobody holds a reference on the page. - */ - if (count == 1) - free_devmap_managed_page(page); - else if (!count) - __put_page(page); - - return true; -} -#else -static bool __unpin_devmap_managed_user_page(struct page *page) -{ - return false; -} -#endif /* CONFIG_DEV_PAGEMAP_OPS */ - /** * unpin_user_page() - release a dma-pinned page * @page: pointer to page to be released @@ -227,15 +192,6 @@ void unpin_user_page(struct page *page) page = compound_head(page); - /* - * For devmap managed pages we need to catch refcount transition from - * GUP_PIN_COUNTING_BIAS to 1, when refco...