Re: [PATCH mptcp-next v2 27/36] mptcp: rename mptcp_pm_remove_addrs

[Matthieu Baerts <matttbe@kernel.org>]

Hi Geliang,

On 22/10/2024 11:14, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> mptcp_pm_remove_addrs() actually only deletes one address, which does
> not match its name. This patch renames it to mptcp_pm_remove_addr_entry()
> and changes the parameter "rm_list" to "entry".
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_netlink.c   | 24 ++++++++++--------------
>  net/mptcp/pm_userspace.c |  2 +-
>  net/mptcp/protocol.h     |  3 ++-
>  3 files changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index 06f0c7df575e..852706e43f13 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -1615,26 +1615,22 @@ int mptcp_pm_nl_del_addr_doit(struct sk_buff *skb, struct genl_info *info)
>  }
>  
>  /* Called from the userspace PM only */

(out of curiosity: after the modifications you did, could this helper
not be "easily" moved to pm_userspace.c?)

> -void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list)
> +void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
> +				struct mptcp_pm_addr_entry *entry)
>  {
>  	struct mptcp_rm_list alist = { .nr = 0 };
> -	struct mptcp_pm_addr_entry *entry;
>  	int anno_nr = 0;
>  
> -	list_for_each_entry(entry, rm_list, list) {
> -		if (alist.nr >= MPTCP_RM_IDS_MAX)
> -			break;
> -
> -		/* only delete if either announced or matching a subflow */
> -		if (remove_anno_list_by_saddr(msk, &entry->addr))
> -			anno_nr++;
> -		else if (!lookup_subflow_by_saddr(&msk->conn_list,
> -						  &entry->addr))
> -			continue;
> +	/* only delete if either announced or matching a subflow */
> +	if (remove_anno_list_by_saddr(msk, &entry->addr))
> +		anno_nr++;
> +	else if (!lookup_subflow_by_saddr(&msk->conn_list,
> +					  &entry->addr))

(maybe this can move to the previous line if it is under 80 chars?)

> +		goto out;
>  
> -		alist.ids[alist.nr++] = entry->addr.id;
> -	}
> +	alist.ids[alist.nr++] = entry->addr.id;
>  
> +out:
>  	if (alist.nr) {
>  		spin_lock_bh(&msk->pm.lock);
>  		msk->pm.add_addr_signaled -= anno_nr;
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 3ad10d8062d9..21b2a3b02cfe 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -322,7 +322,7 @@ int mptcp_pm_nl_remove_doit(struct sk_buff *skb, struct genl_info *info)
>  	list_move(&match->list, &free_list);

Do you still need to do this move? Do you still need "free_list"? Can
you just remove the entry from the list?

>  	spin_unlock_bh(&msk->pm.lock);
>  
> -	mptcp_pm_remove_addrs(msk, &free_list);
> +	mptcp_pm_remove_addr_entry(msk, match);
>  
>  	release_sock(sk);
>  
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 9dc01140af23..13cd97f9e0f7 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -1051,7 +1051,8 @@ int mptcp_pm_announce_addr(struct mptcp_sock *msk,
>  			   const struct mptcp_addr_info *addr,
>  			   bool echo);
>  int mptcp_pm_remove_addr(struct mptcp_sock *msk, const struct mptcp_rm_list *rm_list);
> -void mptcp_pm_remove_addrs(struct mptcp_sock *msk, struct list_head *rm_list);
> +void mptcp_pm_remove_addr_entry(struct mptcp_sock *msk,
> +				struct mptcp_pm_addr_entry *entry);
>  
>  void mptcp_free_local_addr_list(struct mptcp_sock *msk);
>  

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.