Received-SPF: pass (google.com: domain of linux-kernel+bounces-92483-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 5 Mar 2024 14:03:43 +0000
From: Simon Horman <horms@kernel.org>
To: David Howells <dhowells@redhat.com>
Cc: netdev@vger.kernel.org, Marc Dionne <marc.dionne@auristor.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	linux-afs@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v2 14/21] rxrpc: Do zerocopy using
 MSG_SPLICE_PAGES and page frags
Message-ID: <20240305140343.GH2357@kernel.org>
References: <20240304084322.705539-1-dhowells@redhat.com>
 <20240304084322.705539-15-dhowells@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240304084322.705539-15-dhowells@redhat.com>

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;

>  }
>  
>  /*

..