[MPTCP] Re: [MPTCP][PATCH mptcp-next 1/8] mptcp: remove multi addrs on outgoing path

[MPTCP] Re: [MPTCP][PATCH mptcp-next 1/8] mptcp: remove multi addrs on outgoing path

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 6305 bytes --]

On Fri, 29 Jan 2021, Geliang Tang wrote:

> This patch changed the type of rm_id in struct mptcp_out_options from u8
> to u64, and renamed it to rm_ids. It was used as a map of address ids
> that need to be removed. Up to 8 address ids could be encoded in it.
>
> Added a new macro named mptcp_for_each_id to iterate out each address
> id form the ids map.
>
> In mptcp_established_options_rm_addr, invoked mptcp_pm_rm_addr_signal to
> get the ids map. According the number of addresses in the ids map,
> calculated the padded RM_ADDR suboption length. And saved the ids map in
> struct mptcp_out_options's rm_ids member.
>
> In mptcp_write_options, used mptcp_for_each_id to iterate out each
> address id, then filled them into the RM_ADDR suboption.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> include/net/mptcp.h  |  2 +-
> net/mptcp/options.c  | 40 ++++++++++++++++++++++++++++++++--------
> net/mptcp/pm.c       |  4 ++--
> net/mptcp/protocol.h |  9 +++++++--
> 4 files changed, 42 insertions(+), 13 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 5694370be3d4..21232c4e15fe 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -48,7 +48,7 @@ struct mptcp_out_options {
> 	u8 addr_id;
> 	u16 port;
> 	u64 ahmac;
> -	u8 rm_id;
> +	u64 rm_ids;

It looks like this u64 is used as an array of u8's, so it would be better 
to use an array instead of manually doing all the pointer manipulation.

Looking at the RFC, there doesn't seem to be a limit on the number of 
address ids other than the maximum TCP option size. Did you choose 8 
address ids to mach MPTCP_PM_ADDR_MAX?

> 	u8 join_id;
> 	u8 backup;
> 	u32 nonce;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 775f0576592e..11f6182b8319 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -671,20 +671,29 @@ static bool mptcp_established_options_rm_addr(struct sock *sk,
> {
> 	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> 	struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> -	u8 rm_id;
> +	u8 *ptr, id, nr;
> +	u64 rm_ids;
>
> 	if (!mptcp_pm_should_rm_signal(msk) ||
> -	    !(mptcp_pm_rm_addr_signal(msk, remaining, &rm_id)))
> +	    !(mptcp_pm_rm_addr_signal(msk, remaining, &rm_ids)))
> 		return false;
>
> -	if (remaining < TCPOLEN_MPTCP_RM_ADDR_BASE)
> +	mptcp_for_each_id(rm_ids, nr, ptr, id)
> +		;
> +
> +	if (nr > 1)
> +		nr = 5;
> +	if (nr > 5)
> +		nr = 9;
> +
> +	if (remaining < TCPOLEN_MPTCP_RM_ADDR_BASE + nr)
> 		return false;
>
> -	*size = TCPOLEN_MPTCP_RM_ADDR_BASE;
> +	*size = TCPOLEN_MPTCP_RM_ADDR_BASE + nr;
> 	opts->suboptions |= OPTION_MPTCP_RM_ADDR;
> -	opts->rm_id = rm_id;
> +	opts->rm_ids = rm_ids;
>
> -	pr_debug("rm_id=%d", opts->rm_id);
> +	pr_debug("rm_ids=%llu", opts->rm_ids);
>
> 	return true;
> }
> @@ -1211,9 +1220,24 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 	}
>
> 	if (OPTION_MPTCP_RM_ADDR & opts->suboptions) {
> +		u8 rm_ids[8] = { 0 };
> +		u8 *tmp, id, nr;
> +
> +		mptcp_for_each_id(opts->rm_ids, nr, tmp, id)
> +			rm_ids[nr] = id;
> 		*ptr++ = mptcp_option(MPTCPOPT_RM_ADDR,
> -				      TCPOLEN_MPTCP_RM_ADDR_BASE,
> -				      0, opts->rm_id);
> +				      TCPOLEN_MPTCP_RM_ADDR_BASE + nr,
> +				      0, rm_ids[0]);
> +		if (nr > 1) {
> +			put_unaligned_be32(rm_ids[1] << 24 | rm_ids[2] << 16 |
> +					   rm_ids[3] << 8 | rm_ids[4], ptr);
> +			ptr += 1;

All of the unused bytes would have to be set to TCPOPT_NOP here...

> +		}
> +		if (nr > 5) {
> +			put_unaligned_be32(rm_ids[5] << 24 | rm_ids[6] << 16 |
> +					   rm_ids[7] << 8 | TCPOPT_NOP, ptr);
> +			ptr += 1;

...and here.

> +		}
> 	}
>
> 	if (OPTION_MPTCP_PRIO & opts->suboptions) {
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 6fd4b2c1b076..7ec1d2a1582b 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -258,7 +258,7 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> }
>
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> -			     u8 *rm_id)
> +			     u64 *rm_ids)
> {
> 	int ret = false;
>
> @@ -271,7 +271,7 @@ bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> 	if (remaining < TCPOLEN_MPTCP_RM_ADDR_BASE)
> 		goto out_unlock;
>
> -	*rm_id = msk->pm.rm_id;
> +	*rm_ids = msk->pm.rm_id;
> 	WRITE_ONCE(msk->pm.addr_signal, 0);
> 	ret = true;
>
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 447ce4631b43..962cc1b4dd48 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -60,7 +60,7 @@
> #define TCPOLEN_MPTCP_ADD_ADDR6_BASE	20
> #define TCPOLEN_MPTCP_ADD_ADDR6_BASE_PORT	24
> #define TCPOLEN_MPTCP_PORT_LEN		4
> -#define TCPOLEN_MPTCP_RM_ADDR_BASE	4
> +#define TCPOLEN_MPTCP_RM_ADDR_BASE	3
> #define TCPOLEN_MPTCP_PRIO		4
> #define TCPOLEN_MPTCP_FASTCLOSE		12
>
> @@ -288,6 +288,11 @@ struct mptcp_sock {
> #define mptcp_for_each_subflow(__msk, __subflow)			\
> 	list_for_each_entry(__subflow, &((__msk)->conn_list), node)
>
> +#define mptcp_for_each_id(ids, nr, ptr, id)				\
> +	for ((nr) = 0, (ptr) = (u8 *)&(ids);				\
> +	     (nr) < 8 && (id = *(ptr));					\
> +	     (nr)++, (ptr)++)
> +

If rm_ids is an array instead, it would be simpler to just memcpy the 
array as needed and count the non-zero entries in the array.

> static inline void msk_owned_by_me(const struct mptcp_sock *msk)
> {
> 	sock_owned_by_me((const struct sock *)msk);
> @@ -713,7 +718,7 @@ static inline unsigned int mptcp_add_addr_len(int family, bool echo, bool port)
> bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> 			      struct mptcp_addr_info *saddr, bool *echo, bool *port);
> bool mptcp_pm_rm_addr_signal(struct mptcp_sock *msk, unsigned int remaining,
> -			     u8 *rm_id);
> +			     u64 *rm_ids);
> int mptcp_pm_get_local_id(struct mptcp_sock *msk, struct sock_common *skc);
>
> void __init mptcp_pm_nl_init(void);
> -- 
> 2.29.2
> _______________________________________________
> mptcp mailing list -- mptcp(a)lists.01.org
> To unsubscribe send an email to mptcp-leave(a)lists.01.org
>

--
Mat Martineau
Intel