[PATCH v3 04/19] crypto: scatterwalk - add new functions for copying data

Eric Biggers posted 19 patches 10 months ago
[PATCH v3 04/19] crypto: scatterwalk - add new functions for copying data
Posted by Eric Biggers 10 months ago
From: Eric Biggers <ebiggers@google.com>

Add memcpy_from_sglist() and memcpy_to_sglist() which are more readable
versions of scatterwalk_map_and_copy() with the 'out' argument 0 and 1
respectively.  They follow the same argument order as memcpy_from_page()
and memcpy_to_page() from <linux/highmem.h>.  Note that in the case of
memcpy_from_sglist(), this also happens to be the same argument order
that scatterwalk_map_and_copy() uses.

The new code is also faster, mainly because it builds the scatter_walk
directly without creating a temporary scatterlist.  E.g., a 20%
performance improvement is seen for copying the AES-GCM auth tag.

Make scatterwalk_map_and_copy() be a wrapper around memcpy_from_sglist()
and memcpy_to_sglist().  Callers of scatterwalk_map_and_copy() should be
updated to call memcpy_from_sglist() or memcpy_to_sglist() directly, but
there are a lot of them so they aren't all being updated right away.

Also add functions memcpy_from_scatterwalk() and memcpy_to_scatterwalk()
which are similar but operate on a scatter_walk instead of a
scatterlist.  These will replace scatterwalk_copychunks() with the 'out'
argument 0 and 1 respectively.  Their behavior differs slightly from
scatterwalk_copychunks() in that they automatically take care of
flushing the dcache when needed, making them easier to use.

scatterwalk_copychunks() itself is left unchanged for now.  It will be
removed after its callers are updated to use other functions instead.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/scatterwalk.c         | 59 ++++++++++++++++++++++++++++++------
 include/crypto/scatterwalk.h | 24 +++++++++++++--
 2 files changed, 72 insertions(+), 11 deletions(-)

diff --git a/crypto/scatterwalk.c b/crypto/scatterwalk.c
index af436ad02e3ff..2e7a532152d61 100644
--- a/crypto/scatterwalk.c
+++ b/crypto/scatterwalk.c
@@ -65,26 +65,67 @@ void scatterwalk_copychunks(void *buf, struct scatter_walk *walk,
 		scatterwalk_pagedone(walk, out & 1, 1);
 	}
 }
 EXPORT_SYMBOL_GPL(scatterwalk_copychunks);
 
-void scatterwalk_map_and_copy(void *buf, struct scatterlist *sg,
-			      unsigned int start, unsigned int nbytes, int out)
+inline void memcpy_from_scatterwalk(void *buf, struct scatter_walk *walk,
+				    unsigned int nbytes)
+{
+	do {
+		const void *src_addr;
+		unsigned int to_copy;
+
+		src_addr = scatterwalk_next(walk, nbytes, &to_copy);
+		memcpy(buf, src_addr, to_copy);
+		scatterwalk_done_src(walk, src_addr, to_copy);
+		buf += to_copy;
+		nbytes -= to_copy;
+	} while (nbytes);
+}
+EXPORT_SYMBOL_GPL(memcpy_from_scatterwalk);
+
+inline void memcpy_to_scatterwalk(struct scatter_walk *walk, const void *buf,
+				  unsigned int nbytes)
+{
+	do {
+		void *dst_addr;
+		unsigned int to_copy;
+
+		dst_addr = scatterwalk_next(walk, nbytes, &to_copy);
+		memcpy(dst_addr, buf, to_copy);
+		scatterwalk_done_dst(walk, dst_addr, to_copy);
+		buf += to_copy;
+		nbytes -= to_copy;
+	} while (nbytes);
+}
+EXPORT_SYMBOL_GPL(memcpy_to_scatterwalk);
+
+void memcpy_from_sglist(void *buf, struct scatterlist *sg,
+			unsigned int start, unsigned int nbytes)
 {
 	struct scatter_walk walk;
-	struct scatterlist tmp[2];
 
-	if (!nbytes)
+	if (unlikely(nbytes == 0)) /* in case sg == NULL */
 		return;
 
-	sg = scatterwalk_ffwd(tmp, sg, start);
+	scatterwalk_start_at_pos(&walk, sg, start);
+	memcpy_from_scatterwalk(buf, &walk, nbytes);
+}
+EXPORT_SYMBOL_GPL(memcpy_from_sglist);
+
+void memcpy_to_sglist(struct scatterlist *sg, unsigned int start,
+		      const void *buf, unsigned int nbytes)
+{
+	struct scatter_walk walk;
+
+	if (unlikely(nbytes == 0)) /* in case sg == NULL */
+		return;
 
-	scatterwalk_start(&walk, sg);
-	scatterwalk_copychunks(buf, &walk, nbytes, out);
-	scatterwalk_done(&walk, out, 0);
+	scatterwalk_start_at_pos(&walk, sg, start);
+	memcpy_to_scatterwalk(&walk, buf, nbytes);
 }
-EXPORT_SYMBOL_GPL(scatterwalk_map_and_copy);
+EXPORT_SYMBOL_GPL(memcpy_to_sglist);
 
 struct scatterlist *scatterwalk_ffwd(struct scatterlist dst[2],
 				     struct scatterlist *src,
 				     unsigned int len)
 {
diff --git a/include/crypto/scatterwalk.h b/include/crypto/scatterwalk.h
index 8e83c43016c9d..1689ecd7ddafa 100644
--- a/include/crypto/scatterwalk.h
+++ b/include/crypto/scatterwalk.h
@@ -168,12 +168,32 @@ static inline void scatterwalk_done_dst(struct scatter_walk *walk,
 void scatterwalk_skip(struct scatter_walk *walk, unsigned int nbytes);
 
 void scatterwalk_copychunks(void *buf, struct scatter_walk *walk,
 			    size_t nbytes, int out);
 
-void scatterwalk_map_and_copy(void *buf, struct scatterlist *sg,
-			      unsigned int start, unsigned int nbytes, int out);
+void memcpy_from_scatterwalk(void *buf, struct scatter_walk *walk,
+			     unsigned int nbytes);
+
+void memcpy_to_scatterwalk(struct scatter_walk *walk, const void *buf,
+			   unsigned int nbytes);
+
+void memcpy_from_sglist(void *buf, struct scatterlist *sg,
+			unsigned int start, unsigned int nbytes);
+
+void memcpy_to_sglist(struct scatterlist *sg, unsigned int start,
+		      const void *buf, unsigned int nbytes);
+
+/* In new code, please use memcpy_{from,to}_sglist() directly instead. */
+static inline void scatterwalk_map_and_copy(void *buf, struct scatterlist *sg,
+					    unsigned int start,
+					    unsigned int nbytes, int out)
+{
+	if (out)
+		memcpy_to_sglist(sg, start, buf, nbytes);
+	else
+		memcpy_from_sglist(buf, sg, start, nbytes);
+}
 
 struct scatterlist *scatterwalk_ffwd(struct scatterlist dst[2],
 				     struct scatterlist *src,
 				     unsigned int len);
 
-- 
2.48.1
Re: [PATCH v3 04/19] crypto: scatterwalk - add new functions for copying data
Posted by Herbert Xu 9 months, 3 weeks ago
Eric Biggers <ebiggers@kernel.org> wrote:
>
> +void memcpy_from_sglist(void *buf, struct scatterlist *sg,
> +                       unsigned int start, unsigned int nbytes)
> {
>        struct scatter_walk walk;
> -       struct scatterlist tmp[2];
> 
> -       if (!nbytes)
> +       if (unlikely(nbytes == 0)) /* in case sg == NULL */
>                return;
> 
> -       sg = scatterwalk_ffwd(tmp, sg, start);
> +       scatterwalk_start_at_pos(&walk, sg, start);
> +       memcpy_from_scatterwalk(buf, &walk, nbytes);
> +}
> +EXPORT_SYMBOL_GPL(memcpy_from_sglist);
> +
> +void memcpy_to_sglist(struct scatterlist *sg, unsigned int start,
> +                     const void *buf, unsigned int nbytes)

These functions duplicate sg_copy_buffer.  Of course scatterwalk
in general duplicates SG miter which came later IIRC.

What's your plan for eliminating this duplication?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v3 04/19] crypto: scatterwalk - add new functions for copying data
Posted by Eric Biggers 9 months, 3 weeks ago
On Sun, Mar 02, 2025 at 02:40:56PM +0800, Herbert Xu wrote:
> Eric Biggers <ebiggers@kernel.org> wrote:
> >
> > +void memcpy_from_sglist(void *buf, struct scatterlist *sg,
> > +                       unsigned int start, unsigned int nbytes)
> > {
> >        struct scatter_walk walk;
> > -       struct scatterlist tmp[2];
> > 
> > -       if (!nbytes)
> > +       if (unlikely(nbytes == 0)) /* in case sg == NULL */
> >                return;
> > 
> > -       sg = scatterwalk_ffwd(tmp, sg, start);
> > +       scatterwalk_start_at_pos(&walk, sg, start);
> > +       memcpy_from_scatterwalk(buf, &walk, nbytes);
> > +}
> > +EXPORT_SYMBOL_GPL(memcpy_from_sglist);
> > +
> > +void memcpy_to_sglist(struct scatterlist *sg, unsigned int start,
> > +                     const void *buf, unsigned int nbytes)
> 
> These functions duplicate sg_copy_buffer.  Of course scatterwalk
> in general duplicates SG miter which came later IIRC.
> 
> What's your plan for eliminating this duplication?
> 
> Thanks,

The new functions are much better than the lib/scatterlist.c ones: they have a
much better implementation that is faster and doesn't use atomic kmaps, and
(like scatterwalk_map_and_copy() which they are replacing first) they don't
require the unhelpful 'nents' parameter.  My tentative plan is to move them into
lib/scatterlist.c, reimplement sg_copy_buffer() et al on top of them, then
eventually update the callers to use the new functions directly.

However, the 'nents' parameter that sg_copy_buffer() et al take will make the
unification a bit difficult.  Currently those functions copy the minimum of
'buflen' bytes and the first 'nents' scatterlist elements.  I'd like to remove
the 'nents' parameter and just have 'buflen' (or rather 'nbytes'), like the
crypto/scatterwalk.c functions.  I suspect that nearly all callers are passing
in enough 'nents' to cover their 'buflen'.  But there may be some exceptions,
which we'll need to check for.

- Eric
Re: [PATCH v3 04/19] crypto: scatterwalk - add new functions for copying data
Posted by Herbert Xu 9 months, 2 weeks ago
On Sun, Mar 02, 2025 at 01:37:42PM -0800, Eric Biggers wrote:
>
> The new functions are much better than the lib/scatterlist.c ones: they have a
> much better implementation that is faster and doesn't use atomic kmaps, and
> (like scatterwalk_map_and_copy() which they are replacing first) they don't
> require the unhelpful 'nents' parameter.  My tentative plan is to move them into
> lib/scatterlist.c, reimplement sg_copy_buffer() et al on top of them, then
> eventually update the callers to use the new functions directly.

Sounds good.

> However, the 'nents' parameter that sg_copy_buffer() et al take will make the
> unification a bit difficult.  Currently those functions copy the minimum of
> 'buflen' bytes and the first 'nents' scatterlist elements.  I'd like to remove
> the 'nents' parameter and just have 'buflen' (or rather 'nbytes'), like the
> crypto/scatterwalk.c functions.  I suspect that nearly all callers are passing
> in enough 'nents' to cover their 'buflen'.  But there may be some exceptions,
> which we'll need to check for.

Yes this duality of sg_nents vs. total length was always annoying
even within the Crypto API as the driver authors would often get
mixed up.

If we could settle on one of the two it would be great.  Of course
I don't know how much work that's going to be and it could be
prohibitive.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt