Re: [PATCH mptcp-net v2 2/3] mptcp: pm: lockless list traversal

[Geliang Tang <geliang@kernel.org>]

Hi Matt,

Thanks for this patch.

On Fri, 2024-10-25 at 11:32 +0200, Matthieu Baerts (NGI0) wrote:
> In a few places -- to get an endpoint, dump all of them, and change
> their flags -- the list is iterated while holding the pernet->lock,
> but
> only to read the content of the list. In these cases, we can replace
> the
> spin locks, by RCU read ones, and use the _rcu variants to iterate
> over
> the entries list in a lockless way.
> 
> To make it clear, the lookup helpers using the _rcu variant are
> renamed
> with a _rcu suffix. The previous __lookup_addr() helper can then be
> removed, but __lookup_addr_by_id() is still needed.
> 
> While at it, the IDs bitmap is copied before iterating the list to
> dump
> the different addresses, to avoid any consistencies.
> 
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Notes:
>  - This is not a fix, a small improvement for -next.
> ---
>  net/mptcp/pm_netlink.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index
> a93b9b7776b48781a883673fe5fd521a978487ff..f38e1ccd34e95cd88b179a8b50e
> 6965731542871 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -520,12 +520,12 @@ __lookup_addr_by_id(struct pm_nl_pernet
> *pernet, unsigned int id)
>  }
>  
>  static struct mptcp_pm_addr_entry *
> -__lookup_addr(struct pm_nl_pernet *pernet, const struct
> mptcp_addr_info *info)
> +__lookup_addr_by_id_rcu(struct pm_nl_pernet *pernet, unsigned int
> id)
>  {
>  	struct mptcp_pm_addr_entry *entry;
>  
> -	list_for_each_entry(entry, &pernet->local_addr_list, list) {
> -		if (mptcp_addresses_equal(&entry->addr, info, entry-
> >addr.port))
> +	list_for_each_entry_rcu(entry, &pernet->local_addr_list,
> list) {
> +		if (entry->addr.id == id)
>  			return entry;
>  	}
>  	return NULL;
> @@ -1836,8 +1836,8 @@ int mptcp_pm_nl_get_addr(struct sk_buff *skb,
> struct genl_info *info)
>  		goto fail;
>  	}
>  
> -	spin_lock_bh(&pernet->lock);
> -	entry = __lookup_addr_by_id(pernet, addr.addr.id);
> +	rcu_read_lock();
> +	entry = __lookup_addr_by_id_rcu(pernet, addr.addr.id);
>  	if (!entry) {
>  		GENL_SET_ERR_MSG(info, "address not found");
>  		ret = -EINVAL;
> @@ -1850,11 +1850,11 @@ int mptcp_pm_nl_get_addr(struct sk_buff *skb,
> struct genl_info *info)
>  
>  	genlmsg_end(msg, reply);
>  	ret = genlmsg_reply(msg, info);
> -	spin_unlock_bh(&pernet->lock);
> +	rcu_read_unlock();
>  	return ret;
>  
>  unlock_fail:
> -	spin_unlock_bh(&pernet->lock);
> +	rcu_read_unlock();
>  
>  fail:
>  	nlmsg_free(msg);
> @@ -1872,16 +1872,18 @@ int mptcp_pm_nl_dump_addr(struct sk_buff
> *msg,
>  	struct net *net = sock_net(msg->sk);
>  	struct mptcp_pm_addr_entry *entry;
>  	struct pm_nl_pernet *pernet;
> +	unsigned long id_bitmap[4];
>  	int id = cb->args[0];
>  	void *hdr;
>  	int i;
>  
>  	pernet = pm_nl_get_pernet(net);
> +	bitmap_copy(id_bitmap, pernet->id_bitmap,
> MPTCP_PM_MAX_ADDR_ID + 1);

I think the id bitmap should only be copied when id is 0:

if (!id)
  bitmap_copy(id_bitmap, pernet->id_bitmap, MPTCP_PM_MAX_ADDR_ID + 1);

Since this mptcp_pm_nl_dump_addr() may be called repeatedly when the
dump information is very long. We only copy it the first time it is
called, and subsequent calls cannot copy it again. WDYT?

Thanks,
-Geliang

>  
> -	spin_lock_bh(&pernet->lock);
> +	rcu_read_lock();
>  	for (i = id; i < MPTCP_PM_MAX_ADDR_ID + 1; i++) {
> -		if (test_bit(i, pernet->id_bitmap)) {
> -			entry = __lookup_addr_by_id(pernet, i);
> +		if (test_bit(i, id_bitmap)) {
> +			entry = __lookup_addr_by_id_rcu(pernet, i);
>  			if (!entry)
>  				break;
>  
> @@ -1903,7 +1905,7 @@ int mptcp_pm_nl_dump_addr(struct sk_buff *msg,
>  			genlmsg_end(msg, hdr);
>  		}
>  	}
> -	spin_unlock_bh(&pernet->lock);
> +	rcu_read_unlock();
>  
>  	cb->args[0] = id;
>  	return msg->len;
> @@ -2060,17 +2062,17 @@ int mptcp_pm_nl_set_flags(struct sk_buff
> *skb, struct genl_info *info)
>  	if (addr.flags & MPTCP_PM_ADDR_FLAG_BACKUP)
>  		bkup = 1;
>  
> -	spin_lock_bh(&pernet->lock);
> -	entry = lookup_by_id ? __lookup_addr_by_id(pernet,
> addr.addr.id) :
> -			       __lookup_addr(pernet, &addr.addr);
> +	rcu_read_lock();
> +	entry = lookup_by_id ? __lookup_addr_by_id_rcu(pernet,
> addr.addr.id) :
> +			       __lookup_addr_rcu(pernet,
> &addr.addr);
>  	if (!entry) {
> -		spin_unlock_bh(&pernet->lock);
> +		rcu_read_unlock();
>  		GENL_SET_ERR_MSG(info, "address not found");
>  		return -EINVAL;
>  	}
>  	if ((addr.flags & MPTCP_PM_ADDR_FLAG_FULLMESH) &&
>  	    (entry->flags & MPTCP_PM_ADDR_FLAG_SIGNAL)) {
> -		spin_unlock_bh(&pernet->lock);
> +		rcu_read_unlock();
>  		GENL_SET_ERR_MSG(info, "invalid addr flags");
>  		return -EINVAL;
>  	}
> @@ -2078,7 +2080,7 @@ int mptcp_pm_nl_set_flags(struct sk_buff *skb,
> struct genl_info *info)
>  	changed = (addr.flags ^ entry->flags) & mask;
>  	entry->flags = (entry->flags & ~mask) | (addr.flags & mask);
>  	addr = *entry;
> -	spin_unlock_bh(&pernet->lock);
> +	rcu_read_unlock();
>  
>  	mptcp_nl_set_flags(net, &addr.addr, bkup, changed);
>  	return 0;
>