* [MPTCP][PATCH mptcp-next] mptcp: use unlikely for non-DSS suboptions
@ 2021-08-12 7:09 Geliang Tang
2021-08-12 9:37 ` Paolo Abeni
0 siblings, 1 reply; 3+ messages in thread
From: Geliang Tang @ 2021-08-12 7:09 UTC (permalink / raw)
To: mptcp, geliangtang; +Cc: Geliang Tang, Paolo Abeni
From: Geliang Tang <geliangtang@xiaomi.com>
Usually we get a single MPTCP subopt per packet: a DSS. So we could
optimize the other suboptions code path with something alike:
if (unlikely(any subopt other than dss is present))
// go checking all of them individually
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Geliang Tang <geliangtang@xiaomi.com>
---
net/mptcp/options.c | 75 +++++++++++++++++++++++----------------------
1 file changed, 39 insertions(+), 36 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index eaee69df6635..7e8019ad8542 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -1123,49 +1123,52 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
if (!check_fully_established(msk, sk, subflow, skb, &mp_opt))
return sk->sk_state != TCP_CLOSE;
- if (mp_opt.fastclose &&
- msk->local_key == mp_opt.rcvr_key) {
- WRITE_ONCE(msk->rcv_fastclose, true);
- mptcp_schedule_work((struct sock *)msk);
- }
-
- if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) {
- if (!mp_opt.echo) {
- mptcp_pm_add_addr_received(msk, &mp_opt.addr);
- MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR);
- } else {
- mptcp_pm_add_addr_echoed(msk, &mp_opt.addr);
- mptcp_pm_del_add_timer(msk, &mp_opt.addr, true);
- MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD);
+ if (unlikely(mp_opt.fastclose || mp_opt.add_addr || mp_opt.rm_addr ||
+ mp_opt.mp_prio || mp_opt.mp_fail || mp_opt.reset)) {
+ if (mp_opt.fastclose &&
+ msk->local_key == mp_opt.rcvr_key) {
+ WRITE_ONCE(msk->rcv_fastclose, true);
+ mptcp_schedule_work((struct sock *)msk);
}
- if (mp_opt.addr.port)
- MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_PORTADD);
+ if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) {
+ if (!mp_opt.echo) {
+ mptcp_pm_add_addr_received(msk, &mp_opt.addr);
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR);
+ } else {
+ mptcp_pm_add_addr_echoed(msk, &mp_opt.addr);
+ mptcp_pm_del_add_timer(msk, &mp_opt.addr, true);
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD);
+ }
- mp_opt.add_addr = 0;
- }
+ if (mp_opt.addr.port)
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_PORTADD);
- if (mp_opt.rm_addr) {
- mptcp_pm_rm_addr_received(msk, &mp_opt.rm_list);
- mp_opt.rm_addr = 0;
- }
+ mp_opt.add_addr = 0;
+ }
- if (mp_opt.mp_prio) {
- mptcp_pm_mp_prio_received(sk, mp_opt.backup);
- MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIORX);
- mp_opt.mp_prio = 0;
- }
+ if (mp_opt.rm_addr) {
+ mptcp_pm_rm_addr_received(msk, &mp_opt.rm_list);
+ mp_opt.rm_addr = 0;
+ }
- if (mp_opt.mp_fail) {
- mptcp_pm_mp_fail_received(sk, mp_opt.fail_seq);
- MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILRX);
- mp_opt.mp_fail = 0;
- }
+ if (mp_opt.mp_prio) {
+ mptcp_pm_mp_prio_received(sk, mp_opt.backup);
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIORX);
+ mp_opt.mp_prio = 0;
+ }
- if (mp_opt.reset) {
- subflow->reset_seen = 1;
- subflow->reset_reason = mp_opt.reset_reason;
- subflow->reset_transient = mp_opt.reset_transient;
+ if (mp_opt.mp_fail) {
+ mptcp_pm_mp_fail_received(sk, mp_opt.fail_seq);
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILRX);
+ mp_opt.mp_fail = 0;
+ }
+
+ if (mp_opt.reset) {
+ subflow->reset_seen = 1;
+ subflow->reset_reason = mp_opt.reset_reason;
+ subflow->reset_transient = mp_opt.reset_transient;
+ }
}
if (!mp_opt.dss)
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [MPTCP][PATCH mptcp-next] mptcp: use unlikely for non-DSS suboptions 2021-08-12 7:09 [MPTCP][PATCH mptcp-next] mptcp: use unlikely for non-DSS suboptions Geliang Tang @ 2021-08-12 9:37 ` Paolo Abeni 2021-08-12 9:52 ` Geliang Tang 0 siblings, 1 reply; 3+ messages in thread From: Paolo Abeni @ 2021-08-12 9:37 UTC (permalink / raw) To: Geliang Tang, mptcp; +Cc: Geliang Tang Hello, On Thu, 2021-08-12 at 15:09 +0800, Geliang Tang wrote: > From: Geliang Tang <geliangtang@xiaomi.com> > > Usually we get a single MPTCP subopt per packet: a DSS. So we could > optimize the other suboptions code path with something alike: > > if (unlikely(any subopt other than dss is present)) > // go checking all of them individually > > Suggested-by: Paolo Abeni <pabeni@redhat.com> > Signed-off-by: Geliang Tang <geliangtang@xiaomi.com> > --- > net/mptcp/options.c | 75 +++++++++++++++++++++++---------------------- > 1 file changed, 39 insertions(+), 36 deletions(-) > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index eaee69df6635..7e8019ad8542 100644 > --- a/net/mptcp/options.c > +++ b/net/mptcp/options.c > @@ -1123,49 +1123,52 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) > if (!check_fully_established(msk, sk, subflow, skb, &mp_opt)) > return sk->sk_state != TCP_CLOSE; > > - if (mp_opt.fastclose && > - msk->local_key == mp_opt.rcvr_key) { > - WRITE_ONCE(msk->rcv_fastclose, true); > - mptcp_schedule_work((struct sock *)msk); > - } > - > - if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) { > - if (!mp_opt.echo) { > - mptcp_pm_add_addr_received(msk, &mp_opt.addr); > - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR); > - } else { > - mptcp_pm_add_addr_echoed(msk, &mp_opt.addr); > - mptcp_pm_del_add_timer(msk, &mp_opt.addr, true); > - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD); > + if (unlikely(mp_opt.fastclose || mp_opt.add_addr || mp_opt.rm_addr || > + mp_opt.mp_prio || mp_opt.mp_fail || mp_opt.reset)) { > + if (mp_opt.fastclose && > + msk->local_key == mp_opt.rcvr_key) { > + WRITE_ONCE(msk->rcv_fastclose, true); > + mptcp_schedule_work((struct sock *)msk); > } > > - if (mp_opt.addr.port) > - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_PORTADD); > + if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) { > + if (!mp_opt.echo) { > + mptcp_pm_add_addr_received(msk, &mp_opt.addr); > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR); > + } else { > + mptcp_pm_add_addr_echoed(msk, &mp_opt.addr); > + mptcp_pm_del_add_timer(msk, &mp_opt.addr, true); > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD); > + } > > - mp_opt.add_addr = 0; > - } > + if (mp_opt.addr.port) > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_PORTADD); > > - if (mp_opt.rm_addr) { > - mptcp_pm_rm_addr_received(msk, &mp_opt.rm_list); > - mp_opt.rm_addr = 0; > - } > + mp_opt.add_addr = 0; > + } > > - if (mp_opt.mp_prio) { > - mptcp_pm_mp_prio_received(sk, mp_opt.backup); > - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIORX); > - mp_opt.mp_prio = 0; > - } > + if (mp_opt.rm_addr) { > + mptcp_pm_rm_addr_received(msk, &mp_opt.rm_list); > + mp_opt.rm_addr = 0; > + } > > - if (mp_opt.mp_fail) { > - mptcp_pm_mp_fail_received(sk, mp_opt.fail_seq); > - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILRX); > - mp_opt.mp_fail = 0; > - } > + if (mp_opt.mp_prio) { > + mptcp_pm_mp_prio_received(sk, mp_opt.backup); > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIORX); > + mp_opt.mp_prio = 0; > + } > > - if (mp_opt.reset) { > - subflow->reset_seen = 1; > - subflow->reset_reason = mp_opt.reset_reason; > - subflow->reset_transient = mp_opt.reset_transient; > + if (mp_opt.mp_fail) { > + mptcp_pm_mp_fail_received(sk, mp_opt.fail_seq); > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILRX); > + mp_opt.mp_fail = 0; > + } > + > + if (mp_opt.reset) { > + subflow->reset_seen = 1; > + subflow->reset_reason = mp_opt.reset_reason; > + subflow->reset_transient = mp_opt.reset_transient; > + } > } > > if (!mp_opt.dss) I think with a slighly larger change, we can trim completely a lot of conditionals, specifically: - replace all the 'opt.reset', 'opt.mp_fail', 'opt.dss' etc. fields with a bitmask 'suboptions' - in mptcp_incoming_options() if (unlikely(opt.suboptions != BIT(OPT_DSS_WHATEVER_WILL_BE_CALLED))) { // check the individual bits if (!(opt.suboptions & BIT(OPT_DSS_WHATEVER_WILL_BE_CALLED))) return; } // here DSS will be always set. With the above, for the most common scenario, we will have a single 'if' statement in the fastpath (and that will use branch prediction hint, too) I've the patch half-cooked, if I find some time to finish them... Cheers, Paolo ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [MPTCP][PATCH mptcp-next] mptcp: use unlikely for non-DSS suboptions 2021-08-12 9:37 ` Paolo Abeni @ 2021-08-12 9:52 ` Geliang Tang 0 siblings, 0 replies; 3+ messages in thread From: Geliang Tang @ 2021-08-12 9:52 UTC (permalink / raw) To: Paolo Abeni; +Cc: MPTCP Upstream, Geliang Tang Paolo Abeni <pabeni@redhat.com> 于2021年8月12日周四 下午5:37写道： > > Hello, > > On Thu, 2021-08-12 at 15:09 +0800, Geliang Tang wrote: > > From: Geliang Tang <geliangtang@xiaomi.com> > > > > Usually we get a single MPTCP subopt per packet: a DSS. So we could > > optimize the other suboptions code path with something alike: > > > > if (unlikely(any subopt other than dss is present)) > > // go checking all of them individually > > > > Suggested-by: Paolo Abeni <pabeni@redhat.com> > > Signed-off-by: Geliang Tang <geliangtang@xiaomi.com> > > --- > > net/mptcp/options.c | 75 +++++++++++++++++++++++---------------------- > > 1 file changed, 39 insertions(+), 36 deletions(-) > > > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > > index eaee69df6635..7e8019ad8542 100644 > > --- a/net/mptcp/options.c > > +++ b/net/mptcp/options.c > > @@ -1123,49 +1123,52 @@ bool mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) > > if (!check_fully_established(msk, sk, subflow, skb, &mp_opt)) > > return sk->sk_state != TCP_CLOSE; > > > > - if (mp_opt.fastclose && > > - msk->local_key == mp_opt.rcvr_key) { > > - WRITE_ONCE(msk->rcv_fastclose, true); > > - mptcp_schedule_work((struct sock *)msk); > > - } > > - > > - if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) { > > - if (!mp_opt.echo) { > > - mptcp_pm_add_addr_received(msk, &mp_opt.addr); > > - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR); > > - } else { > > - mptcp_pm_add_addr_echoed(msk, &mp_opt.addr); > > - mptcp_pm_del_add_timer(msk, &mp_opt.addr, true); > > - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD); > > + if (unlikely(mp_opt.fastclose || mp_opt.add_addr || mp_opt.rm_addr || > > + mp_opt.mp_prio || mp_opt.mp_fail || mp_opt.reset)) { > > + if (mp_opt.fastclose && > > + msk->local_key == mp_opt.rcvr_key) { > > + WRITE_ONCE(msk->rcv_fastclose, true); > > + mptcp_schedule_work((struct sock *)msk); > > } > > > > - if (mp_opt.addr.port) > > - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_PORTADD); > > + if (mp_opt.add_addr && add_addr_hmac_valid(msk, &mp_opt)) { > > + if (!mp_opt.echo) { > > + mptcp_pm_add_addr_received(msk, &mp_opt.addr); > > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR); > > + } else { > > + mptcp_pm_add_addr_echoed(msk, &mp_opt.addr); > > + mptcp_pm_del_add_timer(msk, &mp_opt.addr, true); > > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD); > > + } > > > > - mp_opt.add_addr = 0; > > - } > > + if (mp_opt.addr.port) > > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_PORTADD); > > > > - if (mp_opt.rm_addr) { > > - mptcp_pm_rm_addr_received(msk, &mp_opt.rm_list); > > - mp_opt.rm_addr = 0; > > - } > > + mp_opt.add_addr = 0; > > + } > > > > - if (mp_opt.mp_prio) { > > - mptcp_pm_mp_prio_received(sk, mp_opt.backup); > > - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIORX); > > - mp_opt.mp_prio = 0; > > - } > > + if (mp_opt.rm_addr) { > > + mptcp_pm_rm_addr_received(msk, &mp_opt.rm_list); > > + mp_opt.rm_addr = 0; > > + } > > > > - if (mp_opt.mp_fail) { > > - mptcp_pm_mp_fail_received(sk, mp_opt.fail_seq); > > - MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILRX); > > - mp_opt.mp_fail = 0; > > - } > > + if (mp_opt.mp_prio) { > > + mptcp_pm_mp_prio_received(sk, mp_opt.backup); > > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPPRIORX); > > + mp_opt.mp_prio = 0; > > + } > > > > - if (mp_opt.reset) { > > - subflow->reset_seen = 1; > > - subflow->reset_reason = mp_opt.reset_reason; > > - subflow->reset_transient = mp_opt.reset_transient; > > + if (mp_opt.mp_fail) { > > + mptcp_pm_mp_fail_received(sk, mp_opt.fail_seq); > > + MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILRX); > > + mp_opt.mp_fail = 0; > > + } > > + > > + if (mp_opt.reset) { > > + subflow->reset_seen = 1; > > + subflow->reset_reason = mp_opt.reset_reason; > > + subflow->reset_transient = mp_opt.reset_transient; > > + } > > } > > > > if (!mp_opt.dss) > > I think with a slighly larger change, we can trim completely a lot of > conditionals, specifically: > - replace all the 'opt.reset', 'opt.mp_fail', 'opt.dss' etc. fields > with a bitmask 'suboptions' > > - in mptcp_incoming_options() > if (unlikely(opt.suboptions != BIT(OPT_DSS_WHATEVER_WILL_BE_CALLED))) { > // check the individual bits > > if (!(opt.suboptions & BIT(OPT_DSS_WHATEVER_WILL_BE_CALLED))) > return; > } > > // here DSS will be always set. > > With the above, for the most common scenario, we will have a single > 'if' statement in the fastpath (and that will use branch prediction > hint, too) > > I've the patch half-cooked, if I find some time to finish them... Thanks Paolo, let's wait for your patch. If you need any help from me to finish or test your patch, I will be happy to do it. Thanks, -Geliang > > Cheers, > > Paolo > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-08-12 9:52 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-12 7:09 [MPTCP][PATCH mptcp-next] mptcp: use unlikely for non-DSS suboptions Geliang Tang 2021-08-12 9:37 ` Paolo Abeni 2021-08-12 9:52 ` Geliang Tang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.