[MPTCP] Re: [MPTCP][PATCH v3 mptcp-next 1/9] mptcp: use rm_ids array in mptcp_out_options

[Mat Martineau <mathew.j.martineau at linux.intel.com>]

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

On Tue, 2 Feb 2021, Geliang Tang wrote:

> This patch changed the member rm_id of struct mptcp_out_options as an
> array of the removing address ids, and renamed it to rm_ids. The array
> size was definced as a new macro MPTCP_RM_IDS_MAX.
>
> Added a new function named mptcp_get_rm_ids_nr to get the number of
> address ids in the ids array.
>
> In mptcp_established_options_rm_addr, invoked mptcp_pm_rm_addr_signal to
> get the ids array. According the number of addresses in it, calculated
> the padded RM_ADDR suboption length. And saved the ids array in struct
> mptcp_out_options's rm_ids member.
>
> In mptcp_write_options, iterated each address id from struct
> mptcp_out_options's rm_ids member, set the zero ones as TCPOPT_NOP,
> then filled them into the RM_ADDR suboption.
>
> Signed-off-by: Geliang Tang <geliangtang(a)gmail.com>
> ---
> include/net/mptcp.h  |  4 +++-
> net/mptcp/options.c  | 41 +++++++++++++++++++++++++++++++++--------
> net/mptcp/pm.c       |  4 ++--
> net/mptcp/protocol.h | 14 ++++++++++++--
> 4 files changed, 50 insertions(+), 13 deletions(-)
>
> diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> index 5694370be3d4..1d33fea674d2 100644
> --- a/include/net/mptcp.h
> +++ b/include/net/mptcp.h
> @@ -34,6 +34,8 @@ struct mptcp_ext {
> 	/* one byte hole */
> };
>
> +#define MPTCP_RM_IDS_MAX	8
> +
> struct mptcp_out_options {
> #if IS_ENABLED(CONFIG_MPTCP)
> 	u16 suboptions;
> @@ -48,7 +50,7 @@ struct mptcp_out_options {
> 	u8 addr_id;
> 	u16 port;
> 	u64 ahmac;
> -	u8 rm_id;
> +	u8 rm_ids[MPTCP_RM_IDS_MAX];
> 	u8 join_id;
> 	u8 backup;
> 	u32 nonce;
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index bb874c5d663a..a6a4fdf03d6c 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -671,20 +671,27 @@ 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 rm_ids[MPTCP_RM_IDS_MAX], i, nr, align;
>
> 	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)
> +	nr = mptcp_get_rm_ids_nr(rm_ids);
> +	if (nr > 1)
> +		align = 5;
> +	if (nr > 5)
> +		align = 9;
> +
> +	if (remaining < TCPOLEN_MPTCP_RM_ADDR_BASE + align)
> 		return false;
>
> -	*size = TCPOLEN_MPTCP_RM_ADDR_BASE;
> +	*size = TCPOLEN_MPTCP_RM_ADDR_BASE + align;
> 	opts->suboptions |= OPTION_MPTCP_RM_ADDR;
> -	opts->rm_id = rm_id;
> +	memcpy(opts->rm_ids, rm_ids, MPTCP_RM_IDS_MAX);
>
> -	pr_debug("rm_id=%d", opts->rm_id);
> +	for (i = 0; i < nr; i++)
> +		pr_debug("rm_ids[%d]=%d", i, opts->rm_ids[i]);
>
> 	return true;
> }
> @@ -1212,9 +1219,27 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> 	}
>
> 	if (OPTION_MPTCP_RM_ADDR & opts->suboptions) {
> +		u8 i, nr = 0;
> +
> +		for (i = 0; i < MPTCP_RM_IDS_MAX; i++) {
> +			if (opts->rm_ids[i])
> +				nr++;
> +			else
> +				opts->rm_ids[i] = TCPOPT_NOP;
> +		}
> 		*ptr++ = mptcp_option(MPTCPOPT_RM_ADDR,
> -				      TCPOLEN_MPTCP_RM_ADDR_BASE,
> -				      0, opts->rm_id);
> +				      TCPOLEN_MPTCP_RM_ADDR_BASE + nr,
> +				      0, opts->rm_ids[0]);
> +		if (nr > 1) {
> +			put_unaligned_be32(opts->rm_ids[1] << 24 | opts->rm_ids[2] << 16 |
> +					   opts->rm_ids[3] << 8 | opts->rm_ids[4], ptr);
> +			ptr += 1;
> +		}
> +		if (nr > 5) {
> +			put_unaligned_be32(opts->rm_ids[5] << 24 | opts->rm_ids[6] << 16 |
> +					   opts->rm_ids[7] << 8 | TCPOPT_NOP, ptr);
> +			ptr += 1;
> +		}
> 	}
>
> 	if (OPTION_MPTCP_PRIO & opts->suboptions) {
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 6fd4b2c1b076..59e0fee52afa 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)
> +			     u8 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[0] = 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 d31edbae8da8..0c00e529f127 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		3
> #define TCPOLEN_MPTCP_PRIO_ALIGN	4
> #define TCPOLEN_MPTCP_FASTCLOSE		12
> @@ -289,6 +289,16 @@ struct mptcp_sock {
> #define mptcp_for_each_subflow(__msk, __subflow)			\
> 	list_for_each_entry(__subflow, &((__msk)->conn_list), node)
>
> +static inline u8 mptcp_get_rm_ids_nr(u8 rm_ids[])
> +{
> +	int i;
> +
> +	for (i = 0; i < MPTCP_RM_IDS_MAX && rm_ids[i]; i++)
> +		;

Semicolon should be at the end of the previous line, rather than on its 
own line: "for (...);"

More importantly, I asked about the use of subflow id 0 in RM_ADDR in the 
community meeting today, and subflow 0 is valid ID to use because the 
initial subflow's interface may be removed. Since all 8-bit values are 
valid subflow IDs, using '0' to mark the end of valid IDs in the array 
won't work. It will be necessary to add something to explicitly store the 
number of valid rm_ids throughout this patch set.


Mat

> +
> +	return i;
> +}
> +
> static inline void msk_owned_by_me(const struct mptcp_sock *msk)
> {
> 	sock_owned_by_me((const struct sock *)msk);
> @@ -714,7 +724,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);
> +			     u8 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

--
Mat Martineau
Intel