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@xxxxxxxxxx>

thanks,
-- 
John Hubbard
NVIDIA

> 
> Cc: Ira Weiny <ira.weiny@xxxxxxxxx>
> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> Cc: JÃrÃme Glisse <jglisse@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Matt Sickler <Matt.Sickler@xxxxxxxxxxxxxx>
> Cc: devel@xxxxxxxxxxxxxxxxxxxx 
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Cc: linux-mm@xxxxxxxxx
> Signed-off-by: Bharath Vedartham <linux.bhar@xxxxxxxxx>
> ---
> 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);
>