Re: [PATCH 1/3] crypto: scatterwalk - Use nth_page instead of doing it by hand

[Eric Biggers <ebiggers@kernel.org>]

On Tue, Mar 11, 2025 at 06:20:29PM +0800, Herbert Xu wrote:
>  static inline void scatterwalk_map(struct scatter_walk *walk)
>  {
> -	struct page *base_page = sg_page(walk->sg);
> +	struct page *page = sg_page(walk->sg);
> +	unsigned int offset = walk->offset;
> +	void *addr;
> +
> +	page = nth_page(page, offset >> PAGE_SHIFT);
> +	offset = offset_in_page(offset);
>  
>  	if (IS_ENABLED(CONFIG_HIGHMEM)) {
> -		walk->__addr = kmap_local_page(base_page +
> -					       (walk->offset >> PAGE_SHIFT)) +
> -			       offset_in_page(walk->offset);
> +		addr = kmap_local_page(page) + offset;
>  	} else {
>  		/*
>  		 * When !HIGHMEM we allow the walker to return segments that
>  		 * span a page boundary; see scatterwalk_clamp().  To make it
>  		 * clear that in this case we're working in the linear buffer of
>  		 * the whole sg entry in the kernel's direct map rather than
> -		 * within the mapped buffer of a single page, compute the
> -		 * address as an offset from the page_address() of the first
> -		 * page of the sg entry.  Either way the result is the address
> -		 * in the direct map, but this makes it clearer what is really
> -		 * going on.
> +		 * within the mapped buffer of a single page, use
> +		 * page_address() instead of going through kmap.
>  		 */
> -		walk->__addr = page_address(base_page) + walk->offset;
> +		addr = page_address(page) + offset;
>  	}
> +	walk->__addr = addr;

In the !HIGHMEM case (i.e., the common case) this is just worse, though.  It
expands into more instructions than before, only to get the same linear address
that it did before.  You also seem to be ignoring the comment that explains that
we're working in the linear buffer of the whole sg entry.

- Eric