[PATCH] af_rxrpc: Keep rxrpc_call pointers in a hashtable

[David Howells]

Tim Smith <tim at electronghost.co.uk> wrote:

> Keep track of rxrpc_call structures in a hashtable so they can be
> found directly from the network parameters which define the call.

Just a few quick thoughts.

> +	const u8	*peer_addr)
> +{
> +	const u8 *p;

You can probably get away with using u16 instead, at least for p.  The
alignment should work.  You would need to fold the result though.

> +	uintptr_t key = 0;

I recommend using unsigned long type.

> +	key += (cid & RXRPC_CIDMASK) >> RXRPC_CIDSHIFT;
> +	key += cid & RXRPC_CHANNELMASK;

cid is __be32 not machine endian so this won't work.

You may want to fold key down to a 16-bit integer before taking the modulus.

> +	key = rxrpc_call_hashfunc(call->in_clientflag, call->cid,
> +				  call->call_id, call->epoch,
> +				  call->service_id, call->proto,
> +				  call->conn->trans->local, addr_size,
> +				  call->peer_ip.ipv6_addr);

The term 'ugh' springs to mind, but I don't think you can avoid all the
pointing.

> +	hash_for_each_rcu(rxrpc_call_hash, key, call, hash_node) {
> +		if (call->local == localptr

Compare the full computed hash value first.  You'll need to save it in the
call struct.  I'd also recommend checking the values in order of likelyhood of
being different, so something like:

	call->call_id
	call->cid
	call->in_clientflag
	call->service_id
	call->proto
	call->peer_ip
	call->local
	call->epoch

> +		    && call->in_clientflag == clientflag

'&&' at the end of the previous line please.

> @@ -704,7 +638,6 @@ void rxrpc_data_ready(struct sock *sk, int count)
>  		_debug("UDP socket error %d", ret);
>  		return;
>  	}
> -
>  	rxrpc_new_skb(skb);
>  
>  	_net("recv skb %p", skb);
> @@ -720,6 +653,7 @@ void rxrpc_data_ready(struct sock *sk, int count)
>  
>  	UDP_INC_STATS_BH(&init_net, UDP_MIB_INDATAGRAMS, 0);
>  
> +
>  	/* the socket buffer we have is owned by UDP, with UDP's data all over

Watch out for random blank line removal and additions.

> +	if (sp->hdr.callNumber == 0) {
> +		/* This is a connection-level packet. These should be
> +		 * fairly rare, so the extra overhead of looking them up the
> +		 * old-fashioned way doesn't really hurt */
> ...
> +		rxrpc_put_connection(conn);
> +	} else {

Turf the conn-level stuff out into its own function.

Looks reasonable otherwise.  I'll give it a more thorough going over tomorrow.

David