Re: [PATCH 1/8] mm/gup: break out gup_fill_pages() helper

[David Hildenbrand (Arm)]

On 11/2/26 20:30, Rik van Riel wrote:
> __get_user_pages() fills pages[] and flushes each page's caches in an
> open-coded loop.
> 
> Move it into a gup_fill_pages() helper, which the follow_page_mask()
> call chain can then use to fill its own pages[] slots.
> 
> No functional changes intended.
> 
> Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
> Reviewed-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> ---
>  mm/gup.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 7d43ce717b246..cdbe2ed3e6442 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -633,6 +633,23 @@ static struct page *no_page_table(struct vm_area_struct *vma,
>  	return NULL;
>  }
>  
> +static void gup_fill_pages(struct vm_area_struct *vma, unsigned long address,
> +		struct page *page, unsigned long nr, struct page **pages)

unsigned long nr_pages ?


> +{
> +	unsigned long i;
> +
> +	if (!pages)
> +		return;

Why is that check needed? Why should someone try filling something that doesn't
exist?

> +
> +	for (i = 0; i < nr; i++) {
> +		struct page *subpage = page + i;

There is no such thing as a subpage as Willy keeps saying :)

Why not reuse "page" ?

	for (i = 0; i < nr; i++, page++) {

> +
> +		pages[i] = subpage;
> +		flush_anon_page(vma, subpage, address + i * PAGE_SIZE);
> +		flush_dcache_page(subpage);

Now I wonder why GUP-fast doesn't need any of that. Likely because architectures
that support GUP-fast don't need any such magic.

Maybe we should in fact base this on the gup-fast batching series and also use
the helper in there (passing vma=NULL)?

-- 
Cheers,

David