* [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization
@ 2021-07-26 10:55 Paolo Abeni
2021-07-26 10:55 ` [PATCH v3 mptcp-next 1/3] Squash-to: "mptcp: move drop_other_suboptions check under pm lock" Paolo Abeni
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Paolo Abeni @ 2021-07-26 10:55 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
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.
This iteration covers the feedback provided by Geliang (many thanks!)
on v1:
- some code moved to new squash-to patch 1/3
- some cleanup in 2/3
please see the individual patch changelog for the details
Paolo Abeni (3):
Squash-to: "mptcp: move drop_other_suboptions check under pm lock"
mptcp: optimize out option generation
mptcp: shrink mptcp_out_options struct
include/net/mptcp.h | 26 +++--
net/mptcp/options.c | 227 +++++++++++++++++++++++--------------------
net/mptcp/pm.c | 9 +-
net/mptcp/protocol.h | 1 +
4 files changed, 142 insertions(+), 121 deletions(-)
--
2.26.3
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 mptcp-next 1/3] Squash-to: "mptcp: move drop_other_suboptions check under pm lock"
2021-07-26 10:55 [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization Paolo Abeni
@ 2021-07-26 10:55 ` Paolo Abeni
2021-07-27 4:27 ` Geliang Tang
2021-07-26 10:55 ` [PATCH v3 mptcp-next 2/3] mptcp: optimize out option generation Paolo Abeni
` (3 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Paolo Abeni @ 2021-07-26 10:55 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
Commit message to be updated appending:
"""
Additionally, always drop the other suboption for TCP pure ack:
that makes both the code simpler and the MPTCP behaviour more
consistent.
"""
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/pm.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 4d1828fd2482..cd2323e5ccdc 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 a
+ * plain dup-ack from TCP perspective. The other MPTCP-relevant info,
+ * if any, will be carried by the 'original' TCP ack
+ */
+ if (skb && skb_is_tcp_pure_ack(skb)) {
remaining += opt_size;
*drop_other_suboptions = true;
}
--
2.26.3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 mptcp-next 2/3] mptcp: optimize out option generation
2021-07-26 10:55 [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization Paolo Abeni
2021-07-26 10:55 ` [PATCH v3 mptcp-next 1/3] Squash-to: "mptcp: move drop_other_suboptions check under pm lock" Paolo Abeni
@ 2021-07-26 10:55 ` Paolo Abeni
2021-07-27 4:33 ` Geliang Tang
2021-07-26 10:55 ` [PATCH v3 mptcp-next 3/3] mptcp: shrink mptcp_out_options struct Paolo Abeni
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Paolo Abeni @ 2021-07-26 10:55 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
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>
--
v1 -> v2:
- moved the pm.c chunk into the previous patch
- avoid changing 'len' type - Geliang
- add a comment describing why we need to clear ahmac field
- don't clear ext_copy fields anymore in mptcp_pm_add_addr_signal()
not needed and confusing
side node: we could probably optimize the conditionals in
mptcp_established_options()
---
net/mptcp/options.c | 227 +++++++++++++++++++++++--------------------
net/mptcp/protocol.h | 1 +
2 files changed, 120 insertions(+), 108 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index eafdb9408f3a..c14314268b36 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 */
@@ -682,8 +684,13 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
if (drop_other_suboptions) {
pr_debug("drop other suboptions");
opts->suboptions = 0;
- opts->ext_copy.use_ack = 0;
- opts->ext_copy.use_map = 0;
+
+ /* note that e.g. DSS could have written into the memory
+ * aliased by ahmac, we must reset the field here
+ * to avoid appending the hmac even for ADD_ADDR echo
+ * options
+ */
+ opts->ahmac = 0;
*size -= opt_size;
}
opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
@@ -735,7 +742,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 +1210,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 +1324,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 +1407,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 +1440,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/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] 9+ messages in thread
* [PATCH v3 mptcp-next 3/3] mptcp: shrink mptcp_out_options struct
2021-07-26 10:55 [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization Paolo Abeni
2021-07-26 10:55 ` [PATCH v3 mptcp-next 1/3] Squash-to: "mptcp: move drop_other_suboptions check under pm lock" Paolo Abeni
2021-07-26 10:55 ` [PATCH v3 mptcp-next 2/3] mptcp: optimize out option generation Paolo Abeni
@ 2021-07-26 10:55 ` Paolo Abeni
2021-07-28 0:40 ` [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization Mat Martineau
2021-07-28 8:09 ` Matthieu Baerts
4 siblings, 0 replies; 9+ messages in thread
From: Paolo Abeni @ 2021-07-26 10:55 UTC (permalink / raw)
To: mptcp; +Cc: Geliang Tang
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] 9+ messages in thread
* Re: [PATCH v3 mptcp-next 1/3] Squash-to: "mptcp: move drop_other_suboptions check under pm lock"
2021-07-26 10:55 ` [PATCH v3 mptcp-next 1/3] Squash-to: "mptcp: move drop_other_suboptions check under pm lock" Paolo Abeni
@ 2021-07-27 4:27 ` Geliang Tang
0 siblings, 0 replies; 9+ messages in thread
From: Geliang Tang @ 2021-07-27 4:27 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Hi Paolo,
Thanks for this patch, it looks good to me.
Paolo Abeni <pabeni@redhat.com> 于2021年7月26日周一 下午6:55写道：
>
> Commit message to be updated appending:
>
> """
> Additionally, always drop the other suboption for TCP pure ack:
> that makes both the code simpler and the MPTCP behaviour more
> consistent.
> """
>
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/pm.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
> index 4d1828fd2482..cd2323e5ccdc 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 a
> + * plain dup-ack from TCP perspective. The other MPTCP-relevant info,
> + * if any, will be carried by the 'original' TCP ack
> + */
> + if (skb && skb_is_tcp_pure_ack(skb)) {
> remaining += opt_size;
> *drop_other_suboptions = true;
> }
If this patch is to be squashed to "mptcp: move drop_other_suboptions check
under pm lock", should it be like this:
@@ -264,10 +264,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 ((mptcp_pm_should_add_signal_ipv6(msk) ||
- mptcp_pm_should_add_signal_port(msk) ||
- mptcp_pm_should_add_signal_echo(msk)) &&
- skb && skb_is_tcp_pure_ack(skb)) {
+ /* always drop every other options for pure ack ADD_ADDR; this is a
+ * plain dup-ack from TCP perspective. The other MPTCP-relevant info,
+ * if any, will be carried by the 'original' TCP ack
+ */
+ if (skb && skb_is_tcp_pure_ack(skb)) {
remaining += opt_size;
*drop_other_suboptions = true;
}
Thanks,
-Geliang
> --
> 2.26.3
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 mptcp-next 2/3] mptcp: optimize out option generation
2021-07-26 10:55 ` [PATCH v3 mptcp-next 2/3] mptcp: optimize out option generation Paolo Abeni
@ 2021-07-27 4:33 ` Geliang Tang
0 siblings, 0 replies; 9+ messages in thread
From: Geliang Tang @ 2021-07-27 4:33 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Paolo Abeni <pabeni@redhat.com> 于2021年7月26日周一 下午6:55写道：
>
> 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>
> --
> v1 -> v2:
> - moved the pm.c chunk into the previous patch
> - avoid changing 'len' type - Geliang
> - add a comment describing why we need to clear ahmac field
> - don't clear ext_copy fields anymore in mptcp_pm_add_addr_signal()
> not needed and confusing
>
> side node: we could probably optimize the conditionals in
> mptcp_established_options()
> ---
> net/mptcp/options.c | 227 +++++++++++++++++++++++--------------------
> net/mptcp/protocol.h | 1 +
> 2 files changed, 120 insertions(+), 108 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index eafdb9408f3a..c14314268b36 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 */
> @@ -682,8 +684,13 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff *
> if (drop_other_suboptions) {
> pr_debug("drop other suboptions");
> opts->suboptions = 0;
> - opts->ext_copy.use_ack = 0;
> - opts->ext_copy.use_map = 0;
> +
> + /* note that e.g. DSS could have written into the memory
> + * aliased by ahmac, we must reset the field here
> + * to avoid appending the hmac even for ADD_ADDR echo
> + * options
> + */
> + opts->ahmac = 0;
> *size -= opt_size;
> }
> opts->suboptions |= OPTION_MPTCP_ADD_ADDR;
> @@ -735,7 +742,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 +1210,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 +1324,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;
This will conflict with: (Squash to "Squash-to: mptcp: build ADD_ADDR/
echo-ADD_ADDR option according pm.add_signal"), since this line is removed
in it.
Thanks,
-Geliang
> u8 len = TCPOLEN_MPTCP_ADD_ADDR_BASE;
> u8 echo = MPTCP_ADDR_ECHO;
> @@ -1308,6 +1407,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 +1440,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/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] 9+ messages in thread
* Re: [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization
2021-07-26 10:55 [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization Paolo Abeni
` (2 preceding siblings ...)
2021-07-26 10:55 ` [PATCH v3 mptcp-next 3/3] mptcp: shrink mptcp_out_options struct Paolo Abeni
@ 2021-07-28 0:40 ` Mat Martineau
2021-07-28 8:09 ` Matthieu Baerts
4 siblings, 0 replies; 9+ messages in thread
From: Mat Martineau @ 2021-07-28 0:40 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp, Geliang Tang
On Mon, 26 Jul 2021, Paolo Abeni wrote:
> 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.
>
> This iteration covers the feedback provided by Geliang (many thanks!)
> on v1:
> - some code moved to new squash-to patch 1/3
> - some cleanup in 2/3
>
> please see the individual patch changelog for the details
>
> Paolo Abeni (3):
> Squash-to: "mptcp: move drop_other_suboptions check under pm lock"
> mptcp: optimize out option generation
> mptcp: shrink mptcp_out_options struct
>
> include/net/mptcp.h | 26 +++--
> net/mptcp/options.c | 227 +++++++++++++++++++++++--------------------
> net/mptcp/pm.c | 9 +-
> net/mptcp/protocol.h | 1 +
> 4 files changed, 142 insertions(+), 121 deletions(-)
>
> --
> 2.26.3
Hi Paolo -
This refactoring looks good to me, and the tests pass on my system. Thanks
for shrinking mptcp_out_options!
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization
2021-07-26 10:55 [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization Paolo Abeni
` (3 preceding siblings ...)
2021-07-28 0:40 ` [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization Mat Martineau
@ 2021-07-28 8:09 ` Matthieu Baerts
2021-07-28 10:46 ` Paolo Abeni
4 siblings, 1 reply; 9+ messages in thread
From: Matthieu Baerts @ 2021-07-28 8:09 UTC (permalink / raw)
To: Paolo Abeni, mptcp; +Cc: Geliang Tang
Hi Paolo, Geliang, Mat,
On 26/07/2021 12:55, Paolo Abeni wrote:
> 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
Should we close any of them? e.g. the issue #15?
> The final goal is shrink the mptcp_out_options struct, which is
> always zeroed by the plain TCP protocol in the output path.
Thank you for looking at that!
> This iteration covers the feedback provided by Geliang (many thanks!)
> on v1:
> - some code moved to new squash-to patch 1/3
> - some cleanup in 2/3
And thanks Geliang for the notes about the conflicts!
> please see the individual patch changelog for the details
>
> Paolo Abeni (3):
> Squash-to: "mptcp: move drop_other_suboptions check under pm lock"
> mptcp: optimize out option generation
> mptcp: shrink mptcp_out_options struct
Thank you for the patches, reviews and tests!
I split the series in two: the squash-to patch first and then the two
new patches.
Part 1: squash-to + update commit msg:
- 9030cfa001f6: "squashed" (with conflicts) in "mptcp: move
drop_other_suboptions check under pm lock"
- fb6c1d39264e: "Signed-off-by" + "Co-developed-by"
- b6a51f709e5b: tg: update msg after having applied 1 squash-to patch
- a4f59d64a674: conflict in
t/mptcp-remove-MPTCP_ADD_ADDR_IPV6-and-MPTCP_ADD_ADDR_PORT
- Results: 49320b30c955..14a78d3af24a
Builds and tests are now in progress:
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210728T074049
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210728T074049
Part 2: new patches with Mat's RvB tag and a few checkpatch fixes:
- ef6b13c8bbad: mptcp: optimize out option generation
- 7547285ef776: mptcp: shrink mptcp_out_options struct
- Results: 14a78d3af24a..4c599993729d
Builds and tests are now in progress:
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210728T080904
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export/20210728T080904
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization
2021-07-28 8:09 ` Matthieu Baerts
@ 2021-07-28 10:46 ` Paolo Abeni
0 siblings, 0 replies; 9+ messages in thread
From: Paolo Abeni @ 2021-07-28 10:46 UTC (permalink / raw)
To: Matthieu Baerts, mptcp; +Cc: Geliang Tang
On Wed, 2021-07-28 at 10:09 +0200, Matthieu Baerts wrote:
> Hi Paolo, Geliang, Mat,
>
> On 26/07/2021 12:55, Paolo Abeni wrote:
> > 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
>
> Should we close any of them? e.g. the issue #15?
I think we can close #15. I also think we can keep #42 open - not only
for hhgttg's sake - becuse looking at the MP_FAIL series it seams we
have a few more conditional we can get rid of in the input path.
Cheers,
Paolo
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-28 10:46 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-26 10:55 [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization Paolo Abeni
2021-07-26 10:55 ` [PATCH v3 mptcp-next 1/3] Squash-to: "mptcp: move drop_other_suboptions check under pm lock" Paolo Abeni
2021-07-27 4:27 ` Geliang Tang
2021-07-26 10:55 ` [PATCH v3 mptcp-next 2/3] mptcp: optimize out option generation Paolo Abeni
2021-07-27 4:33 ` Geliang Tang
2021-07-26 10:55 ` [PATCH v3 mptcp-next 3/3] mptcp: shrink mptcp_out_options struct Paolo Abeni
2021-07-28 0:40 ` [PATCH v3 mptcp-next 0/3] mptcp: minor out optimization Mat Martineau
2021-07-28 8:09 ` Matthieu Baerts
2021-07-28 10:46 ` 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).