[RFC,23/28] algif: Remove hash_sendpage*()
Commit Message
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(-)
Comments
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,
@@ -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,
};