diff mbox series

[v5,mptcp-next,1/5] mptcp: remote addresses fullmesh

Message ID b8af3a77da2fcd8e16e8b5d6b63c7c8386abcbac.1627372396.git.geliangtang@xiaomi.com (mailing list archive)
State Superseded, archived
Delegated to: Mat Martineau
Headers show
Series fullmesh path manager support | expand

Commit Message

Geliang Tang July 27, 2021, 7:58 a.m. UTC 
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(-)

Comments

Paolo Abeni July 27, 2021, 9:38 a.m. UTC | #1 
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
Paolo Abeni July 27, 2021, 9:40 a.m. UTC | #2 
On Tue, 2021-07-27 at 15:58 +0800, Geliang Tang wrote:
> +static unsigned int fill_remote_addresses_vec(struct mptcp_sock *msk,
> +					      struct mptcp_pm_addr_entry *local,
> +					      struct mptcp_addr_info *addrs)
> +{

I almost forgot another minor nit: some comments before the function
and/or in the main loop describing the function goal/behavior will make
the code more maintainable in the long term, thanks!

Paolo
Paolo Abeni July 27, 2021, 9:51 a.m. UTC | #3 
Hello,


I'm sorry for the partial feedback so far, I noticed more things
looking at the following patches. Again, I'm sorry!

On Tue, 2021-07-27 at 15:58 +0800, Geliang Tang wrote:
> +static bool lookup_subflow_by_addrs(const struct list_head *list,
> +				    struct mptcp_addr_info *saddr,
> +				    struct mptcp_addr_info *daddr)
> +{

both 'saddr' and 'daddr' could be 'const', I think.

> +	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;
> +}
> +
>  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)

'addr' could be 'const', I think.

> +{
> +	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)
> +{

'local' could be 'const', I think.

hopefully no more comments on this patch ;)

Cheers,

Paolo
Geliang Tang July 27, 2021, 12:36 p.m. UTC | #4 
Paolo Abeni <pabeni@redhat.com> 于2021年7月27日周二 下午5:38写道：
>
> 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.

I'll drop lookup_subflow_by_addrs() in v6.

>
> > +
> >  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

This id filed of 'remote' need to initialize, otherwise it will be a random
number.

>
> Cheers,
>
> Paolo
>
Geliang Tang July 27, 2021, 12:40 p.m. UTC | #5 
Paolo Abeni <pabeni@redhat.com> 于2021年7月27日周二 下午5:51写道：
>
> Hello,
>
>
> I'm sorry for the partial feedback so far, I noticed more things
> looking at the following patches. Again, I'm sorry!
>
> On Tue, 2021-07-27 at 15:58 +0800, Geliang Tang wrote:
> > +static bool lookup_subflow_by_addrs(const struct list_head *list,
> > +                                 struct mptcp_addr_info *saddr,
> > +                                 struct mptcp_addr_info *daddr)
> > +{
>
> both 'saddr' and 'daddr' could be 'const', I think.
>
> > +     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;
> > +}
> > +
> >  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)
>
> 'addr' could be 'const', I think.

It can be 'const', since the 2nd argument of addresses_equal is non-const.

And all the other 'const's in this comment too.

>
> > +{
> > +     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)
> > +{
>
> 'local' could be 'const', I think.
>
> hopefully no more comments on this patch ;)
>
> Cheers,
>
> Paolo
>
diff mbox series

Patch

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;
+}
+
 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 };
+	struct pm_nl_pernet *pernet;
+	unsigned int subflows_max;
+	int i = 0;
+
+	pernet = net_generic(sock_net(sk), pm_nl_pernet_id);
+	subflows_max = mptcp_pm_get_subflows_max(msk);
+
+	if (!(local->flags & MPTCP_PM_ADDR_FLAG_FULLMESH)) {
+		remote_address((struct sock_common *)sk, &remote);
+		msk->pm.subflows++;
+		addrs[i++] = remote;
+	} else {
+		mptcp_for_each_subflow(msk, subflow) {
+			ssk = mptcp_subflow_tcp_sock(subflow);
+			remote_address((struct sock_common *)ssk, &remote);
+			if (!lookup_subflow_by_addrs(&msk->conn_list, &local->addr, &remote) &&
+			    !lookup_address_in_vec(addrs, i, &remote) &&
+			    msk->pm.subflows < subflows_max) {
+				msk->pm.subflows++;
+				addrs[i++] = remote;
+			}
+		}
+	}
+
+	return i;
+}
+
 static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 {
 	struct sock *sk = (struct sock *)msk;
@@ -455,15 +523,17 @@  static void mptcp_pm_create_subflow_or_signal_addr(struct mptcp_sock *msk)
 	    !READ_ONCE(msk->pm.remote_deny_join_id0)) {
 		local = select_local_address(pernet, msk);
 		if (local) {
-			struct mptcp_addr_info remote = { 0 };
+			struct mptcp_addr_info addrs[MPTCP_PM_ADDR_MAX];
+			int i, nr;
 
 			msk->pm.local_addr_used++;
-			msk->pm.subflows++;
 			check_work_pending(msk);
-			remote_address((struct sock_common *)sk, &remote);
+			nr = fill_remote_addresses_vec(msk, local, addrs);
 			spin_unlock_bh(&msk->pm.lock);
-			__mptcp_subflow_connect(sk, &local->addr, &remote,
-						local->flags, local->ifindex);
+			for (i = 0; i < nr; i++) {
+				__mptcp_subflow_connect(sk, &local->addr, &addrs[i],
+							local->flags, local->ifindex);
+			}
 			spin_lock_bh(&msk->pm.lock);
 			return;
 		}