| Message ID | 20230316152618.711970-24-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | splice, net: Replace sendpage with sendmsg(MSG_SPLICE_PAGES) | expand |
David Howells <dhowells@redhat.com> wrote: > Remove hash_sendpage*() and use hash_sendmsg() as the latter seems to just > use the source pages directly anyway. ... > - if (!(flags & MSG_MORE)) { > - if (ctx->more) > - err = crypto_ahash_finup(&ctx->req); > - else > - err = crypto_ahash_digest(&ctx->req); You've just removed the optimised path from user-space to finup/digest. You need to add them back to sendmsg if you want to eliminate sendpage. Cheers,
Herbert Xu <herbert@gondor.apana.org.au> wrote: > David Howells <dhowells@redhat.com> wrote: > > Remove hash_sendpage*() and use hash_sendmsg() as the latter seems to just > > use the source pages directly anyway. > > ... > > > - if (!(flags & MSG_MORE)) { > > - if (ctx->more) > > - err = crypto_ahash_finup(&ctx->req); > > - else > > - err = crypto_ahash_digest(&ctx->req); > > You've just removed the optimised path from user-space to > finup/digest. You need to add them back to sendmsg if you > want to eliminate sendpage. I must be missing something, I think. What's particularly optimal about the code in hash_sendpage() but not hash_sendmsg()? Is it that the former uses finup/digest, but the latter ony does update+final? Also, looking at: if (!ctx->more) { if ((msg->msg_flags & MSG_MORE)) hash_free_result(sk, ctx); how is ctx->more meant to be interpreted? I'm guessing it means that we're continuing to the previous op. But we do we need to free any old result if MSG_MORE is set, but not if it isn't? David
On Fri, Mar 24, 2023 at 04:47:50PM +0000, David Howells wrote: > > I must be missing something, I think. What's particularly optimal about the > code in hash_sendpage() but not hash_sendmsg()? Is it that the former uses > finup/digest, but the latter ony does update+final? A lot of hardware hashes can't perform partial updates, so they will always fall back to software unless you use finup/digest. Cheers,
Herbert Xu <herbert@gondor.apana.org.au> wrote: > > I must be missing something, I think. What's particularly optimal about the > > code in hash_sendpage() but not hash_sendmsg()? Is it that the former uses > > finup/digest, but the latter ony does update+final? > > A lot of hardware hashes can't perform partial updates, so they > will always fall back to software unless you use finup/digest. Okay. Btw, how much of a hard limit is ALG_MAX_PAGES? Multipage folios can exceed the current limit (16 pages, 64K) in size. Is it just to prevent too much memory being pinned at once? David
On Sat, Mar 25, 2023 at 07:44:14AM +0000, David Howells wrote: > > Okay. Btw, how much of a hard limit is ALG_MAX_PAGES? Multipage folios can > exceed the current limit (16 pages, 64K) in size. Is it just to prevent too > much memory being pinned at once? Yes, we don't want user-space to be able to pin an unlimited amount of memory. Cheers,
diff --git a/crypto/algif_hash.c b/crypto/algif_hash.c index 1d017ec5c63c..52f5828a054a 100644 --- a/crypto/algif_hash.c +++ b/crypto/algif_hash.c @@ -129,58 +129,6 @@ static int hash_sendmsg(struct socket *sock, struct msghdr *msg, return err ?: copied; } -static ssize_t hash_sendpage(struct socket *sock, struct page *page, - int offset, size_t size, int flags) -{ - struct sock *sk = sock->sk; - struct alg_sock *ask = alg_sk(sk); - struct hash_ctx *ctx = ask->private; - int err; - - if (flags & MSG_SENDPAGE_NOTLAST) - flags |= MSG_MORE; - - lock_sock(sk); - sg_init_table(ctx->sgl.sg, 1); - sg_set_page(ctx->sgl.sg, page, size, offset); - - if (!(flags & MSG_MORE)) { - err = hash_alloc_result(sk, ctx); - if (err) - goto unlock; - } else if (!ctx->more) - hash_free_result(sk, ctx); - - ahash_request_set_crypt(&ctx->req, ctx->sgl.sg, ctx->result, size); - - if (!(flags & MSG_MORE)) { - if (ctx->more) - err = crypto_ahash_finup(&ctx->req); - else - err = crypto_ahash_digest(&ctx->req); - } else { - if (!ctx->more) { - err = crypto_ahash_init(&ctx->req); - err = crypto_wait_req(err, &ctx->wait); - if (err) - goto unlock; - } - - err = crypto_ahash_update(&ctx->req); - } - - err = crypto_wait_req(err, &ctx->wait); - if (err) - goto unlock; - - ctx->more = flags & MSG_MORE; - -unlock: - release_sock(sk); - - return err ?: size; -} - static int hash_recvmsg(struct socket *sock, struct msghdr *msg, size_t len, int flags) { @@ -285,7 +233,6 @@ static struct proto_ops algif_hash_ops = { .release = af_alg_release, .sendmsg = hash_sendmsg, - .sendpage = hash_sendpage, .recvmsg = hash_recvmsg, .accept = hash_accept, }; @@ -337,18 +284,6 @@ static int hash_sendmsg_nokey(struct socket *sock, struct msghdr *msg, return hash_sendmsg(sock, msg, size); } -static ssize_t hash_sendpage_nokey(struct socket *sock, struct page *page, - int offset, size_t size, int flags) -{ - int err; - - err = hash_check_key(sock); - if (err) - return err; - - return hash_sendpage(sock, page, offset, size, flags); -} - static int hash_recvmsg_nokey(struct socket *sock, struct msghdr *msg, size_t ignored, int flags) { @@ -387,7 +322,6 @@ static struct proto_ops algif_hash_ops_nokey = { .release = af_alg_release, .sendmsg = hash_sendmsg_nokey, - .sendpage = hash_sendpage_nokey, .recvmsg = hash_recvmsg_nokey, .accept = hash_accept_nokey, };
Remove hash_sendpage*() and use hash_sendmsg() as the latter seems to just use the source pages directly anyway. Signed-off-by: David Howells <dhowells@redhat.com> cc: Herbert Xu <herbert@gondor.apana.org.au> cc: "David S. Miller" <davem@davemloft.net> cc: Eric Dumazet <edumazet@google.com> cc: Jakub Kicinski <kuba@kernel.org> cc: Paolo Abeni <pabeni@redhat.com> cc: Jens Axboe <axboe@kernel.dk> cc: Matthew Wilcox <willy@infradead.org> cc: linux-crypto@vger.kernel.org cc: netdev@vger.kernel.org --- crypto/algif_hash.c | 66 --------------------------------------------- 1 file changed, 66 deletions(-)