| Message ID | 20190719200235.GA16122@bharath12345-Inspiron-5559 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] staging: kpc2000: Convert put_page to put_user_page*() | expand |
>From: Bharath Vedartham <linux.bhar@gmail.com> >Changes since v2 > - Added back PageResevered check as suggested by John Hubbard. > >The PageReserved check needs a closer look and is not worth messing >around with for now. > >Matt, Could you give any suggestions for testing this patch? Myself or someone else from Daktronics would have to do the testing since the hardware isn't really commercially available. I've been toying with the idea of asking for a volunteer from the mailing list to help me out with this - I'd send them some hardware and they'd do all the development and testing. :) I still have to run that idea by Management though. >If in-case, you are willing to pick this up to test. Could you >apply this patch to this tree and test it with your devices? I've been meaning to get to testing the changes to the drivers since upstreaming them, but I've been swamped with other development. I'm keeping an eye on the mailing lists, so I'm at least aware of what is coming down the pipe. I'm not too worried about this specific change, even though I don't really know if the reserved check and the dirtying are even necessary. It sounded like John's suggestion was to not do the PageReserved() check and just use put_user_pges_dirty() all the time. John, is that incorrect?
On 7/19/19 1:59 PM, Matt Sickler wrote: >> From: Bharath Vedartham <linux.bhar@gmail.com> >> Changes since v2 >> - Added back PageResevered check as suggested by John Hubbard. >> >> The PageReserved check needs a closer look and is not worth messing >> around with for now. >> >> Matt, Could you give any suggestions for testing this patch? > > Myself or someone else from Daktronics would have to do the testing since the > hardware isn't really commercially available. I've been toying with the idea > of asking for a volunteer from the mailing list to help me out with this - I'd > send them some hardware and they'd do all the development and testing. :) > I still have to run that idea by Management though. > >> If in-case, you are willing to pick this up to test. Could you >> apply this patch to this tree and test it with your devices? > > I've been meaning to get to testing the changes to the drivers since upstreaming > them, but I've been swamped with other development. I'm keeping an eye on the > mailing lists, so I'm at least aware of what is coming down the pipe. > I'm not too worried about this specific change, even though I don't really know > if the reserved check and the dirtying are even necessary. > It sounded like John's suggestion was to not do the PageReserved() check and just > use put_user_pges_dirty() all the time. John, is that incorrect? > That's what I suggested at first. But then I saw at least one other place where this pattern is being used, and it shook my confidence. I don't clearly see what the PageReserved check is protecting against here, but it's better to be safe, and do things in two steps: step 1 is *only* convert from put_page() to put_user_page(), and step 2 is to maybe remove the PageReserved() check, once fully understood. thanks,
On 7/19/19 1:02 PM, Bharath Vedartham wrote: > There have been issues with coordination of various subsystems using > get_user_pages. These issues are better described in [1]. > > An implementation of tracking get_user_pages is currently underway > The implementation requires the use put_user_page*() variants to release > a reference rather than put_page(). The commit that introduced > put_user_pages, Commit fc1d8e7cca2daa18d2fe56b94874848adf89d7f5 ("mm: introduce > put_user_page*(), placeholder version"). > > The implementation currently simply calls put_page() within > put_user_page(). But in the future, it is to change to add a mechanism > to keep track of get_user_pages. Once a tracking mechanism is > implemented, we can make attempts to work on improving on coordination > between various subsystems using get_user_pages. > > [1] https://lwn.net/Articles/753027/ Optional: I've been fussing about how to keep the change log reasonable, and finally came up with the following recommended template for these conversion patches. This would replace the text you have above, because the put_user_page placeholder commit has all the documentation (and then some) that we need: For pages that were retained via get_user_pages*(), release those pages via the new put_user_page*() routines, instead of via put_page(). This is part a tree-wide conversion, as described in commit fc1d8e7cca2d ("mm: introduce put_user_page*(), placeholder versions"). For the change itself, you will need to rebase it onto the latest linux.git, as it doesn't quite apply there. Testing is good if we can get it, but as far as I can tell this is correct, so you can also add: Reviewed-by: John Hubbard <jhubbard@nvidia.com> thanks,
On Fri, Jul 19, 2019 at 02:28:39PM -0700, John Hubbard wrote: > On 7/19/19 1:02 PM, Bharath Vedartham wrote: > > There have been issues with coordination of various subsystems using > > get_user_pages. These issues are better described in [1]. > > > > An implementation of tracking get_user_pages is currently underway > > The implementation requires the use put_user_page*() variants to release > > a reference rather than put_page(). The commit that introduced > > put_user_pages, Commit fc1d8e7cca2daa18d2fe56b94874848adf89d7f5 ("mm: introduce > > put_user_page*(), placeholder version"). > > > > The implementation currently simply calls put_page() within > > put_user_page(). But in the future, it is to change to add a mechanism > > to keep track of get_user_pages. Once a tracking mechanism is > > implemented, we can make attempts to work on improving on coordination > > between various subsystems using get_user_pages. > > > > [1] https://lwn.net/Articles/753027/ > > Optional: I've been fussing about how to keep the change log reasonable, > and finally came up with the following recommended template for these > conversion patches. This would replace the text you have above, because the > put_user_page placeholder commit has all the documentation (and then some) > that we need: > > > For pages that were retained via get_user_pages*(), release those pages > via the new put_user_page*() routines, instead of via put_page(). > > This is part a tree-wide conversion, as described in commit fc1d8e7cca2d > ("mm: introduce put_user_page*(), placeholder versions"). Great then, I ll send another patch with the updated changelog. > > For the change itself, you will need to rebase it onto the latest > linux.git, as it doesn't quite apply there. > > Testing is good if we can get it, but as far as I can tell this is > correct, so you can also add: > > Reviewed-by: John Hubbard <jhubbard@nvidia.com> Thanks! > thanks, > -- > John Hubbard > NVIDIA > > > > > Cc: Ira Weiny <ira.weiny@intel.com> > > Cc: John Hubbard <jhubbard@nvidia.com> > > Cc: Jérôme Glisse <jglisse@redhat.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Matt Sickler <Matt.Sickler@daktronics.com> > > Cc: devel@driverdev.osuosl.org > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-mm@kvack.org > > Signed-off-by: Bharath Vedartham <linux.bhar@gmail.com> > > --- > > Changes since v1 > > - Improved changelog by John's suggestion. > > - Moved logic to dirty pages below sg_dma_unmap > > and removed PageReserved check. > > Changes since v2 > > - Added back PageResevered check as suggested by John Hubbard. > > > > The PageReserved check needs a closer look and is not worth messing > > around with for now. > > > > Matt, Could you give any suggestions for testing this patch? > > > > If in-case, you are willing to pick this up to test. Could you > > apply this patch to this tree > > https://github.com/johnhubbard/linux/tree/gup_dma_core > > and test it with your devices? > > > > --- > > drivers/staging/kpc2000/kpc_dma/fileops.c | 17 ++++++----------- > > 1 file changed, 6 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c > > index 6166587..75ad263 100644 > > --- a/drivers/staging/kpc2000/kpc_dma/fileops.c > > +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c > > @@ -198,9 +198,7 @@ int kpc_dma_transfer(struct dev_private_data *priv, struct kiocb *kcb, unsigned > > sg_free_table(&acd->sgt); > > err_dma_map_sg: > > err_alloc_sg_table: > > - for (i = 0 ; i < acd->page_count ; i++){ > > - put_page(acd->user_pages[i]); > > - } > > + put_user_pages(acd->user_pages, acd->page_count); > > err_get_user_pages: > > kfree(acd->user_pages); > > err_alloc_userpages: > > @@ -221,16 +219,13 @@ void transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags) > > > > dev_dbg(&acd->ldev->pldev->dev, "transfer_complete_cb(acd = [%p])\n", acd); > > > > - for (i = 0 ; i < acd->page_count ; i++){ > > - if (!PageReserved(acd->user_pages[i])){ > > - set_page_dirty(acd->user_pages[i]); > > - } > > - } > > - > > dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir); > > > > - for (i = 0 ; i < acd->page_count ; i++){ > > - put_page(acd->user_pages[i]); > > + for (i = 0; i < acd->page_count; i++) { > > + if (!PageReserved(acd->user_pages[i])) > > + put_user_pages_dirty(&acd->user_pages[i], 1); > > + else > > + put_user_page(acd->user_pages[i]); > > } > > > > sg_free_table(&acd->sgt); > >
On Fri, Jul 19, 2019 at 08:59:02PM +0000, Matt Sickler wrote: > >From: Bharath Vedartham <linux.bhar@gmail.com> > >Changes since v2 > > - Added back PageResevered check as suggested by John Hubbard. > > > >The PageReserved check needs a closer look and is not worth messing > >around with for now. > > > >Matt, Could you give any suggestions for testing this patch? > > Myself or someone else from Daktronics would have to do the testing since the > hardware isn't really commercially available. I've been toying with the idea > of asking for a volunteer from the mailing list to help me out with this - I'd > send them some hardware and they'd do all the development and testing. :) > I still have to run that idea by Management though. > > >If in-case, you are willing to pick this up to test. Could you > >apply this patch to this tree and test it with your devices? > > I've been meaning to get to testing the changes to the drivers since upstreaming > them, but I've been swamped with other development. I'm keeping an eye on the > mailing lists, so I'm at least aware of what is coming down the pipe. > I'm not too worried about this specific change, even though I don't really know > if the reserved check and the dirtying are even necessary. > It sounded like John's suggestion was to not do the PageReserved() check and just > use put_user_pges_dirty() all the time. John, is that incorrect? The change is fairly trivial in the upstream kernel. It requires no testing in the upstream kernel. It would be great if you could test it on John's git tree with the implemented gup tracking subsystem and check if gup tracking is working alright with your dma driver. I think this patch will easily apply to John's git tree. Thanks! Bharath
diff --git a/drivers/staging/kpc2000/kpc_dma/fileops.c b/drivers/staging/kpc2000/kpc_dma/fileops.c index 6166587..75ad263 100644 --- a/drivers/staging/kpc2000/kpc_dma/fileops.c +++ b/drivers/staging/kpc2000/kpc_dma/fileops.c @@ -198,9 +198,7 @@ int kpc_dma_transfer(struct dev_private_data *priv, struct kiocb *kcb, unsigned sg_free_table(&acd->sgt); err_dma_map_sg: err_alloc_sg_table: - for (i = 0 ; i < acd->page_count ; i++){ - put_page(acd->user_pages[i]); - } + put_user_pages(acd->user_pages, acd->page_count); err_get_user_pages: kfree(acd->user_pages); err_alloc_userpages: @@ -221,16 +219,13 @@ void transfer_complete_cb(struct aio_cb_data *acd, size_t xfr_count, u32 flags) dev_dbg(&acd->ldev->pldev->dev, "transfer_complete_cb(acd = [%p])\n", acd); - for (i = 0 ; i < acd->page_count ; i++){ - if (!PageReserved(acd->user_pages[i])){ - set_page_dirty(acd->user_pages[i]); - } - } - dma_unmap_sg(&acd->ldev->pldev->dev, acd->sgt.sgl, acd->sgt.nents, acd->ldev->dir); - for (i = 0 ; i < acd->page_count ; i++){ - put_page(acd->user_pages[i]); + for (i = 0; i < acd->page_count; i++) { + if (!PageReserved(acd->user_pages[i])) + put_user_pages_dirty(&acd->user_pages[i], 1); + else + put_user_page(acd->user_pages[i]); } sg_free_table(&acd->sgt);
There have been issues with coordination of various subsystems using get_user_pages. These issues are better described in [1]. An implementation of tracking get_user_pages is currently underway The implementation requires the use put_user_page*() variants to release a reference rather than put_page(). The commit that introduced put_user_pages, Commit fc1d8e7cca2daa18d2fe56b94874848adf89d7f5 ("mm: introduce put_user_page*(), placeholder version"). The implementation currently simply calls put_page() within put_user_page(). But in the future, it is to change to add a mechanism to keep track of get_user_pages. Once a tracking mechanism is implemented, we can make attempts to work on improving on coordination between various subsystems using get_user_pages. [1] https://lwn.net/Articles/753027/ Cc: Ira Weiny <ira.weiny@intel.com> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Jérôme Glisse <jglisse@redhat.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Matt Sickler <Matt.Sickler@daktronics.com> Cc: devel@driverdev.osuosl.org Cc: linux-kernel@vger.kernel.org Cc: linux-mm@kvack.org Signed-off-by: Bharath Vedartham <linux.bhar@gmail.com> --- Changes since v1 - Improved changelog by John's suggestion. - Moved logic to dirty pages below sg_dma_unmap and removed PageReserved check. Changes since v2 - Added back PageResevered check as suggested by John Hubbard. The PageReserved check needs a closer look and is not worth messing around with for now. Matt, Could you give any suggestions for testing this patch? If in-case, you are willing to pick this up to test. Could you apply this patch to this tree https://github.com/johnhubbard/linux/tree/gup_dma_core and test it with your devices? --- drivers/staging/kpc2000/kpc_dma/fileops.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)