[MPTCP] Re: [PATCH 2/4] mptcp: refactor token container.

[Paolo Abeni <pabeni at redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

Hi,

On Fri, 2020-05-22 at 17:34 -0700, Mat Martineau wrote:
> > +struct token_bucket {
> > +	spinlock_t		lock;
> > +	int			chain_len;
> > +	struct hlist_nulls_head	req_chain;
> > +	struct hlist_nulls_head	msk_chain;
> > +};
> 
> I remember a discussion a while ago about how we didn't really need two 
> radix trees for req & msk, since there's only one "token space". 
> Similarly, seems like we only need one linked list per bucket if we can 
> define a common structure to use in both sock types, like:
> 
> struct mptcp_token_node {
>  	struct hlist_nulls_node node;
>  	u32 value;
>  	bool is_req;
> };
> 
> It would use a little more space per mptcp_sock/req_sock and would require 
> extra code in the lookup to return the appropriate container pointer (req 
> or mptcp_sock), but would shrink the hash table or give us more buckets in 
> the same memory, which seems like a win.

LGTM, I'll try that.

[...]
> > struct mptcp_sock *mptcp_token_get_sock(u32 token)
> > {
> > -	struct sock *conn;
> > -
> > -	spin_lock_bh(&token_tree_lock);
> > -	conn = radix_tree_lookup(&token_tree, token);
> > -	if (conn) {
> > -		/* token still reserved? */
> > -		if (conn == (struct sock *)&token_used)
> > -			conn = NULL;
> > -		else
> > -			sock_hold(conn);
> > +	struct hlist_nulls_node *pos;
> > +	struct token_bucket *bucket;
> > +	struct mptcp_sock *msk;
> > +
> > +	rcu_read_lock();
> > +	bucket = token_bucket(token);
> > +
> > +again:
> > +	hlist_nulls_for_each_entry_rcu(msk, pos, &bucket->msk_chain,
> > +				       token_node) {
> > +		if (READ_ONCE(msk->token) != token)
> > +			continue;
> > +		if (!refcount_inc_not_zero(&((struct sock *)msk)->sk_refcnt))
> > +			goto not_found;
> > +		if (READ_ONCE(msk->token) != token)
> > +			goto again;
> 
> Checking msk->token twice here, but it doesn't look like it ever gets 
> changed after it's set?

AFAICT, this is the common list_nulls usage pattern. The problem is
that a msk can be freed and re-allocated concurrently by another CPU.
Since this lookup is lockless, the token value can possibly change
'under-the-wood' till we get a reference to the token - and forbit msk
de-allocation.

Cheers,

Paolo