| Message ID | 20191125231035.1539120-18-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pin_user_pages(): reduced-risk series for Linux 5.5 | expand |
On Mon 25-11-19 15:10:33, John Hubbard wrote: > 1. Convert from get_user_pages() to pin_user_pages(). > > 2. As required by pin_user_pages(), release these pages via > put_user_page(). In this case, do so via put_user_pages_dirty_lock(). > > That has the side effect of calling set_page_dirty_lock(), instead > of set_page_dirty(). This is probably more accurate. Maybe more accurate but it doesn't work for mm_iommu_unpin(). As I'm checking mm_iommu_unpin() gets called from RCU callback which is executed interrupt context and you cannot lock pages from such context. So you need to queue work from the RCU callback and then do the real work from the workqueue... Honza > > As Christoph Hellwig put it, "set_page_dirty() is only safe if we are > dealing with a file backed page where we have reference on the inode it > hangs off." [1] > > [1] https://lore.kernel.org/r/20190723153640.GB720@lst.de > > Cc: Jan Kara <jack@suse.cz> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> > --- > arch/powerpc/mm/book3s64/iommu_api.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) > > diff --git a/arch/powerpc/mm/book3s64/iommu_api.c b/arch/powerpc/mm/book3s64/iommu_api.c > index 56cc84520577..fc1670a6fc3c 100644 > --- a/arch/powerpc/mm/book3s64/iommu_api.c > +++ b/arch/powerpc/mm/book3s64/iommu_api.c > @@ -103,7 +103,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, > for (entry = 0; entry < entries; entry += chunk) { > unsigned long n = min(entries - entry, chunk); > > - ret = get_user_pages(ua + (entry << PAGE_SHIFT), n, > + ret = pin_user_pages(ua + (entry << PAGE_SHIFT), n, > FOLL_WRITE | FOLL_LONGTERM, > mem->hpages + entry, NULL); > if (ret == n) { > @@ -167,9 +167,8 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, > return 0; > > free_exit: > - /* free the reference taken */ > - for (i = 0; i < pinned; i++) > - put_page(mem->hpages[i]); > + /* free the references taken */ > + put_user_pages(mem->hpages, pinned); > > vfree(mem->hpas); > kfree(mem); > @@ -212,10 +211,9 @@ static void mm_iommu_unpin(struct mm_iommu_table_group_mem_t *mem) > if (!page) > continue; > > - if (mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY) > - SetPageDirty(page); > + put_user_pages_dirty_lock(&page, 1, > + mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY); > > - put_page(page); > mem->hpas[i] = 0; > } > } > -- > 2.24.0 >
On 11/29/19 3:23 AM, Jan Kara wrote: > On Mon 25-11-19 15:10:33, John Hubbard wrote: >> 1. Convert from get_user_pages() to pin_user_pages(). >> >> 2. As required by pin_user_pages(), release these pages via >> put_user_page(). In this case, do so via put_user_pages_dirty_lock(). >> >> That has the side effect of calling set_page_dirty_lock(), instead >> of set_page_dirty(). This is probably more accurate. > > Maybe more accurate but it doesn't work for mm_iommu_unpin(). As I'm > checking mm_iommu_unpin() gets called from RCU callback which is executed > interrupt context and you cannot lock pages from such context. So you need > to queue work from the RCU callback and then do the real work from the > workqueue... > > Honza ah yes, fixed locally. (In order to avoid distracting people during the merge window, I won't post any more versions of the series until the merge window is over, unless a maintainer tells me that any of these patches are desired for 5.5.) With that, we are back to a one-line diff for this part: @@ -215,7 +214,7 @@ static void mm_iommu_unpin(struct mm_iommu_table_group_mem_t *mem) if (mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY) SetPageDirty(page); - put_page(page); + put_user_page(page); mem->hpas[i] = 0; } } btw, I'm also working on your feedback for patch 17 (mm/gup: track FOLL_PIN pages [1]), from a few days earlier, it's not being ignored, I'm just trying to avoid distracting people during the merge window. [1] https://lore.kernel.org/r/20191121093941.GA18190@quack2.suse.cz thanks,
diff --git a/arch/powerpc/mm/book3s64/iommu_api.c b/arch/powerpc/mm/book3s64/iommu_api.c index 56cc84520577..fc1670a6fc3c 100644 --- a/arch/powerpc/mm/book3s64/iommu_api.c +++ b/arch/powerpc/mm/book3s64/iommu_api.c @@ -103,7 +103,7 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, for (entry = 0; entry < entries; entry += chunk) { unsigned long n = min(entries - entry, chunk); - ret = get_user_pages(ua + (entry << PAGE_SHIFT), n, + ret = pin_user_pages(ua + (entry << PAGE_SHIFT), n, FOLL_WRITE | FOLL_LONGTERM, mem->hpages + entry, NULL); if (ret == n) { @@ -167,9 +167,8 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, return 0; free_exit: - /* free the reference taken */ - for (i = 0; i < pinned; i++) - put_page(mem->hpages[i]); + /* free the references taken */ + put_user_pages(mem->hpages, pinned); vfree(mem->hpas); kfree(mem); @@ -212,10 +211,9 @@ static void mm_iommu_unpin(struct mm_iommu_table_group_mem_t *mem) if (!page) continue; - if (mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY) - SetPageDirty(page); + put_user_pages_dirty_lock(&page, 1, + mem->hpas[i] & MM_IOMMU_TABLE_GROUP_PAGE_DIRTY); - put_page(page); mem->hpas[i] = 0; } }
1. Convert from get_user_pages() to pin_user_pages(). 2. As required by pin_user_pages(), release these pages via put_user_page(). In this case, do so via put_user_pages_dirty_lock(). That has the side effect of calling set_page_dirty_lock(), instead of set_page_dirty(). This is probably more accurate. As Christoph Hellwig put it, "set_page_dirty() is only safe if we are dealing with a file backed page where we have reference on the inode it hangs off." [1] [1] https://lore.kernel.org/r/20190723153640.GB720@lst.de Cc: Jan Kara <jack@suse.cz> Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- arch/powerpc/mm/book3s64/iommu_api.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)