Re: [MPTCP][PATCH v5 mptcp-next 1/5] mptcp: remote addresses fullmesh

[Paolo Abeni <pabeni@redhat.com>]

Hello,

On Tue, 2021-07-27 at 15:58 +0800, Geliang Tang wrote:
> From: Geliang Tang <geliangtang@xiaomi.com>
> 
> This patch added and managed a new per endpoint flag, named
> MPTCP_PM_ADDR_FLAG_FULLMESH.
> 
> In mptcp_pm_create_subflow_or_signal_addr(), if such flag is set, instead
> of:
> 
>         remote_address((struct sock_common *)sk, &remote);
> 
> fill a temporary allocated array of all known remote address. After
> releaseing the pm lock loop on such array and create a subflow for each
> remote address from the given local.
> 
> Note that the we could still use an array even for non 'fullmesh'
> endpoint: with a single entry corresponding to the primary MPC subflow
> remote address.
> 
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
> ---
>  include/uapi/linux/mptcp.h |  1 +
>  net/mptcp/pm_netlink.c     | 80 +++++++++++++++++++++++++++++++++++---
>  2 files changed, 76 insertions(+), 5 deletions(-)
> 
> diff --git a/include/uapi/linux/mptcp.h b/include/uapi/linux/mptcp.h
> index 7b05f7102321..f66038b9551f 100644
> --- a/include/uapi/linux/mptcp.h
> +++ b/include/uapi/linux/mptcp.h
> @@ -73,6 +73,7 @@ enum {
>  #define MPTCP_PM_ADDR_FLAG_SIGNAL			(1 << 0)
>  #define MPTCP_PM_ADDR_FLAG_SUBFLOW			(1 << 1)
>  #define MPTCP_PM_ADDR_FLAG_BACKUP			(1 << 2)
> +#define MPTCP_PM_ADDR_FLAG_FULLMESH			(1 << 3)
>  
>  enum {
>  	MPTCP_PM_CMD_UNSPEC,
> diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c
> index ba0e1d71504d..2259c424485f 100644
> --- a/net/mptcp/pm_netlink.c
> +++ b/net/mptcp/pm_netlink.c
> @@ -158,6 +158,27 @@ static bool lookup_subflow_by_daddr(const struct list_head *list,
>  	return false;
>  }
>  
> +static bool lookup_subflow_by_addrs(const struct list_head *list,
> +				    struct mptcp_addr_info *saddr,
> +				    struct mptcp_addr_info *daddr)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct mptcp_addr_info local, remote;
> +	struct sock_common *skc;
> +
> +	list_for_each_entry(subflow, list, node) {
> +		skc = (struct sock_common *)mptcp_subflow_tcp_sock(subflow);
> +
> +		local_address(skc, &local);
> +		remote_address(skc, &remote);
> +		if (addresses_equal(&local, saddr, saddr->port) &&
> +		    addresses_equal(&remote, daddr, daddr->port))
> +			return true;
> +	}
> +
> +	return false;
> +}

I'm sorry for not noticing this earlier, do we need this function and
the related check in fill_remote_addresses_vec()?

'saddr' is the return value of select_local_address(), so existing
subflows is bound to such address. 

> +
>  static struct mptcp_pm_addr_entry *
>  select_local_address(const struct pm_nl_pernet *pernet,
>  		     struct mptcp_sock *msk)
> @@ -410,6 +431,53 @@ void mptcp_pm_free_anno_list(struct mptcp_sock *msk)
>  	}
>  }
>  
> +static bool lookup_address_in_vec(struct mptcp_addr_info *addrs, unsigned int nr,
> +				  struct mptcp_addr_info *addr)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr; i++) {
> +		if (addresses_equal(&addrs[i], addr, addr->port))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk,
> +					      struct mptcp_pm_addr_entry *local,
> +					      struct mptcp_addr_info *addrs)
> +{
> +	struct sock *sk = (struct sock *)msk, *ssk;
> +	struct mptcp_subflow_context *subflow;
> +	struct mptcp_addr_info remote = { 0 };

Minor nit: no need to initialize the 'remote' variable

Cheers,

Paolo