| Message ID | 20190227011051.GB24463@iweiny-DESK2.sc.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | put_user_page() question | expand |
On 2/26/19 5:10 PM, Ira Weiny wrote: > John, > > With your put_user_page patchset, Is there a reason you don't call > put_user_page() in the 2 spots shown in the diff below? > > And Jason, in the second case I find it odd that the driver needs to clear the > invalidate_range() callback prior to the final ib_umem_odp_unmap_dma_pages() > call such that put_user_page() is called. I am correct that this is how the > pages finally get put in the end? > > Ira > > diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c > index 730b2fa0942e..a63f5eda02ca 100644 > --- a/drivers/infiniband/core/umem_odp.c > +++ b/drivers/infiniband/core/umem_odp.c > @@ -687,7 +687,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, > if (ret < 0) { > /* Release left over pages when handling errors. */ > for (++j; j < npages; ++j) > - put_page(local_page_list[j]); > + put_user_page(local_page_list[j]); Hi Ira, This one I somehow just overlooked. It definitely should have been a put_user_page() call. > break; > } > } > @@ -750,7 +750,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt, > } > /* on demand pinning support */ > if (!umem->context->invalidate_range) > - put_page(page); > + put_user_page(page); ...and this one, I didn't recognize as being part of the pool of pages that were acquired via get_user_pages(). But assuming that it is acquired via get_user_pages(), then of course it also should be changed to put_user_page(), you're right. Glad you're taking a hard look at this! thanks,
On Tue, Feb 26, 2019 at 05:22:44PM -0800, John Hubbard wrote: > On 2/26/19 5:10 PM, Ira Weiny wrote: > > John, > > > > With your put_user_page patchset, Is there a reason you don't call > > put_user_page() in the 2 spots shown in the diff below? > > > > And Jason, in the second case I find it odd that the driver needs to clear the > > invalidate_range() callback prior to the final ib_umem_odp_unmap_dma_pages() > > call such that put_user_page() is called. I am correct that this is how the > > pages finally get put in the end? > > > > Ira > > > > diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c > > index 730b2fa0942e..a63f5eda02ca 100644 > > --- a/drivers/infiniband/core/umem_odp.c > > +++ b/drivers/infiniband/core/umem_odp.c > > @@ -687,7 +687,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, > > if (ret < 0) { > > /* Release left over pages when handling errors. */ > > for (++j; j < npages; ++j) > > - put_page(local_page_list[j]); > > + put_user_page(local_page_list[j]); > > Hi Ira, > > This one I somehow just overlooked. It definitely should have been a put_user_page() > call. > > > break; > > } > > } > > @@ -750,7 +750,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt, > > } > > /* on demand pinning support */ > > if (!umem->context->invalidate_range) > > - put_page(page); > > + put_user_page(page); > > > ...and this one, I didn't recognize as being part of the pool of pages that > were acquired via get_user_pages(). But assuming that it is acquired via > get_user_pages(), then of course it also should be changed to put_user_page(), > you're right. Yes I'm 99% sure this matches up with the get_user_pages_remote() in ib_umem_odp_map_dma_pages(). But the check for the invalidate_range() callback threw me a bit. Hence my question to Jason regarding the requirements of the driver when calling unmap. It seems like if the driver has not cleared the invalidate callback then there could be a potential for an unmatched GUP depending on what the driver is doing with the umem. But I'm not 100% sure I've found a problem. Ira > > Glad you're taking a hard look at this! > > > thanks, > -- > John Hubbard > NVIDIA
On 2/27/19 10:28 AM, Ira Weiny wrote: > On Tue, Feb 26, 2019 at 05:22:44PM -0800, John Hubbard wrote: >> On 2/26/19 5:10 PM, Ira Weiny wrote: >>> John, >>> >>> With your put_user_page patchset, Is there a reason you don't call >>> put_user_page() in the 2 spots shown in the diff below? >>> >>> And Jason, in the second case I find it odd that the driver needs to clear the >>> invalidate_range() callback prior to the final ib_umem_odp_unmap_dma_pages() >>> call such that put_user_page() is called. I am correct that this is how the >>> pages finally get put in the end? >>> >>> Ira >>> >>> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c >>> index 730b2fa0942e..a63f5eda02ca 100644 >>> --- a/drivers/infiniband/core/umem_odp.c >>> +++ b/drivers/infiniband/core/umem_odp.c >>> @@ -687,7 +687,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, >>> if (ret < 0) { >>> /* Release left over pages when handling errors. */ >>> for (++j; j < npages; ++j) >>> - put_page(local_page_list[j]); >>> + put_user_page(local_page_list[j]); >> >> Hi Ira, >> >> This one I somehow just overlooked. It definitely should have been a put_user_page() >> call. >> >>> break; >>> } >>> } >>> @@ -750,7 +750,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt, >>> } >>> /* on demand pinning support */ >>> if (!umem->context->invalidate_range) >>> - put_page(page); >>> + put_user_page(page); >> >> >> ...and this one, I didn't recognize as being part of the pool of pages that >> were acquired via get_user_pages(). But assuming that it is acquired via >> get_user_pages(), then of course it also should be changed to put_user_page(), >> you're right. > > Yes I'm 99% sure this matches up with the get_user_pages_remote() in > ib_umem_odp_map_dma_pages(). But the check for the invalidate_range() callback > threw me a bit. Hence my question to Jason regarding the requirements of the > driver when calling unmap. It seems like if the driver has not cleared the > invalidate callback then there could be a potential for an unmatched > GUP depending on what the driver is doing with the umem. But I'm > not 100% sure I've found a problem. > > Ira > Jason, we could use your advice for this one, if you have a moment to take a quick peek? thanks,
On Sat, Mar 02, 2019 at 09:25:42PM -0400, Jason Gunthorpe wrote: > On Wed, Feb 27, 2019 at 10:28:13AM -0800, Ira Weiny wrote: > > On Tue, Feb 26, 2019 at 05:22:44PM -0800, John Hubbard wrote: > > > On 2/26/19 5:10 PM, Ira Weiny wrote: > > > > John, > > > > > > > > With your put_user_page patchset, Is there a reason you don't call > > > > put_user_page() in the 2 spots shown in the diff below? > > > > > > > > And Jason, in the second case I find it odd that the driver needs to clear the > > > > invalidate_range() callback prior to the final ib_umem_odp_unmap_dma_pages() > > > > call such that put_user_page() is called. I am correct that this is how the > > > > pages finally get put in the end? > > > > > > > > Ira > > > > > > > > diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c > > > > index 730b2fa0942e..a63f5eda02ca 100644 > > > > +++ b/drivers/infiniband/core/umem_odp.c > > > > @@ -687,7 +687,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, > > > > if (ret < 0) { > > > > /* Release left over pages when handling errors. */ > > > > for (++j; j < npages; ++j) > > > > - put_page(local_page_list[j]); > > > > + put_user_page(local_page_list[j]); > > > > > > Hi Ira, > > > > > > This one I somehow just overlooked. It definitely should have been a put_user_page() > > > call. > > > > > > > break; > > > > } > > > > } > > > > @@ -750,7 +750,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt, > > > > } > > > > /* on demand pinning support */ > > > > if (!umem->context->invalidate_range) > > > > - put_page(page); > > > > + put_user_page(page); > > > > > > > > > ...and this one, I didn't recognize as being part of the pool of pages that > > > were acquired via get_user_pages(). But assuming that it is acquired via > > > get_user_pages(), then of course it also should be changed to put_user_page(), > > > you're right. > > > > Yes I'm 99% sure this matches up with the get_user_pages_remote() in > > ib_umem_odp_map_dma_pages(). But the check for the > > invalidate_range() callback > > invalidate_range does not change, it always set to something if the > driver supports ODP, and since this function accepts a ib_umem_odp it > cannot be called on non-ODP umem's.. > > In this case I assume the code can never be called?? Ok, that is what I thought... So where is the matching put_[user_]page (currently put_page but John's change would be user page) for the get_user_pages_remote() call? (CC Haggai) I think this is it and I think it is a bug to have the check here. Or perhaps the check should be: if (umem->context->invalidate_range) ... ??? Ira > > Jason
On Wed, Feb 27, 2019 at 10:28:13AM -0800, Ira Weiny wrote: > On Tue, Feb 26, 2019 at 05:22:44PM -0800, John Hubbard wrote: > > On 2/26/19 5:10 PM, Ira Weiny wrote: > > > John, > > > > > > With your put_user_page patchset, Is there a reason you don't call > > > put_user_page() in the 2 spots shown in the diff below? > > > > > > And Jason, in the second case I find it odd that the driver needs to clear the > > > invalidate_range() callback prior to the final ib_umem_odp_unmap_dma_pages() > > > call such that put_user_page() is called. I am correct that this is how the > > > pages finally get put in the end? > > > > > > Ira > > > > > > diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c > > > index 730b2fa0942e..a63f5eda02ca 100644 > > > +++ b/drivers/infiniband/core/umem_odp.c > > > @@ -687,7 +687,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, > > > if (ret < 0) { > > > /* Release left over pages when handling errors. */ > > > for (++j; j < npages; ++j) > > > - put_page(local_page_list[j]); > > > + put_user_page(local_page_list[j]); > > > > Hi Ira, > > > > This one I somehow just overlooked. It definitely should have been a put_user_page() > > call. > > > > > break; > > > } > > > } > > > @@ -750,7 +750,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt, > > > } > > > /* on demand pinning support */ > > > if (!umem->context->invalidate_range) > > > - put_page(page); > > > + put_user_page(page); > > > > > > ...and this one, I didn't recognize as being part of the pool of pages that > > were acquired via get_user_pages(). But assuming that it is acquired via > > get_user_pages(), then of course it also should be changed to put_user_page(), > > you're right. > > Yes I'm 99% sure this matches up with the get_user_pages_remote() in > ib_umem_odp_map_dma_pages(). But the check for the > invalidate_range() callback invalidate_range does not change, it always set to something if the driver supports ODP, and since this function accepts a ib_umem_odp it cannot be called on non-ODP umem's.. In this case I assume the code can never be called?? Jason
On 3/2/2019 9:54 PM, Ira Weiny wrote: > On Sat, Mar 02, 2019 at 09:25:42PM -0400, Jason Gunthorpe wrote: >> On Wed, Feb 27, 2019 at 10:28:13AM -0800, Ira Weiny wrote: >>> On Tue, Feb 26, 2019 at 05:22:44PM -0800, John Hubbard wrote: >>>> On 2/26/19 5:10 PM, Ira Weiny wrote: >>>>> John, >>>>> >>>>> With your put_user_page patchset, Is there a reason you don't call >>>>> put_user_page() in the 2 spots shown in the diff below? >>>>> >>>>> And Jason, in the second case I find it odd that the driver needs to clear the >>>>> invalidate_range() callback prior to the final ib_umem_odp_unmap_dma_pages() >>>>> call such that put_user_page() is called. I am correct that this is how the >>>>> pages finally get put in the end? >>>>> >>>>> Ira >>>>> >>>>> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c >>>>> index 730b2fa0942e..a63f5eda02ca 100644 >>>>> +++ b/drivers/infiniband/core/umem_odp.c >>>>> @@ -687,7 +687,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, >>>>> if (ret < 0) { >>>>> /* Release left over pages when handling errors. */ >>>>> for (++j; j < npages; ++j) >>>>> - put_page(local_page_list[j]); >>>>> + put_user_page(local_page_list[j]); >>>> >>>> Hi Ira, >>>> >>>> This one I somehow just overlooked. It definitely should have been a put_user_page() >>>> call. >>>> >>>>> break; >>>>> } >>>>> } >>>>> @@ -750,7 +750,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt, >>>>> } >>>>> /* on demand pinning support */ >>>>> if (!umem->context->invalidate_range) >>>>> - put_page(page); >>>>> + put_user_page(page); >>>> >>>> >>>> ...and this one, I didn't recognize as being part of the pool of pages that >>>> were acquired via get_user_pages(). But assuming that it is acquired via >>>> get_user_pages(), then of course it also should be changed to put_user_page(), >>>> you're right. >>> >>> Yes I'm 99% sure this matches up with the get_user_pages_remote() in >>> ib_umem_odp_map_dma_pages(). But the check for the >>> invalidate_range() callback >> >> invalidate_range does not change, it always set to something if the >> driver supports ODP, and since this function accepts a ib_umem_odp it >> cannot be called on non-ODP umem's.. >> >> In this case I assume the code can never be called?? > > Ok, that is what I thought... So where is the matching put_[user_]page > (currently put_page but John's change would be user page) for the > get_user_pages_remote() call? (CC Haggai) I also think this code is never called. IIRC, in order to split the on-demand paging patch-set we had an intermediate mode called "on-demand pinning", where the driver would only support page faults, but not invalidations. In this mode the page references are taken when a page fault occurs, but are held until the MR is released. As far as I know no driver uses this mode, so it is probably safe to remove it. > > I think this is it and I think it is a bug to have the check here. Or perhaps > the check should be: > > > if (umem->context->invalidate_range) > ... > ??? The thought behind this interface was that invalidate_range callback would either be set or cleared during the per mm or the umem creation, so it is stable between the map and unmap calls. If it was set during map, then the page reference is immediately released there, so there's no need for another put_page in unmap. Regards, Haggai
On Sun, Mar 03, 2019 at 06:00:35AM +0000, Haggai Eran wrote: > On 3/2/2019 9:54 PM, Ira Weiny wrote: > > On Sat, Mar 02, 2019 at 09:25:42PM -0400, Jason Gunthorpe wrote: > >> On Wed, Feb 27, 2019 at 10:28:13AM -0800, Ira Weiny wrote: > >>> On Tue, Feb 26, 2019 at 05:22:44PM -0800, John Hubbard wrote: > >>>> On 2/26/19 5:10 PM, Ira Weiny wrote: > >>>>> John, > >>>>> > >>>>> With your put_user_page patchset, Is there a reason you don't call > >>>>> put_user_page() in the 2 spots shown in the diff below? > >>>>> > >>>>> And Jason, in the second case I find it odd that the driver needs to clear the > >>>>> invalidate_range() callback prior to the final ib_umem_odp_unmap_dma_pages() > >>>>> call such that put_user_page() is called. I am correct that this is how the > >>>>> pages finally get put in the end? > >>>>> > >>>>> Ira > >>>>> > >>>>> diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c > >>>>> index 730b2fa0942e..a63f5eda02ca 100644 > >>>>> +++ b/drivers/infiniband/core/umem_odp.c > >>>>> @@ -687,7 +687,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, > >>>>> if (ret < 0) { > >>>>> /* Release left over pages when handling errors. */ > >>>>> for (++j; j < npages; ++j) > >>>>> - put_page(local_page_list[j]); > >>>>> + put_user_page(local_page_list[j]); > >>>> > >>>> Hi Ira, > >>>> > >>>> This one I somehow just overlooked. It definitely should have been a put_user_page() > >>>> call. > >>>> > >>>>> break; > >>>>> } > >>>>> } > >>>>> @@ -750,7 +750,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt, > >>>>> } > >>>>> /* on demand pinning support */ > >>>>> if (!umem->context->invalidate_range) > >>>>> - put_page(page); > >>>>> + put_user_page(page); > >>>> > >>>> > >>>> ...and this one, I didn't recognize as being part of the pool of pages that > >>>> were acquired via get_user_pages(). But assuming that it is acquired via > >>>> get_user_pages(), then of course it also should be changed to put_user_page(), > >>>> you're right. > >>> > >>> Yes I'm 99% sure this matches up with the get_user_pages_remote() in > >>> ib_umem_odp_map_dma_pages(). But the check for the > >>> invalidate_range() callback > >> > >> invalidate_range does not change, it always set to something if the > >> driver supports ODP, and since this function accepts a ib_umem_odp it > >> cannot be called on non-ODP umem's.. > >> > >> In this case I assume the code can never be called?? > > > > Ok, that is what I thought... So where is the matching put_[user_]page > > (currently put_page but John's change would be user page) for the > > get_user_pages_remote() call? (CC Haggai) > > I also think this code is never called. IIRC, in order to split the > on-demand paging patch-set we had an intermediate mode called "on-demand > pinning", where the driver would only support page faults, but not > invalidations. In this mode the page references are taken when a page > fault occurs, but are held until the MR is released. As far as I know no > driver uses this mode, so it is probably safe to remove it. > > > > > I think this is it and I think it is a bug to have the check here. Or perhaps > > the check should be: > > > > > > if (umem->context->invalidate_range) > > ... > > ??? > > The thought behind this interface was that invalidate_range callback > would either be set or cleared during the per mm or the umem creation, > so it is stable between the map and unmap calls. If it was set during > map, then the page reference is immediately released there, so there's > no need for another put_page in unmap. I'm missing something. I don't see any check for the invalidate_range callback in ib_umem_odp_map_dma_pages(). So I'm still missing when the pages which get pinned by the GUP_remote() call there are "put". It seems like the ib_umem_odp_[un]map_dma_pages() should mirror each other. Ira > > Regards, > Haggai
diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c index 730b2fa0942e..a63f5eda02ca 100644 --- a/drivers/infiniband/core/umem_odp.c +++ b/drivers/infiniband/core/umem_odp.c @@ -687,7 +687,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem_odp *umem_odp, u64 user_virt, if (ret < 0) { /* Release left over pages when handling errors. */ for (++j; j < npages; ++j) - put_page(local_page_list[j]); + put_user_page(local_page_list[j]); break; } } @@ -750,7 +750,7 @@ void ib_umem_odp_unmap_dma_pages(struct ib_umem_odp *umem_odp, u64 virt, } /* on demand pinning support */ if (!umem->context->invalidate_range) - put_page(page); + put_user_page(page); umem_odp->page_list[idx] = NULL; umem_odp->dma_list[idx] = 0; umem->npages--;