Re: [PATCH net-next v2 10/10] crypto: af_alg/hash: Support MSG_SPLICE_PAGES

From: David Howells <dhowells@redhat.com>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: dhowells@redhat.com, netdev@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
	David Ahern <dsahern@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Jens Axboe <axboe@kernel.dk>,
	linux-crypto@vger.kernel.org, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v2 10/10] crypto: af_alg/hash: Support MSG_SPLICE_PAGES
Date: Tue, 06 Jun 2023 10:24:55 +0100	[thread overview]
Message-ID: <1845449.1686043495@warthog.procyon.org.uk> (raw)
In-Reply-To: <ZH7xzYfwQoWZLUYa@gondor.apana.org.au>

Herbert Xu <herbert@gondor.apana.org.au> wrote:

> > -	if (limit > sk->sk_sndbuf)
> > -		limit = sk->sk_sndbuf;
> > +	/* Don't limit to ALG_MAX_PAGES if the pages are all already pinned. */
> > +	if (!user_backed_iter(&msg->msg_iter))
> > +		max_pages = INT_MAX;

If the iov_iter is a kernel-backed type (BVEC, KVEC, XARRAY) then (a) all the
pages it refers to must already be pinned in memory and (b) the caller must
have limited it in some way (splice is limited by the pipe capacity, for
instance).  In which case, it seems pointless taking more than one pass of the
while loop if we can avoid it - at least from the point of view of memory
handling; granted there might be other criteria such as hogging crypto offload
hardware.

> > +	else
> > +		max_pages = min_t(size_t, max_pages,
> > +				  DIV_ROUND_UP(sk->sk_sndbuf, PAGE_SIZE));
> 
> What's the purpose of relaxing this limit?

If the iov_iter is a user-backed type (IOVEC or UBUF) then it's not relaxed.
max_pages is ALG_MAX_PAGES here (actually, I should just move that here so
that it's clearer).

I am, however, applying the sk_sndbuf limit here also - there's no point
extracting more pages than we need to if ALG_MAX_PAGES of whole pages would
overrun the byte limit.

> Even if there is a reason for this shouldn't this be in a patch by itself?

I suppose I could do it as a follow-on patch; use ALG_MAX_PAGES and sk_sndbuf
before that as for user-backed iterators.

Actually, is it worth paying attention to sk_sndbuf for kernel-backed
iterators?

David