* [PATCH mptcp-next 0/2] mptcp: minor out optimization
@ 2021-07-23 17:06 Paolo Abeni
2021-07-23 17:06 ` [PATCH mptcp-next 1/2] mptcp: optimize out option generation Paolo Abeni
2021-07-23 17:06 ` [PATCH mptcp-next 2/2] mptcp: shrink mptcp_out_options struct Paolo Abeni
0 siblings, 2 replies; 7+ messages in thread
From: Paolo Abeni @ 2021-07-23 17:06 UTC (permalink / raw)
To: mptcp
This series implements a few ideas discussed in the past mtg,
specifcally:
https://github.com/multipath-tcp/mptcp_net-next/issues/15
https://github.com/multipath-tcp/mptcp_net-next/issues/42
The final goal is shrink the mptcp_out_options struct, which is
always zeroed by the plain TCP protocol in the output path.
Only lightly tested.
Paolo Abeni (2):
mptcp: optimize out option generation
mptcp: shrink mptcp_out_options struct
include/net/mptcp.h | 26 +++--
net/mptcp/options.c | 225 ++++++++++++++++++++++---------------------
net/mptcp/pm.c | 9 +-
net/mptcp/protocol.h | 1 +
4 files changed, 139 insertions(+), 122 deletions(-)
--
2.26.3
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH mptcp-next 1/2] mptcp: optimize out option generation
2021-07-23 17:06 [PATCH mptcp-next 0/2] mptcp: minor out optimization Paolo Abeni
@ 2021-07-23 17:06 ` Paolo Abeni
2021-07-26 3:21 ` Geliang Tang
2021-07-23 17:06 ` [PATCH mptcp-next 2/2] mptcp: shrink mptcp_out_options struct Paolo Abeni
1 sibling, 1 reply; 7+ messages in thread
From: Paolo Abeni @ 2021-07-23 17:06 UTC (permalink / raw)
To: mptcp
Currently we have several protocol contraint on MPTCP option
generation (e.g. MPC and MPJ subopt are mutually exclusive)
and some additionall ones required by our implementation
(e.g. almost all ADD_ADDR variant are mutually exclusive with
everything else).
We can leverage the above to optimize the out option generation:
we check DSS/MPC/MPJ presencence in a mutually exclusive way,
avoiding many unneeded conditionals in the common case.
Additionally extend the existing constraint on ADD_ADDR opt on
all subvariant, so that it become fully mutually exclusive with
the above and we can skip another conditional statement the common
case.
This change is also need by the next patch.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
--
side node: we could probably optimize the conditionals in
mptcp_established_options()
---
net/mptcp/options.c | 225 ++++++++++++++++++++++---------------------
net/mptcp/pm.c | 9 +-
net/mptcp/protocol.h | 1 +
3 files changed, 122 insertions(+), 113 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index eafdb9408f3a..38f324667761 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -592,6 +592,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
dss_size = map_size;
if (skb && snd_data_fin_enable)
mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
+ opts->suboptions = OPTION_MPTCP_DSS;
ret = true;
}
@@ -615,6 +616,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
opts->ext_copy.ack64 = 0;
}
opts->ext_copy.use_ack = 1;
+ opts->suboptions = OPTION_MPTCP_DSS;
WRITE_ONCE(msk->old_wspace, __mptcp_space((struct sock *)msk));
/* Add kind/length/subtype/flag overhead if mapping is not populated */
@@ -663,9 +665,9 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
struct mptcp_sock *msk = mptcp_sk(subflow->conn);
bool drop_other_suboptions = false;
unsigned int opt_size = *size;
+ unsigned int len;
bool echo;
bool port;
- int len;
if (!mptcp_pm_should_add_signal(msk) ||
!mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts,
@@ -687,11 +689,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
*size -= opt_size;
}
opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
- if (!echo) {
+ if (!echo)
opts->ahmac = add_addr_generate_hmac(msk->local_key,
msk->remote_key,
&opts->addr);
- }
+ else
+ opts->ahmac = 0;
pr_debug("addr_id=%d, addr_port=%d, ahmac=%llu, echo=%d",
opts->addr.id, ntohs(opts->addr.port), opts->ahmac, echo);
@@ -735,7 +738,12 @@ static bool mptcp_established_options_mp_prio(struct sock *sk,
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
- if (!subflow->send_mp_prio)
+ /* can't send MP_PRIO with MPC, as they share the same option space:
+ * 'backup'. Also it makes no sense at all
+ */
+ if (!subflow->send_mp_prio ||
+ ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
+ OPTION_MPTCP_MPC_ACK) & opts->suboptions))
return false;
/* account for the trailing 'nop' option */
@@ -1198,7 +1206,73 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
struct mptcp_out_options *opts)
{
- if ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
+ /* RST is mutually exclusive with everything else */
+ if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
+ *ptr++ = mptcp_option(MPTCPOPT_RST,
+ TCPOLEN_MPTCP_RST,
+ opts->reset_transient,
+ opts->reset_reason);
+ return;
+ }
+
+ /* DSS, MPC, MPJ and ADD_ADDR are mutually exclusive, see
+ * mptcp_established_options*()
+ */
+ if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
+ struct mptcp_ext *mpext = &opts->ext_copy;
+ u8 len = TCPOLEN_MPTCP_DSS_BASE;
+ u8 flags = 0;
+
+ if (mpext->use_ack) {
+ flags = MPTCP_DSS_HAS_ACK;
+ if (mpext->ack64) {
+ len += TCPOLEN_MPTCP_DSS_ACK64;
+ flags |= MPTCP_DSS_ACK64;
+ } else {
+ len += TCPOLEN_MPTCP_DSS_ACK32;
+ }
+ }
+
+ if (mpext->use_map) {
+ len += TCPOLEN_MPTCP_DSS_MAP64;
+
+ /* Use only 64-bit mapping flags for now, add
+ * support for optional 32-bit mappings later.
+ */
+ flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
+ if (mpext->data_fin)
+ flags |= MPTCP_DSS_DATA_FIN;
+
+ if (opts->csum_reqd)
+ len += TCPOLEN_MPTCP_DSS_CHECKSUM;
+ }
+
+ *ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);
+
+ if (mpext->use_ack) {
+ if (mpext->ack64) {
+ put_unaligned_be64(mpext->data_ack, ptr);
+ ptr += 2;
+ } else {
+ put_unaligned_be32(mpext->data_ack32, ptr);
+ ptr += 1;
+ }
+ }
+
+ if (mpext->use_map) {
+ put_unaligned_be64(mpext->data_seq, ptr);
+ ptr += 2;
+ put_unaligned_be32(mpext->subflow_seq, ptr);
+ ptr += 1;
+ if (opts->csum_reqd) {
+ put_unaligned_be32(mpext->data_len << 16 |
+ mptcp_make_csum(mpext), ptr);
+ } else {
+ put_unaligned_be32(mpext->data_len << 16 |
+ TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
+ }
+ }
+ } else if ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
OPTION_MPTCP_MPC_ACK) & opts->suboptions) {
u8 len, flag = MPTCP_CAP_HMAC_SHA256;
@@ -1246,10 +1320,31 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
}
ptr += 1;
- }
-mp_capable_done:
- if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
+ /* MPC is additionally mutually exclusive with MP_PRIO */
+ goto mp_capable_done;
+ } else if (OPTION_MPTCP_MPJ_SYN & opts->suboptions) {
+ *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
+ TCPOLEN_MPTCP_MPJ_SYN,
+ opts->backup, opts->join_id);
+ put_unaligned_be32(opts->token, ptr);
+ ptr += 1;
+ put_unaligned_be32(opts->nonce, ptr);
+ ptr += 1;
+ } else if (OPTION_MPTCP_MPJ_SYNACK & opts->suboptions) {
+ *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
+ TCPOLEN_MPTCP_MPJ_SYNACK,
+ opts->backup, opts->join_id);
+ put_unaligned_be64(opts->thmac, ptr);
+ ptr += 2;
+ put_unaligned_be32(opts->nonce, ptr);
+ ptr += 1;
+ } else if (OPTION_MPTCP_MPJ_ACK & opts->suboptions) {
+ *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
+ TCPOLEN_MPTCP_MPJ_ACK, 0, 0);
+ memcpy(ptr, opts->hmac, MPTCPOPT_HMAC_LEN);
+ ptr += 5;
+ } else if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
struct mptcp_addr_info *addr = &opts->addr;
u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
u8 echo = MPTCP_ADDR_ECHO;
@@ -1308,6 +1403,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
}
}
+ if (OPTION_MPTCP_PRIO & opts->suboptions) {
+ const struct sock *ssk = (const struct sock *)tp;
+ struct mptcp_subflow_context *subflow;
+
+ subflow = mptcp_subflow_ctx(ssk);
+ subflow->send_mp_prio = 0;
+
+ *ptr++ = mptcp_option(MPTCPOPT_MP_PRIO,
+ TCPOLEN_MPTCP_PRIO,
+ opts->backup, TCPOPT_NOP);
+ }
+
+mp_capable_done:
if (OPTION_MPTCP_RM_ADDR & opts->suboptions) {
u8 i = 1;
@@ -1328,107 +1436,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
}
}
- if (OPTION_MPTCP_PRIO & opts->suboptions) {
- const struct sock *ssk = (const struct sock *)tp;
- struct mptcp_subflow_context *subflow;
-
- subflow = mptcp_subflow_ctx(ssk);
- subflow->send_mp_prio = 0;
-
- *ptr++ = mptcp_option(MPTCPOPT_MP_PRIO,
- TCPOLEN_MPTCP_PRIO,
- opts->backup, TCPOPT_NOP);
- }
-
- if (OPTION_MPTCP_MPJ_SYN & opts->suboptions) {
- *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
- TCPOLEN_MPTCP_MPJ_SYN,
- opts->backup, opts->join_id);
- put_unaligned_be32(opts->token, ptr);
- ptr += 1;
- put_unaligned_be32(opts->nonce, ptr);
- ptr += 1;
- }
-
- if (OPTION_MPTCP_MPJ_SYNACK & opts->suboptions) {
- *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
- TCPOLEN_MPTCP_MPJ_SYNACK,
- opts->backup, opts->join_id);
- put_unaligned_be64(opts->thmac, ptr);
- ptr += 2;
- put_unaligned_be32(opts->nonce, ptr);
- ptr += 1;
- }
-
- if (OPTION_MPTCP_MPJ_ACK & opts->suboptions) {
- *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
- TCPOLEN_MPTCP_MPJ_ACK, 0, 0);
- memcpy(ptr, opts->hmac, MPTCPOPT_HMAC_LEN);
- ptr += 5;
- }
-
- if (OPTION_MPTCP_RST & opts->suboptions)
- *ptr++ = mptcp_option(MPTCPOPT_RST,
- TCPOLEN_MPTCP_RST,
- opts->reset_transient,
- opts->reset_reason);
-
- if (opts->ext_copy.use_ack || opts->ext_copy.use_map) {
- struct mptcp_ext *mpext = &opts->ext_copy;
- u8 len = TCPOLEN_MPTCP_DSS_BASE;
- u8 flags = 0;
-
- if (mpext->use_ack) {
- flags = MPTCP_DSS_HAS_ACK;
- if (mpext->ack64) {
- len += TCPOLEN_MPTCP_DSS_ACK64;
- flags |= MPTCP_DSS_ACK64;
- } else {
- len += TCPOLEN_MPTCP_DSS_ACK32;
- }
- }
-
- if (mpext->use_map) {
- len += TCPOLEN_MPTCP_DSS_MAP64;
-
- /* Use only 64-bit mapping flags for now, add
- * support for optional 32-bit mappings later.
- */
- flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
- if (mpext->data_fin)
- flags |= MPTCP_DSS_DATA_FIN;
-
- if (opts->csum_reqd)
- len += TCPOLEN_MPTCP_DSS_CHECKSUM;
- }
-
- *ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);
-
- if (mpext->use_ack) {
- if (mpext->ack64) {
- put_unaligned_be64(mpext->data_ack, ptr);
- ptr += 2;
- } else {
- put_unaligned_be32(mpext->data_ack32, ptr);
- ptr += 1;
- }
- }
-
- if (mpext->use_map) {
- put_unaligned_be64(mpext->data_seq, ptr);
- ptr += 2;
- put_unaligned_be32(mpext->subflow_seq, ptr);
- ptr += 1;
- if (opts->csum_reqd) {
- put_unaligned_be32(mpext->data_len << 16 |
- mptcp_make_csum(mpext), ptr);
- } else {
- put_unaligned_be32(mpext->data_len << 16 |
- TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
- }
- }
- }
-
if (tp)
mptcp_set_rwin(tp);
}
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 4d1828fd2482..baf2d6e524bd 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -266,10 +266,11 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
if (!mptcp_pm_should_add_signal(msk))
goto out_unlock;
- if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
- ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
- (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
- skb && skb_is_tcp_pure_ack(skb)) {
+ /* always drop every other options for pure ack ADD_ADDR; this is
+ * plain dup-ack from TCP perspective. The other MPTCP-relevant info,
+ * if any, will be carried by the other pure ack
+ */
+ if (skb && skb_is_tcp_pure_ack(skb)) {
remaining += opt_size;
*drop_other_suboptions = true;
}
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 34ad1c4bc29f..d960d8b8a8ce 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -26,6 +26,7 @@
#define OPTION_MPTCP_FASTCLOSE BIT(8)
#define OPTION_MPTCP_PRIO BIT(9)
#define OPTION_MPTCP_RST BIT(10)
+#define OPTION_MPTCP_DSS BIT(11)
/* MPTCP option subtypes */
#define MPTCPOPT_MP_CAPABLE 0
--
2.26.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH mptcp-next 2/2] mptcp: shrink mptcp_out_options struct
2021-07-23 17:06 [PATCH mptcp-next 0/2] mptcp: minor out optimization Paolo Abeni
2021-07-23 17:06 ` [PATCH mptcp-next 1/2] mptcp: optimize out option generation Paolo Abeni
@ 2021-07-23 17:06 ` Paolo Abeni
1 sibling, 0 replies; 7+ messages in thread
From: Paolo Abeni @ 2021-07-23 17:06 UTC (permalink / raw)
To: mptcp
After the previous patch we can alias with an union several
fields in mptcp_out_options. Such struct is stack allocated and
memset() for each plain TCP out packet. Every saved byted counts.
Before:
pahole -EC mptcp_out_options
# ...
/* size: 136, cachelines: 3, members: 17 */
After:
pahole -EC mptcp_out_options
# ...
/* size: 56, cachelines: 1, members: 9 */
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
include/net/mptcp.h | 26 +++++++++++++++++---------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/include/net/mptcp.h b/include/net/mptcp.h
index 8b5af683a818..3236010afa29 100644
--- a/include/net/mptcp.h
+++ b/include/net/mptcp.h
@@ -58,10 +58,6 @@ struct mptcp_addr_info {
struct mptcp_out_options {
#if IS_ENABLED(CONFIG_MPTCP)
u16 suboptions;
- u64 sndr_key;
- u64 rcvr_key;
- u64 ahmac;
- struct mptcp_addr_info addr;
struct mptcp_rm_list rm_list;
u8 join_id;
u8 backup;
@@ -69,11 +65,23 @@ struct mptcp_out_options {
reset_transient:1,
csum_reqd:1,
allow_join_id0:1;
- u32 nonce;
- u64 thmac;
- u32 token;
- u8 hmac[20];
- struct mptcp_ext ext_copy;
+ union {
+ struct {
+ u64 sndr_key;
+ u64 rcvr_key;
+ };
+ struct {
+ struct mptcp_addr_info addr;
+ u64 ahmac;
+ };
+ struct mptcp_ext ext_copy;
+ struct {
+ u32 nonce;
+ u32 token;
+ u64 thmac;
+ u8 hmac[20];
+ };
+ };
#endif
};
--
2.26.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-next 1/2] mptcp: optimize out option generation
2021-07-23 17:06 ` [PATCH mptcp-next 1/2] mptcp: optimize out option generation Paolo Abeni
@ 2021-07-26 3:21 ` Geliang Tang
2021-07-26 9:44 ` Paolo Abeni
2021-07-26 10:06 ` Paolo Abeni
0 siblings, 2 replies; 7+ messages in thread
From: Geliang Tang @ 2021-07-26 3:21 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Hi Paolo,
Thanks for this patchset, and I have some small comments.
Paolo Abeni <pabeni@redhat.com> 于2021年7月24日周六 上午1:06写道：
>
> Currently we have several protocol contraint on MPTCP option
> generation (e.g. MPC and MPJ subopt are mutually exclusive)
> and some additionall ones required by our implementation
> (e.g. almost all ADD_ADDR variant are mutually exclusive with
> everything else).
>
> We can leverage the above to optimize the out option generation:
> we check DSS/MPC/MPJ presencence in a mutually exclusive way,
> avoiding many unneeded conditionals in the common case.
>
> Additionally extend the existing constraint on ADD_ADDR opt on
> all subvariant, so that it become fully mutually exclusive with
> the above and we can skip another conditional statement the common
> case.
>
> This change is also need by the next patch.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> --
> side node: we could probably optimize the conditionals in
> mptcp_established_options()
> ---
> net/mptcp/options.c | 225 ++++++++++++++++++++++---------------------
> net/mptcp/pm.c | 9 +-
> net/mptcp/protocol.h | 1 +
> 3 files changed, 122 insertions(+), 113 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index eafdb9408f3a..38f324667761 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -592,6 +592,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> dss_size = map_size;
> if (skb && snd_data_fin_enable)
> mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
> + opts->suboptions = OPTION_MPTCP_DSS;
> ret = true;
> }
>
> @@ -615,6 +616,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> opts->ext_copy.ack64 = 0;
> }
> opts->ext_copy.use_ack = 1;
> + opts->suboptions = OPTION_MPTCP_DSS;
> WRITE_ONCE(msk->old_wspace, __mptcp_space((struct sock *)msk));
>
> /* Add kind/length/subtype/flag overhead if mapping is not populated */
> @@ -663,9 +665,9 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
I think no need to modify the code in mptcp_established_options_add_addr.
> struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> bool drop_other_suboptions = false;
> unsigned int opt_size = *size;
> + unsigned int len;
> bool echo;
> bool port;
> - int len;
I prefer to keep the type of len unchanged.
>
> if (!mptcp_pm_should_add_signal(msk) ||
> !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts,
> @@ -687,11 +689,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> *size -= opt_size;
> }
> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> - if (!echo) {
> + if (!echo)
> opts->ahmac = add_addr_generate_hmac(msk->local_key,
> msk->remote_key,
> &opts->addr);
> - }
> + else
> + opts->ahmac = 0;
opts->ahmac had been set to zero in mptcp_get_options, no need to clear it
again.
> pr_debug("addr_id=%d, addr_port=%d, ahmac=%llu, echo=%d",
> opts->addr.id, ntohs(opts->addr.port), opts->ahmac, echo);
>
> @@ -735,7 +738,12 @@ static bool mptcp_established_options_mp_prio(struct sock *sk,
> {
> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
>
> - if (!subflow->send_mp_prio)
> + /* can't send MP_PRIO with MPC, as they share the same option space:
> + * 'backup'. Also it makes no sense at all
> + */
> + if (!subflow->send_mp_prio ||
> + ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
> + OPTION_MPTCP_MPC_ACK) & opts->suboptions))
> return false;
>
> /* account for the trailing 'nop' option */
> @@ -1198,7 +1206,73 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> struct mptcp_out_options *opts)
> {
> - if ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
> + /* RST is mutually exclusive with everything else */
> + if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
> + *ptr++ = mptcp_option(MPTCPOPT_RST,
> + TCPOLEN_MPTCP_RST,
> + opts->reset_transient,
> + opts->reset_reason);
> + return;
> + }
> +
> + /* DSS, MPC, MPJ and ADD_ADDR are mutually exclusive, see
> + * mptcp_established_options*()
> + */
> + if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
> + struct mptcp_ext *mpext = &opts->ext_copy;
> + u8 len = TCPOLEN_MPTCP_DSS_BASE;
> + u8 flags = 0;
> +
> + if (mpext->use_ack) {
> + flags = MPTCP_DSS_HAS_ACK;
> + if (mpext->ack64) {
> + len += TCPOLEN_MPTCP_DSS_ACK64;
> + flags |= MPTCP_DSS_ACK64;
> + } else {
> + len += TCPOLEN_MPTCP_DSS_ACK32;
> + }
> + }
> +
> + if (mpext->use_map) {
> + len += TCPOLEN_MPTCP_DSS_MAP64;
> +
> + /* Use only 64-bit mapping flags for now, add
> + * support for optional 32-bit mappings later.
> + */
> + flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
> + if (mpext->data_fin)
> + flags |= MPTCP_DSS_DATA_FIN;
> +
> + if (opts->csum_reqd)
> + len += TCPOLEN_MPTCP_DSS_CHECKSUM;
> + }
> +
> + *ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);
> +
> + if (mpext->use_ack) {
> + if (mpext->ack64) {
> + put_unaligned_be64(mpext->data_ack, ptr);
> + ptr += 2;
> + } else {
> + put_unaligned_be32(mpext->data_ack32, ptr);
> + ptr += 1;
> + }
> + }
> +
> + if (mpext->use_map) {
> + put_unaligned_be64(mpext->data_seq, ptr);
> + ptr += 2;
> + put_unaligned_be32(mpext->subflow_seq, ptr);
> + ptr += 1;
> + if (opts->csum_reqd) {
> + put_unaligned_be32(mpext->data_len << 16 |
> + mptcp_make_csum(mpext), ptr);
> + } else {
> + put_unaligned_be32(mpext->data_len << 16 |
> + TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> + }
> + }
> + } else if ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
> OPTION_MPTCP_MPC_ACK) & opts->suboptions) {
> u8 len, flag = MPTCP_CAP_HMAC_SHA256;
>
> @@ -1246,10 +1320,31 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> }
> ptr += 1;
> - }
>
> -mp_capable_done:
> - if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> + /* MPC is additionally mutually exclusive with MP_PRIO */
> + goto mp_capable_done;
> + } else if (OPTION_MPTCP_MPJ_SYN & opts->suboptions) {
> + *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> + TCPOLEN_MPTCP_MPJ_SYN,
> + opts->backup, opts->join_id);
> + put_unaligned_be32(opts->token, ptr);
> + ptr += 1;
> + put_unaligned_be32(opts->nonce, ptr);
> + ptr += 1;
> + } else if (OPTION_MPTCP_MPJ_SYNACK & opts->suboptions) {
> + *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> + TCPOLEN_MPTCP_MPJ_SYNACK,
> + opts->backup, opts->join_id);
> + put_unaligned_be64(opts->thmac, ptr);
> + ptr += 2;
> + put_unaligned_be32(opts->nonce, ptr);
> + ptr += 1;
> + } else if (OPTION_MPTCP_MPJ_ACK & opts->suboptions) {
> + *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> + TCPOLEN_MPTCP_MPJ_ACK, 0, 0);
> + memcpy(ptr, opts->hmac, MPTCPOPT_HMAC_LEN);
> + ptr += 5;
> + } else if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> struct mptcp_addr_info *addr = &opts->addr;
> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> u8 echo = MPTCP_ADDR_ECHO;
> @@ -1308,6 +1403,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> }
> }
>
> + if (OPTION_MPTCP_PRIO & opts->suboptions) {
> + const struct sock *ssk = (const struct sock *)tp;
> + struct mptcp_subflow_context *subflow;
> +
> + subflow = mptcp_subflow_ctx(ssk);
> + subflow->send_mp_prio = 0;
> +
> + *ptr++ = mptcp_option(MPTCPOPT_MP_PRIO,
> + TCPOLEN_MPTCP_PRIO,
> + opts->backup, TCPOPT_NOP);
> + }
> +
> +mp_capable_done:
> if (OPTION_MPTCP_RM_ADDR & opts->suboptions) {
> u8 i = 1;
>
> @@ -1328,107 +1436,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> }
> }
>
> - if (OPTION_MPTCP_PRIO & opts->suboptions) {
> - const struct sock *ssk = (const struct sock *)tp;
> - struct mptcp_subflow_context *subflow;
> -
> - subflow = mptcp_subflow_ctx(ssk);
> - subflow->send_mp_prio = 0;
> -
> - *ptr++ = mptcp_option(MPTCPOPT_MP_PRIO,
> - TCPOLEN_MPTCP_PRIO,
> - opts->backup, TCPOPT_NOP);
> - }
> -
> - if (OPTION_MPTCP_MPJ_SYN & opts->suboptions) {
> - *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> - TCPOLEN_MPTCP_MPJ_SYN,
> - opts->backup, opts->join_id);
> - put_unaligned_be32(opts->token, ptr);
> - ptr += 1;
> - put_unaligned_be32(opts->nonce, ptr);
> - ptr += 1;
> - }
> -
> - if (OPTION_MPTCP_MPJ_SYNACK & opts->suboptions) {
> - *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> - TCPOLEN_MPTCP_MPJ_SYNACK,
> - opts->backup, opts->join_id);
> - put_unaligned_be64(opts->thmac, ptr);
> - ptr += 2;
> - put_unaligned_be32(opts->nonce, ptr);
> - ptr += 1;
> - }
> -
> - if (OPTION_MPTCP_MPJ_ACK & opts->suboptions) {
> - *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> - TCPOLEN_MPTCP_MPJ_ACK, 0, 0);
> - memcpy(ptr, opts->hmac, MPTCPOPT_HMAC_LEN);
> - ptr += 5;
> - }
> -
> - if (OPTION_MPTCP_RST & opts->suboptions)
> - *ptr++ = mptcp_option(MPTCPOPT_RST,
> - TCPOLEN_MPTCP_RST,
> - opts->reset_transient,
> - opts->reset_reason);
> -
> - if (opts->ext_copy.use_ack || opts->ext_copy.use_map) {
> - struct mptcp_ext *mpext = &opts->ext_copy;
> - u8 len = TCPOLEN_MPTCP_DSS_BASE;
> - u8 flags = 0;
> -
> - if (mpext->use_ack) {
> - flags = MPTCP_DSS_HAS_ACK;
> - if (mpext->ack64) {
> - len += TCPOLEN_MPTCP_DSS_ACK64;
> - flags |= MPTCP_DSS_ACK64;
> - } else {
> - len += TCPOLEN_MPTCP_DSS_ACK32;
> - }
> - }
> -
> - if (mpext->use_map) {
> - len += TCPOLEN_MPTCP_DSS_MAP64;
> -
> - /* Use only 64-bit mapping flags for now, add
> - * support for optional 32-bit mappings later.
> - */
> - flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
> - if (mpext->data_fin)
> - flags |= MPTCP_DSS_DATA_FIN;
> -
> - if (opts->csum_reqd)
> - len += TCPOLEN_MPTCP_DSS_CHECKSUM;
> - }
> -
> - *ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);
> -
> - if (mpext->use_ack) {
> - if (mpext->ack64) {
> - put_unaligned_be64(mpext->data_ack, ptr);
> - ptr += 2;
> - } else {
> - put_unaligned_be32(mpext->data_ack32, ptr);
> - ptr += 1;
> - }
> - }
> -
> - if (mpext->use_map) {
> - put_unaligned_be64(mpext->data_seq, ptr);
> - ptr += 2;
> - put_unaligned_be32(mpext->subflow_seq, ptr);
> - ptr += 1;
> - if (opts->csum_reqd) {
> - put_unaligned_be32(mpext->data_len << 16 |
> - mptcp_make_csum(mpext), ptr);
> - } else {
> - put_unaligned_be32(mpext->data_len << 16 |
> - TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> - }
> - }
> - }
> -
> if (tp)
> mptcp_set_rwin(tp);
> }
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 4d1828fd2482..baf2d6e524bd 100644
> --- a/net/mptcp/pm.c
> +++ b/net/mptcp/pm.c
> @@ -266,10 +266,11 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> if (!mptcp_pm_should_add_signal(msk))
> goto out_unlock;
>
> - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> - skb && skb_is_tcp_pure_ack(skb)) {
> + /* always drop every other options for pure ack ADD_ADDR; this is
> + * plain dup-ack from TCP perspective. The other MPTCP-relevant info,
> + * if any, will be carried by the other pure ack
> + */
> + if (skb && skb_is_tcp_pure_ack(skb)) {
> remaining += opt_size;
> *drop_other_suboptions = true;
> }
This part in pm.c should be squashed to the patch "mptcp: move
drop_other_suboptions check under pm lock".
WDYT?
Thanks,
-Geliang
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index 34ad1c4bc29f..d960d8b8a8ce 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -26,6 +26,7 @@
> #define OPTION_MPTCP_FASTCLOSE BIT(8)
> #define OPTION_MPTCP_PRIO BIT(9)
> #define OPTION_MPTCP_RST BIT(10)
> +#define OPTION_MPTCP_DSS BIT(11)
>
> /* MPTCP option subtypes */
> #define MPTCPOPT_MP_CAPABLE 0
> --
> 2.26.3
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-next 1/2] mptcp: optimize out option generation
2021-07-26 3:21 ` Geliang Tang
@ 2021-07-26 9:44 ` Paolo Abeni
2021-07-26 10:02 ` Geliang Tang
2021-07-26 10:06 ` Paolo Abeni
1 sibling, 1 reply; 7+ messages in thread
From: Paolo Abeni @ 2021-07-26 9:44 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
On Mon, 2021-07-26 at 11:21 +0800, Geliang Tang wrote:
> Hi Paolo,
>
> Thanks for this patchset, and I have some small comments.
>
> Paolo Abeni <pabeni@redhat.com> 于2021年7月24日周六 上午1:06写道：
> > Currently we have several protocol contraint on MPTCP option
> > generation (e.g. MPC and MPJ subopt are mutually exclusive)
> > and some additionall ones required by our implementation
> > (e.g. almost all ADD_ADDR variant are mutually exclusive with
> > everything else).
> >
> > We can leverage the above to optimize the out option generation:
> > we check DSS/MPC/MPJ presencence in a mutually exclusive way,
> > avoiding many unneeded conditionals in the common case.
> >
> > Additionally extend the existing constraint on ADD_ADDR opt on
> > all subvariant, so that it become fully mutually exclusive with
> > the above and we can skip another conditional statement the common
> > case.
> >
> > This change is also need by the next patch.
> >
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > --
> > side node: we could probably optimize the conditionals in
> > mptcp_established_options()
> > ---
> > net/mptcp/options.c | 225 ++++++++++++++++++++++---------------------
> > net/mptcp/pm.c | 9 +-
> > net/mptcp/protocol.h | 1 +
> > 3 files changed, 122 insertions(+), 113 deletions(-)
> >
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index eafdb9408f3a..38f324667761 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -592,6 +592,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > dss_size = map_size;
> > if (skb && snd_data_fin_enable)
> > mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
> > + opts->suboptions = OPTION_MPTCP_DSS;
> > ret = true;
> > }
> >
> > @@ -615,6 +616,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > opts->ext_copy.ack64 = 0;
> > }
> > opts->ext_copy.use_ack = 1;
> > + opts->suboptions = OPTION_MPTCP_DSS;
> > WRITE_ONCE(msk->old_wspace, __mptcp_space((struct sock *)msk));
> >
> > /* Add kind/length/subtype/flag overhead if mapping is not populated */
> > @@ -663,9 +665,9 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>
> I think no need to modify the code in mptcp_established_options_add_addr.
>
> > struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> > bool drop_other_suboptions = false;
> > unsigned int opt_size = *size;
> > + unsigned int len;
> > bool echo;
> > bool port;
> > - int len;
>
> I prefer to keep the type of len unchanged.
OK.
>
> > if (!mptcp_pm_should_add_signal(msk) ||
> > !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts,
> > @@ -687,11 +689,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > *size -= opt_size;
> > }
> > opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> > - if (!echo) {
> > + if (!echo)
> > opts->ahmac = add_addr_generate_hmac(msk->local_key,
> > msk->remote_key,
> > &opts->addr);
> > - }
> > + else
> > + opts->ahmac = 0;
>
> opts->ahmac had been set to zero in mptcp_get_options, no need to clear it
> again.
This is for the output path, mptcp_get_options() is not invoked. 'ops'
has been cleared by the TCP caller. Sice a few fields are aliased, and
the add_addr is added after the DSS, the latter could have already
modified the underlaying memory.
Without this assignment we will end-up appending the hmac even for
add_addr 'echo', and a bunch of tests will fail.
Perhaps is better adding some comments explaining why such set
operation is needed...
>
> > pr_debug("addr_id=%d, addr_port=%d, ahmac=%llu, echo=%d",
> > opts->addr.id, ntohs(opts->addr.port), opts->ahmac, echo);
> >
> > @@ -735,7 +738,12 @@ static bool mptcp_established_options_mp_prio(struct sock *sk,
> > {
> > struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> >
> > - if (!subflow->send_mp_prio)
> > + /* can't send MP_PRIO with MPC, as they share the same option space:
> > + * 'backup'. Also it makes no sense at all
> > + */
> > + if (!subflow->send_mp_prio ||
> > + ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
> > + OPTION_MPTCP_MPC_ACK) & opts->suboptions))
> > return false;
> >
> > /* account for the trailing 'nop' option */
> > @@ -1198,7 +1206,73 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> > void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> > struct mptcp_out_options *opts)
> > {
> > - if ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
> > + /* RST is mutually exclusive with everything else */
> > + if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
> > + *ptr++ = mptcp_option(MPTCPOPT_RST,
> > + TCPOLEN_MPTCP_RST,
> > + opts->reset_transient,
> > + opts->reset_reason);
> > + return;
> > + }
> > +
> > + /* DSS, MPC, MPJ and ADD_ADDR are mutually exclusive, see
> > + * mptcp_established_options*()
> > + */
> > + if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
> > + struct mptcp_ext *mpext = &opts->ext_copy;
> > + u8 len = TCPOLEN_MPTCP_DSS_BASE;
> > + u8 flags = 0;
> > +
> > + if (mpext->use_ack) {
> > + flags = MPTCP_DSS_HAS_ACK;
> > + if (mpext->ack64) {
> > + len += TCPOLEN_MPTCP_DSS_ACK64;
> > + flags |= MPTCP_DSS_ACK64;
> > + } else {
> > + len += TCPOLEN_MPTCP_DSS_ACK32;
> > + }
> > + }
> > +
> > + if (mpext->use_map) {
> > + len += TCPOLEN_MPTCP_DSS_MAP64;
> > +
> > + /* Use only 64-bit mapping flags for now, add
> > + * support for optional 32-bit mappings later.
> > + */
> > + flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
> > + if (mpext->data_fin)
> > + flags |= MPTCP_DSS_DATA_FIN;
> > +
> > + if (opts->csum_reqd)
> > + len += TCPOLEN_MPTCP_DSS_CHECKSUM;
> > + }
> > +
> > + *ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);
> > +
> > + if (mpext->use_ack) {
> > + if (mpext->ack64) {
> > + put_unaligned_be64(mpext->data_ack, ptr);
> > + ptr += 2;
> > + } else {
> > + put_unaligned_be32(mpext->data_ack32, ptr);
> > + ptr += 1;
> > + }
> > + }
> > +
> > + if (mpext->use_map) {
> > + put_unaligned_be64(mpext->data_seq, ptr);
> > + ptr += 2;
> > + put_unaligned_be32(mpext->subflow_seq, ptr);
> > + ptr += 1;
> > + if (opts->csum_reqd) {
> > + put_unaligned_be32(mpext->data_len << 16 |
> > + mptcp_make_csum(mpext), ptr);
> > + } else {
> > + put_unaligned_be32(mpext->data_len << 16 |
> > + TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> > + }
> > + }
> > + } else if ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
> > OPTION_MPTCP_MPC_ACK) & opts->suboptions) {
> > u8 len, flag = MPTCP_CAP_HMAC_SHA256;
> >
> > @@ -1246,10 +1320,31 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> > TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> > }
> > ptr += 1;
> > - }
> >
> > -mp_capable_done:
> > - if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> > + /* MPC is additionally mutually exclusive with MP_PRIO */
> > + goto mp_capable_done;
> > + } else if (OPTION_MPTCP_MPJ_SYN & opts->suboptions) {
> > + *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > + TCPOLEN_MPTCP_MPJ_SYN,
> > + opts->backup, opts->join_id);
> > + put_unaligned_be32(opts->token, ptr);
> > + ptr += 1;
> > + put_unaligned_be32(opts->nonce, ptr);
> > + ptr += 1;
> > + } else if (OPTION_MPTCP_MPJ_SYNACK & opts->suboptions) {
> > + *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > + TCPOLEN_MPTCP_MPJ_SYNACK,
> > + opts->backup, opts->join_id);
> > + put_unaligned_be64(opts->thmac, ptr);
> > + ptr += 2;
> > + put_unaligned_be32(opts->nonce, ptr);
> > + ptr += 1;
> > + } else if (OPTION_MPTCP_MPJ_ACK & opts->suboptions) {
> > + *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > + TCPOLEN_MPTCP_MPJ_ACK, 0, 0);
> > + memcpy(ptr, opts->hmac, MPTCPOPT_HMAC_LEN);
> > + ptr += 5;
> > + } else if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> > struct mptcp_addr_info *addr = &opts->addr;
> > u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> > u8 echo = MPTCP_ADDR_ECHO;
> > @@ -1308,6 +1403,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> > }
> > }
> >
> > + if (OPTION_MPTCP_PRIO & opts->suboptions) {
> > + const struct sock *ssk = (const struct sock *)tp;
> > + struct mptcp_subflow_context *subflow;
> > +
> > + subflow = mptcp_subflow_ctx(ssk);
> > + subflow->send_mp_prio = 0;
> > +
> > + *ptr++ = mptcp_option(MPTCPOPT_MP_PRIO,
> > + TCPOLEN_MPTCP_PRIO,
> > + opts->backup, TCPOPT_NOP);
> > + }
> > +
> > +mp_capable_done:
> > if (OPTION_MPTCP_RM_ADDR & opts->suboptions) {
> > u8 i = 1;
> >
> > @@ -1328,107 +1436,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> > }
> > }
> >
> > - if (OPTION_MPTCP_PRIO & opts->suboptions) {
> > - const struct sock *ssk = (const struct sock *)tp;
> > - struct mptcp_subflow_context *subflow;
> > -
> > - subflow = mptcp_subflow_ctx(ssk);
> > - subflow->send_mp_prio = 0;
> > -
> > - *ptr++ = mptcp_option(MPTCPOPT_MP_PRIO,
> > - TCPOLEN_MPTCP_PRIO,
> > - opts->backup, TCPOPT_NOP);
> > - }
> > -
> > - if (OPTION_MPTCP_MPJ_SYN & opts->suboptions) {
> > - *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > - TCPOLEN_MPTCP_MPJ_SYN,
> > - opts->backup, opts->join_id);
> > - put_unaligned_be32(opts->token, ptr);
> > - ptr += 1;
> > - put_unaligned_be32(opts->nonce, ptr);
> > - ptr += 1;
> > - }
> > -
> > - if (OPTION_MPTCP_MPJ_SYNACK & opts->suboptions) {
> > - *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > - TCPOLEN_MPTCP_MPJ_SYNACK,
> > - opts->backup, opts->join_id);
> > - put_unaligned_be64(opts->thmac, ptr);
> > - ptr += 2;
> > - put_unaligned_be32(opts->nonce, ptr);
> > - ptr += 1;
> > - }
> > -
> > - if (OPTION_MPTCP_MPJ_ACK & opts->suboptions) {
> > - *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > - TCPOLEN_MPTCP_MPJ_ACK, 0, 0);
> > - memcpy(ptr, opts->hmac, MPTCPOPT_HMAC_LEN);
> > - ptr += 5;
> > - }
> > -
> > - if (OPTION_MPTCP_RST & opts->suboptions)
> > - *ptr++ = mptcp_option(MPTCPOPT_RST,
> > - TCPOLEN_MPTCP_RST,
> > - opts->reset_transient,
> > - opts->reset_reason);
> > -
> > - if (opts->ext_copy.use_ack || opts->ext_copy.use_map) {
> > - struct mptcp_ext *mpext = &opts->ext_copy;
> > - u8 len = TCPOLEN_MPTCP_DSS_BASE;
> > - u8 flags = 0;
> > -
> > - if (mpext->use_ack) {
> > - flags = MPTCP_DSS_HAS_ACK;
> > - if (mpext->ack64) {
> > - len += TCPOLEN_MPTCP_DSS_ACK64;
> > - flags |= MPTCP_DSS_ACK64;
> > - } else {
> > - len += TCPOLEN_MPTCP_DSS_ACK32;
> > - }
> > - }
> > -
> > - if (mpext->use_map) {
> > - len += TCPOLEN_MPTCP_DSS_MAP64;
> > -
> > - /* Use only 64-bit mapping flags for now, add
> > - * support for optional 32-bit mappings later.
> > - */
> > - flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
> > - if (mpext->data_fin)
> > - flags |= MPTCP_DSS_DATA_FIN;
> > -
> > - if (opts->csum_reqd)
> > - len += TCPOLEN_MPTCP_DSS_CHECKSUM;
> > - }
> > -
> > - *ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);
> > -
> > - if (mpext->use_ack) {
> > - if (mpext->ack64) {
> > - put_unaligned_be64(mpext->data_ack, ptr);
> > - ptr += 2;
> > - } else {
> > - put_unaligned_be32(mpext->data_ack32, ptr);
> > - ptr += 1;
> > - }
> > - }
> > -
> > - if (mpext->use_map) {
> > - put_unaligned_be64(mpext->data_seq, ptr);
> > - ptr += 2;
> > - put_unaligned_be32(mpext->subflow_seq, ptr);
> > - ptr += 1;
> > - if (opts->csum_reqd) {
> > - put_unaligned_be32(mpext->data_len << 16 |
> > - mptcp_make_csum(mpext), ptr);
> > - } else {
> > - put_unaligned_be32(mpext->data_len << 16 |
> > - TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> > - }
> > - }
> > - }
> > -
> > if (tp)
> > mptcp_set_rwin(tp);
> > }
> > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > index 4d1828fd2482..baf2d6e524bd 100644
> > --- a/net/mptcp/pm.c
> > +++ b/net/mptcp/pm.c
> > @@ -266,10 +266,11 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> > if (!mptcp_pm_should_add_signal(msk))
> > goto out_unlock;
> >
> > - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> > - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> > - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> > - skb && skb_is_tcp_pure_ack(skb)) {
> > + /* always drop every other options for pure ack ADD_ADDR; this is
> > + * plain dup-ack from TCP perspective. The other MPTCP-relevant info,
> > + * if any, will be carried by the other pure ack
> > + */
> > + if (skb && skb_is_tcp_pure_ack(skb)) {
> > remaining += opt_size;
> > *drop_other_suboptions = true;
> > }
>
> This part in pm.c should be squashed to the patch "mptcp: move
> drop_other_suboptions check under pm lock".
>
> WDYT?
I was wondering if that chunk would deserve a separate patch.
Squashing the change in another patch would still mix multiple intens
in a single change, but the modifications are indeed related. Overall
agreed ;)
/P
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-next 1/2] mptcp: optimize out option generation
2021-07-26 9:44 ` Paolo Abeni
@ 2021-07-26 10:02 ` Geliang Tang
0 siblings, 0 replies; 7+ messages in thread
From: Geliang Tang @ 2021-07-26 10:02 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Paolo Abeni <pabeni@redhat.com> 于2021年7月26日周一 下午5:44写道：
>
> On Mon, 2021-07-26 at 11:21 +0800, Geliang Tang wrote:
> > Hi Paolo,
> >
> > Thanks for this patchset, and I have some small comments.
> >
> > Paolo Abeni <pabeni@redhat.com> 于2021年7月24日周六 上午1:06写道：
> > > Currently we have several protocol contraint on MPTCP option
> > > generation (e.g. MPC and MPJ subopt are mutually exclusive)
> > > and some additionall ones required by our implementation
> > > (e.g. almost all ADD_ADDR variant are mutually exclusive with
> > > everything else).
> > >
> > > We can leverage the above to optimize the out option generation:
> > > we check DSS/MPC/MPJ presencence in a mutually exclusive way,
> > > avoiding many unneeded conditionals in the common case.
> > >
> > > Additionally extend the existing constraint on ADD_ADDR opt on
> > > all subvariant, so that it become fully mutually exclusive with
> > > the above and we can skip another conditional statement the common
> > > case.
> > >
> > > This change is also need by the next patch.
> > >
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > --
> > > side node: we could probably optimize the conditionals in
> > > mptcp_established_options()
> > > ---
> > > net/mptcp/options.c | 225 ++++++++++++++++++++++---------------------
> > > net/mptcp/pm.c | 9 +-
> > > net/mptcp/protocol.h | 1 +
> > > 3 files changed, 122 insertions(+), 113 deletions(-)
> > >
> > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > > index eafdb9408f3a..38f324667761 100644
> > > --- a/net/mptcp/options.c
> > > +++ b/net/mptcp/options.c
> > > @@ -592,6 +592,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > > dss_size = map_size;
> > > if (skb && snd_data_fin_enable)
> > > mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
> > > + opts->suboptions = OPTION_MPTCP_DSS;
> > > ret = true;
> > > }
> > >
> > > @@ -615,6 +616,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > > opts->ext_copy.ack64 = 0;
> > > }
> > > opts->ext_copy.use_ack = 1;
> > > + opts->suboptions = OPTION_MPTCP_DSS;
> > > WRITE_ONCE(msk->old_wspace, __mptcp_space((struct sock *)msk));
> > >
> > > /* Add kind/length/subtype/flag overhead if mapping is not populated */
> > > @@ -663,9 +665,9 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> >
> > I think no need to modify the code in mptcp_established_options_add_addr.
> >
> > > struct mptcp_sock *msk = mptcp_sk(subflow->conn);
> > > bool drop_other_suboptions = false;
> > > unsigned int opt_size = *size;
> > > + unsigned int len;
> > > bool echo;
> > > bool port;
> > > - int len;
> >
> > I prefer to keep the type of len unchanged.
>
> OK.
>
> >
> > > if (!mptcp_pm_should_add_signal(msk) ||
> > > !mptcp_pm_add_addr_signal(msk, skb, opt_size, remaining, opts,
> > > @@ -687,11 +689,12 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> > > *size -= opt_size;
> > > }
> > > opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> > > - if (!echo) {
> > > + if (!echo)
> > > opts->ahmac = add_addr_generate_hmac(msk->local_key,
> > > msk->remote_key,
> > > &opts->addr);
> > > - }
> > > + else
> > > + opts->ahmac = 0;
> >
> > opts->ahmac had been set to zero in mptcp_get_options, no need to clear it
> > again.
>
> This is for the output path, mptcp_get_options() is not invoked. 'ops'
Sorry, my fault.
> has been cleared by the TCP caller. Sice a few fields are aliased, and
> the add_addr is added after the DSS, the latter could have already
> modified the underlaying memory.
>
> Without this assignment we will end-up appending the hmac even for
> add_addr 'echo', and a bunch of tests will fail.
>
> Perhaps is better adding some comments explaining why such set
> operation is needed...
Yes, that's helpful.
> >
> > > pr_debug("addr_id=%d, addr_port=%d, ahmac=%llu, echo=%d",
> > > opts->addr.id, ntohs(opts->addr.port), opts->ahmac, echo);
> > >
> > > @@ -735,7 +738,12 @@ static bool mptcp_established_options_mp_prio(struct sock *sk,
> > > {
> > > struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> > >
> > > - if (!subflow->send_mp_prio)
> > > + /* can't send MP_PRIO with MPC, as they share the same option space:
> > > + * 'backup'. Also it makes no sense at all
> > > + */
> > > + if (!subflow->send_mp_prio ||
> > > + ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
> > > + OPTION_MPTCP_MPC_ACK) & opts->suboptions))
> > > return false;
> > >
> > > /* account for the trailing 'nop' option */
> > > @@ -1198,7 +1206,73 @@ static u16 mptcp_make_csum(const struct mptcp_ext *mpext)
> > > void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> > > struct mptcp_out_options *opts)
> > > {
> > > - if ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
> > > + /* RST is mutually exclusive with everything else */
> > > + if (unlikely(OPTION_MPTCP_RST & opts->suboptions)) {
> > > + *ptr++ = mptcp_option(MPTCPOPT_RST,
> > > + TCPOLEN_MPTCP_RST,
> > > + opts->reset_transient,
> > > + opts->reset_reason);
> > > + return;
> > > + }
> > > +
> > > + /* DSS, MPC, MPJ and ADD_ADDR are mutually exclusive, see
> > > + * mptcp_established_options*()
> > > + */
> > > + if (likely(OPTION_MPTCP_DSS & opts->suboptions)) {
> > > + struct mptcp_ext *mpext = &opts->ext_copy;
> > > + u8 len = TCPOLEN_MPTCP_DSS_BASE;
> > > + u8 flags = 0;
> > > +
> > > + if (mpext->use_ack) {
> > > + flags = MPTCP_DSS_HAS_ACK;
> > > + if (mpext->ack64) {
> > > + len += TCPOLEN_MPTCP_DSS_ACK64;
> > > + flags |= MPTCP_DSS_ACK64;
> > > + } else {
> > > + len += TCPOLEN_MPTCP_DSS_ACK32;
> > > + }
> > > + }
> > > +
> > > + if (mpext->use_map) {
> > > + len += TCPOLEN_MPTCP_DSS_MAP64;
> > > +
> > > + /* Use only 64-bit mapping flags for now, add
> > > + * support for optional 32-bit mappings later.
> > > + */
> > > + flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
> > > + if (mpext->data_fin)
> > > + flags |= MPTCP_DSS_DATA_FIN;
> > > +
> > > + if (opts->csum_reqd)
> > > + len += TCPOLEN_MPTCP_DSS_CHECKSUM;
> > > + }
> > > +
> > > + *ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);
> > > +
> > > + if (mpext->use_ack) {
> > > + if (mpext->ack64) {
> > > + put_unaligned_be64(mpext->data_ack, ptr);
> > > + ptr += 2;
> > > + } else {
> > > + put_unaligned_be32(mpext->data_ack32, ptr);
> > > + ptr += 1;
> > > + }
> > > + }
> > > +
> > > + if (mpext->use_map) {
> > > + put_unaligned_be64(mpext->data_seq, ptr);
> > > + ptr += 2;
> > > + put_unaligned_be32(mpext->subflow_seq, ptr);
> > > + ptr += 1;
> > > + if (opts->csum_reqd) {
> > > + put_unaligned_be32(mpext->data_len << 16 |
> > > + mptcp_make_csum(mpext), ptr);
> > > + } else {
> > > + put_unaligned_be32(mpext->data_len << 16 |
> > > + TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> > > + }
> > > + }
> > > + } else if ((OPTION_MPTCP_MPC_SYN | OPTION_MPTCP_MPC_SYNACK |
> > > OPTION_MPTCP_MPC_ACK) & opts->suboptions) {
> > > u8 len, flag = MPTCP_CAP_HMAC_SHA256;
> > >
> > > @@ -1246,10 +1320,31 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> > > TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> > > }
> > > ptr += 1;
> > > - }
> > >
> > > -mp_capable_done:
> > > - if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> > > + /* MPC is additionally mutually exclusive with MP_PRIO */
> > > + goto mp_capable_done;
> > > + } else if (OPTION_MPTCP_MPJ_SYN & opts->suboptions) {
> > > + *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > > + TCPOLEN_MPTCP_MPJ_SYN,
> > > + opts->backup, opts->join_id);
> > > + put_unaligned_be32(opts->token, ptr);
> > > + ptr += 1;
> > > + put_unaligned_be32(opts->nonce, ptr);
> > > + ptr += 1;
> > > + } else if (OPTION_MPTCP_MPJ_SYNACK & opts->suboptions) {
> > > + *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > > + TCPOLEN_MPTCP_MPJ_SYNACK,
> > > + opts->backup, opts->join_id);
> > > + put_unaligned_be64(opts->thmac, ptr);
> > > + ptr += 2;
> > > + put_unaligned_be32(opts->nonce, ptr);
> > > + ptr += 1;
> > > + } else if (OPTION_MPTCP_MPJ_ACK & opts->suboptions) {
> > > + *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > > + TCPOLEN_MPTCP_MPJ_ACK, 0, 0);
> > > + memcpy(ptr, opts->hmac, MPTCPOPT_HMAC_LEN);
> > > + ptr += 5;
> > > + } else if (OPTION_MPTCP_ADD_ADDR & opts->suboptions) {
> > > struct mptcp_addr_info *addr = &opts->addr;
> > > u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> > > u8 echo = MPTCP_ADDR_ECHO;
> > > @@ -1308,6 +1403,19 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> > > }
> > > }
> > >
> > > + if (OPTION_MPTCP_PRIO & opts->suboptions) {
> > > + const struct sock *ssk = (const struct sock *)tp;
> > > + struct mptcp_subflow_context *subflow;
> > > +
> > > + subflow = mptcp_subflow_ctx(ssk);
> > > + subflow->send_mp_prio = 0;
> > > +
> > > + *ptr++ = mptcp_option(MPTCPOPT_MP_PRIO,
> > > + TCPOLEN_MPTCP_PRIO,
> > > + opts->backup, TCPOPT_NOP);
> > > + }
> > > +
> > > +mp_capable_done:
> > > if (OPTION_MPTCP_RM_ADDR & opts->suboptions) {
> > > u8 i = 1;
> > >
> > > @@ -1328,107 +1436,6 @@ void mptcp_write_options(__be32 *ptr, const struct tcp_sock *tp,
> > > }
> > > }
> > >
> > > - if (OPTION_MPTCP_PRIO & opts->suboptions) {
> > > - const struct sock *ssk = (const struct sock *)tp;
> > > - struct mptcp_subflow_context *subflow;
> > > -
> > > - subflow = mptcp_subflow_ctx(ssk);
> > > - subflow->send_mp_prio = 0;
> > > -
> > > - *ptr++ = mptcp_option(MPTCPOPT_MP_PRIO,
> > > - TCPOLEN_MPTCP_PRIO,
> > > - opts->backup, TCPOPT_NOP);
> > > - }
> > > -
> > > - if (OPTION_MPTCP_MPJ_SYN & opts->suboptions) {
> > > - *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > > - TCPOLEN_MPTCP_MPJ_SYN,
> > > - opts->backup, opts->join_id);
> > > - put_unaligned_be32(opts->token, ptr);
> > > - ptr += 1;
> > > - put_unaligned_be32(opts->nonce, ptr);
> > > - ptr += 1;
> > > - }
> > > -
> > > - if (OPTION_MPTCP_MPJ_SYNACK & opts->suboptions) {
> > > - *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > > - TCPOLEN_MPTCP_MPJ_SYNACK,
> > > - opts->backup, opts->join_id);
> > > - put_unaligned_be64(opts->thmac, ptr);
> > > - ptr += 2;
> > > - put_unaligned_be32(opts->nonce, ptr);
> > > - ptr += 1;
> > > - }
> > > -
> > > - if (OPTION_MPTCP_MPJ_ACK & opts->suboptions) {
> > > - *ptr++ = mptcp_option(MPTCPOPT_MP_JOIN,
> > > - TCPOLEN_MPTCP_MPJ_ACK, 0, 0);
> > > - memcpy(ptr, opts->hmac, MPTCPOPT_HMAC_LEN);
> > > - ptr += 5;
> > > - }
> > > -
> > > - if (OPTION_MPTCP_RST & opts->suboptions)
> > > - *ptr++ = mptcp_option(MPTCPOPT_RST,
> > > - TCPOLEN_MPTCP_RST,
> > > - opts->reset_transient,
> > > - opts->reset_reason);
> > > -
> > > - if (opts->ext_copy.use_ack || opts->ext_copy.use_map) {
> > > - struct mptcp_ext *mpext = &opts->ext_copy;
> > > - u8 len = TCPOLEN_MPTCP_DSS_BASE;
> > > - u8 flags = 0;
> > > -
> > > - if (mpext->use_ack) {
> > > - flags = MPTCP_DSS_HAS_ACK;
> > > - if (mpext->ack64) {
> > > - len += TCPOLEN_MPTCP_DSS_ACK64;
> > > - flags |= MPTCP_DSS_ACK64;
> > > - } else {
> > > - len += TCPOLEN_MPTCP_DSS_ACK32;
> > > - }
> > > - }
> > > -
> > > - if (mpext->use_map) {
> > > - len += TCPOLEN_MPTCP_DSS_MAP64;
> > > -
> > > - /* Use only 64-bit mapping flags for now, add
> > > - * support for optional 32-bit mappings later.
> > > - */
> > > - flags |= MPTCP_DSS_HAS_MAP | MPTCP_DSS_DSN64;
> > > - if (mpext->data_fin)
> > > - flags |= MPTCP_DSS_DATA_FIN;
> > > -
> > > - if (opts->csum_reqd)
> > > - len += TCPOLEN_MPTCP_DSS_CHECKSUM;
> > > - }
> > > -
> > > - *ptr++ = mptcp_option(MPTCPOPT_DSS, len, 0, flags);
> > > -
> > > - if (mpext->use_ack) {
> > > - if (mpext->ack64) {
> > > - put_unaligned_be64(mpext->data_ack, ptr);
> > > - ptr += 2;
> > > - } else {
> > > - put_unaligned_be32(mpext->data_ack32, ptr);
> > > - ptr += 1;
> > > - }
> > > - }
> > > -
> > > - if (mpext->use_map) {
> > > - put_unaligned_be64(mpext->data_seq, ptr);
> > > - ptr += 2;
> > > - put_unaligned_be32(mpext->subflow_seq, ptr);
> > > - ptr += 1;
> > > - if (opts->csum_reqd) {
> > > - put_unaligned_be32(mpext->data_len << 16 |
> > > - mptcp_make_csum(mpext), ptr);
> > > - } else {
> > > - put_unaligned_be32(mpext->data_len << 16 |
> > > - TCPOPT_NOP << 8 | TCPOPT_NOP, ptr);
> > > - }
> > > - }
> > > - }
> > > -
> > > if (tp)
> > > mptcp_set_rwin(tp);
> > > }
> > > diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> > > index 4d1828fd2482..baf2d6e524bd 100644
> > > --- a/net/mptcp/pm.c
> > > +++ b/net/mptcp/pm.c
> > > @@ -266,10 +266,11 @@ bool mptcp_pm_add_addr_signal(struct mptcp_sock *msk, struct sk_buff *skb,
> > > if (!mptcp_pm_should_add_signal(msk))
> > > goto out_unlock;
> > >
> > > - if (((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_ECHO)) ||
> > > - ((msk->pm.addr_signal & BIT(MPTCP_ADD_ADDR_SIGNAL)) &&
> > > - (msk->pm.local.family == AF_INET6 || msk->pm.local.port))) &&
> > > - skb && skb_is_tcp_pure_ack(skb)) {
> > > + /* always drop every other options for pure ack ADD_ADDR; this is
> > > + * plain dup-ack from TCP perspective. The other MPTCP-relevant info,
> > > + * if any, will be carried by the other pure ack
> > > + */
> > > + if (skb && skb_is_tcp_pure_ack(skb)) {
> > > remaining += opt_size;
> > > *drop_other_suboptions = true;
> > > }
> >
> > This part in pm.c should be squashed to the patch "mptcp: move
> > drop_other_suboptions check under pm lock".
> >
> > WDYT?
>
> I was wondering if that chunk would deserve a separate patch.
Sure, a separate patch is fine.
Thanks,
-Geliang
>
> Squashing the change in another patch would still mix multiple intens
> in a single change, but the modifications are indeed related. Overall
> agreed ;)
>
> /P
>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH mptcp-next 1/2] mptcp: optimize out option generation
2021-07-26 3:21 ` Geliang Tang
2021-07-26 9:44 ` Paolo Abeni
@ 2021-07-26 10:06 ` Paolo Abeni
1 sibling, 0 replies; 7+ messages in thread
From: Paolo Abeni @ 2021-07-26 10:06 UTC (permalink / raw)
To: Geliang Tang; +Cc: mptcp
On Mon, 2021-07-26 at 11:21 +0800, Geliang Tang wrote:
> Hi Paolo,
>
> Thanks for this patchset, and I have some small comments.
>
> Paolo Abeni <pabeni@redhat.com> 于2021年7月24日周六 上午1:06写道：
> > Currently we have several protocol contraint on MPTCP option
> > generation (e.g. MPC and MPJ subopt are mutually exclusive)
> > and some additionall ones required by our implementation
> > (e.g. almost all ADD_ADDR variant are mutually exclusive with
> > everything else).
> >
> > We can leverage the above to optimize the out option generation:
> > we check DSS/MPC/MPJ presencence in a mutually exclusive way,
> > avoiding many unneeded conditionals in the common case.
> >
> > Additionally extend the existing constraint on ADD_ADDR opt on
> > all subvariant, so that it become fully mutually exclusive with
> > the above and we can skip another conditional statement the common
> > case.
> >
> > This change is also need by the next patch.
> >
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > --
> > side node: we could probably optimize the conditionals in
> > mptcp_established_options()
> > ---
> > net/mptcp/options.c | 225 ++++++++++++++++++++++---------------------
> > net/mptcp/pm.c | 9 +-
> > net/mptcp/protocol.h | 1 +
> > 3 files changed, 122 insertions(+), 113 deletions(-)
> >
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index eafdb9408f3a..38f324667761 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -592,6 +592,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > dss_size = map_size;
> > if (skb && snd_data_fin_enable)
> > mptcp_write_data_fin(subflow, skb, &opts->ext_copy);
> > + opts->suboptions = OPTION_MPTCP_DSS;
> > ret = true;
> > }
> >
> > @@ -615,6 +616,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb,
> > opts->ext_copy.ack64 = 0;
> > }
> > opts->ext_copy.use_ack = 1;
> > + opts->suboptions = OPTION_MPTCP_DSS;
> > WRITE_ONCE(msk->old_wspace, __mptcp_space((struct sock *)msk));
> >
> > /* Add kind/length/subtype/flag overhead if mapping is not populated */
> > @@ -663,9 +665,9 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
>
> I think no need to modify the code in mptcp_established_options_add_addr.
Uhmmm... I just noted with need an additional change in there: avoid
clearing 'ext_copy'.
/P
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-26 10:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 17:06 [PATCH mptcp-next 0/2] mptcp: minor out optimization Paolo Abeni
2021-07-23 17:06 ` [PATCH mptcp-next 1/2] mptcp: optimize out option generation Paolo Abeni
2021-07-26 3:21 ` Geliang Tang
2021-07-26 9:44 ` Paolo Abeni
2021-07-26 10:02 ` Geliang Tang
2021-07-26 10:06 ` Paolo Abeni
2021-07-23 17:06 ` [PATCH mptcp-next 2/2] mptcp: shrink mptcp_out_options struct Paolo Abeni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).