Re: [PATCH mptcp-next v7 1/8] mptcp: add struct mptcp_sched_ops

[Florian Westphal <fw@strlen.de>]

Geliang Tang <geliang.tang@suse.com> wrote:
> This patch defines struct mptcp_sched_ops, which has three struct members,
> name, owner and list, and three function pointers, init, release and
> get_subflow.
> 
> Add the scheduler registering, unregistering and finding functions to add
> or delete or find a packet scheduler on the pernet sched_list.
> 
> Suggested-by: Florian Westphal <fw@strlen.de>

Huh?  I made some comments on earlier patch but I did not suggest this.
In fact, I don't like this at all but I guess I've been vetoed wrt.
pernet design.

> +struct mptcp_sched_ops *mptcp_sched_find(const struct net *net,
> +					 const char *name)
> +{
> +	struct sched_pernet *pernet = sched_get_pernet(net);
> +	struct mptcp_sched_ops *sched, *ret = NULL;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(sched, &pernet->sched_list, list) {
> +		if (!strcmp(sched->name, name)) {
> +			ret = sched;
> +			break;
> +		}
> +	}
> +	rcu_read_unlock();
> +	return ret;

This is fishy.  Either caller must already hold rcu read lock,
or mptcp_sched_ops can be free'd before function returns.

I suspect its best to force callers to do the rcu_read_lock and
document that this function is called with rcu read lock held.

> +int mptcp_register_scheduler(const struct net *net,
> +			     struct mptcp_sched_ops *sched)
> +{
> +	struct sched_pernet *pernet = sched_get_pernet(net);
> +
> +	if (!sched->get_subflow)
> +		return -EINVAL;
> +
> +	if (mptcp_sched_find(net, sched->name))
> +		return -EEXIST;
> +
> +	spin_lock(&pernet->lock);
> +	list_add_tail_rcu(&sched->list, &pernet->sched_list);
> +	spin_unlock(&pernet->lock);

This is racy.  Lock, test, add, unlock, not test, lock, ...

> +	spin_lock(&pernet->lock);
> +	list_del_rcu(&sched->list);
> +	spin_unlock(&pernet->lock);
> +
> +	/* avoid workqueue lockup */
> +	synchronize_rcu();

Hmm.  You mean 'wait until rcu read sides are done'?
But even that makes little sense to me.

There is no free operation done here so this is safe even
without synchronize_rcu()?