* [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.