[PATCH v3] staging: kpc2000: Convert put_page to put_user_page*()

[PATCH v3] staging: kpc2000: Convert put_page to put_user_page*()

[Bharath Vedartham]

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(-)

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);
-- 
2.7.4

RE: [PATCH v3] staging: kpc2000: Convert put_page to put_user_page*()

[Matt Sickler]

>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?

Re: [PATCH v3] staging: kpc2000: Convert put_page to put_user_page*()

[John Hubbard]

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,
-- 
John Hubbard
NVIDIA

Re: [PATCH v3] staging: kpc2000: Convert put_page to put_user_page*()

[John Hubbard]

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,
-- 
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);
> 

Re: [PATCH v3] staging: kpc2000: Convert put_page to put_user_page*()

[Bharath Vedartham]

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);
> > 

Re: [PATCH v3] staging: kpc2000: Convert put_page to put_user_page*()

[Bharath Vedartham]

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