diff mbox series

[mptcp-next] mptcp: use unlikely for non-DSS suboptions

Message ID e59a65d4bc669101b598c79e57ff425e4cf88d6e.1628752099.git.geliangtang@xiaomi.com (mailing list archive)
State Superseded, archived
Delegated to: Paolo Abeni
Headers show
Series [mptcp-next] mptcp: use unlikely for non-DSS suboptions | expand

Commit Message

Geliang Tang Aug. 12, 2021, 7:09 a.m. UTC 
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(-)

Comments

Paolo Abeni Aug. 12, 2021, 9:37 a.m. UTC | #1 
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
Geliang Tang Aug. 12, 2021, 9:52 a.m. UTC | #2 
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
>
diff mbox series

Patch

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)