Re: [PATCH net-next 19/19] rxrpc: Use RCU to access a peer's service connection tree

[Peter Zijlstra]

On Thu, Jun 30, 2016 at 03:12:58PM +0100, David Howells wrote:
> +struct rxrpc_connection *rxrpc_find_service_conn_rcu(struct rxrpc_peer *peer,
> +						     struct sk_buff *skb)
> +{
> +	struct rxrpc_connection *conn = NULL;
> +	struct rxrpc_conn_proto k;
> +	struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
> +	struct rb_node *p;
> +	unsigned int seq;
> +
> +	k.epoch	= sp->hdr.epoch;
> +	k.cid	= sp->hdr.cid & RXRPC_CIDMASK;
> +
> +	do {
> +		/* Unfortunately, rbtree walking doesn't give reliable results
> +		 * under just the RCU read lock, so we have to check for
> +		 * changes.
> +		 */
> +		read_seqbegin_or_lock(&peer->service_conn_lock, &seq);
> +
> +		p = peer->service_conns.rb_node;
> +		while (p) {
> +			conn = rb_entry(p, struct rxrpc_connection, service_node);
> +
> +			if (conn->proto.index_key < k.index_key)
> +				p = p->rb_left;
> +			else if (conn->proto.index_key > k.index_key)
> +				p = p->rb_right;

You still very much need rcu_dereference() for both left and right
pointers. As well as the first p load.

> +			else
> +				goto done;
> +			conn = NULL;
> +		}
> +	} while (need_seqretry(&peer->service_conn_lock, seq));
> +
> +done:
> +	done_seqretry(&peer->service_conn_lock, seq);
> +	_leave(" = %d", conn ? conn->debug_id : -1);
> +	return conn;
> +}


> +static struct rxrpc_connection *
> +rxrpc_publish_service_conn(struct rxrpc_peer *peer,
> +			   struct rxrpc_connection *conn)
> +{
> +	struct rxrpc_connection *cursor = NULL;
> +	struct rxrpc_conn_proto k = conn->proto;
> +	struct rb_node **pp, *parent;
> +
> +	write_seqlock_bh(&peer->service_conn_lock);
> +
> +	pp = &peer->service_conns.rb_node;
> +	parent = NULL;
> +	while (*pp) {
> +		parent = *pp;
> +		cursor = rb_entry(parent,
> +				  struct rxrpc_connection, service_node);
> +
> +		if (cursor->proto.index_key < k.index_key)
> +			pp = &(*pp)->rb_left;
> +		else if (cursor->proto.index_key > k.index_key)
> +			pp = &(*pp)->rb_right;
> +		else
> +			goto found_extant_conn;
> +	}
> +
> +	rb_link_node(&conn->service_node, parent, pp);

You want rb_link_node_rcu() here.

> +	rb_insert_color(&conn->service_node, &peer->service_conns);
> +conn_published:
> +	set_bit(RXRPC_CONN_IN_SERVICE_CONNS, &conn->flags);
> +	write_sequnlock_bh(&peer->service_conn_lock);
> +	_leave(" = %d [new]", conn->debug_id);
> +	return conn;