Re: [PATCH net-next v2 14/21] rxrpc: Do zerocopy using MSG_SPLICE_PAGES and page frags

[Simon Horman]

On Mon, Mar 04, 2024 at 08:43:11AM +0000, David Howells wrote:

..

> diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
> index ef0849c8329c..e540501a20ad 100644
> --- a/net/rxrpc/rxkad.c
> +++ b/net/rxrpc/rxkad.c
> @@ -145,16 +145,17 @@ static int rxkad_init_connection_security(struct rxrpc_connection *conn,
>  /*
>   * Work out how much data we can put in a packet.
>   */
> -static int rxkad_how_much_data(struct rxrpc_call *call, size_t remain,
> -			       size_t *_buf_size, size_t *_data_size, size_t *_offset)
> +static struct rxrpc_txbuf *rxkad_alloc_txbuf(struct rxrpc_call *call, size_t remain, gfp_t gfp)
>  {
> -	size_t shdr, buf_size, chunk;
> +	struct rxrpc_txbuf *txb;
> +	size_t shdr, space;
> +
> +	remain = min(remain, 65535 - sizeof(struct rxrpc_wire_header));
>  
>  	switch (call->conn->security_level) {
>  	default:
> -		buf_size = chunk = min_t(size_t, remain, RXRPC_JUMBO_DATALEN);
> -		shdr = 0;
> -		goto out;
> +		space = min_t(size_t, remain, RXRPC_JUMBO_DATALEN);
> +		return rxrpc_alloc_data_txbuf(call, space, 0, GFP_KERNEL);

Hi David,

should gfp be used here in place of GFP_KERNEL?

>  	case RXRPC_SECURITY_AUTH:
>  		shdr = sizeof(struct rxkad_level1_hdr);
>  		break;
> @@ -163,17 +164,15 @@ static int rxkad_how_much_data(struct rxrpc_call *call, size_t remain,
>  		break;
>  	}
>  
> -	buf_size = round_down(RXRPC_JUMBO_DATALEN, RXKAD_ALIGN);
> -
> -	chunk = buf_size - shdr;
> -	if (remain < chunk)
> -		buf_size = round_up(shdr + remain, RXKAD_ALIGN);
> +	space = min_t(size_t, round_down(RXRPC_JUMBO_DATALEN, RXKAD_ALIGN), remain + shdr);
> +	space = round_up(space, RXKAD_ALIGN);
>  
> -out:
> -	*_buf_size = buf_size;
> -	*_data_size = chunk;
> -	*_offset = shdr;
> -	return 0;
> +	txb = rxrpc_alloc_data_txbuf(call, space, RXKAD_ALIGN, GFP_KERNEL);

Likewise, here too.

Flagged by Smatch.

> +	if (txb) {
> +		txb->offset += shdr;
> +		txb->space -= shdr;
> +	}
> +	return txb;

nit: I think this would be a more idiomatic construction.
     (Completely untested!)

	if (!txb)
		return NULL;

	txb->offset += shdr;
	txb->space -= shdr;

	return txb;

>  }
>  
>  /*

..