[v3] BPF path manager, part 1

[PATCH mptcp-next v3 9/9] mptcp: drop useless "err = 0" in subflow_destroy

Posted by Geliang Tang 1 year, 3 months ago

From: Geliang Tang <tanggeliang@kylinos.cn>

Upon successful return, mptcp_pm_parse_addr() returns 0. There is no need
to set "err = 0" after this. So after mptcp_nl_find_ssk() returns, just
need to set "err = -ESRCH", then release and free msk socket if it returns
NULL.

Also, no need to define the veriable "subflow" in subflow_destroy(), use
mptcp_subflow_ctx(ssk) directly.

This patch doesn't change the behaviour of the code, just refactoring.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 net/mptcp/pm_userspace.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
index 07e0c7259494..8545212f023e 100644
--- a/net/mptcp/pm_userspace.c
+++ b/net/mptcp/pm_userspace.c
@@ -535,19 +535,18 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
 
 	lock_sock(sk);
 	ssk = mptcp_nl_find_ssk(msk, &addr_l.addr, &addr_r);
-	if (ssk) {
-		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
-
-		spin_lock_bh(&msk->pm.lock);
-		mptcp_userspace_pm_delete_local_addr(msk, &addr_l);
-		spin_unlock_bh(&msk->pm.lock);
-		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
-		mptcp_close_ssk(sk, ssk, subflow);
-		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
-		err = 0;
-	} else {
+	if (!ssk) {
 		err = -ESRCH;
+		release_sock(sk);
+		goto destroy_err;
 	}
+
+	spin_lock_bh(&msk->pm.lock);
+	mptcp_userspace_pm_delete_local_addr(msk, &addr_l);
+	spin_unlock_bh(&msk->pm.lock);
+	mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
+	mptcp_close_ssk(sk, ssk, mptcp_subflow_ctx(ssk));
+	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
 	release_sock(sk);
 
 destroy_err:
-- 
2.45.2

Re: [PATCH mptcp-next v3 9/9] mptcp: drop useless "err = 0" in subflow_destroy

Posted by Matthieu Baerts 1 year, 2 months ago

Hi Geliang,

On 07/11/2024 07:45, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Upon successful return, mptcp_pm_parse_addr() returns 0. There is no need
> to set "err = 0" after this. So after mptcp_nl_find_ssk() returns, just
> need to set "err = -ESRCH", then release and free msk socket if it returns
> NULL.
> 
> Also, no need to define the veriable "subflow" in subflow_destroy(), use
> mptcp_subflow_ctx(ssk) directly.
> 
> This patch doesn't change the behaviour of the code, just refactoring.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  net/mptcp/pm_userspace.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> index 07e0c7259494..8545212f023e 100644
> --- a/net/mptcp/pm_userspace.c
> +++ b/net/mptcp/pm_userspace.c
> @@ -535,19 +535,18 @@ int mptcp_pm_nl_subflow_destroy_doit(struct sk_buff *skb, struct genl_info *info
>  
>  	lock_sock(sk);
>  	ssk = mptcp_nl_find_ssk(msk, &addr_l.addr, &addr_r);
> -	if (ssk) {
> -		struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> -
> -		spin_lock_bh(&msk->pm.lock);
> -		mptcp_userspace_pm_delete_local_addr(msk, &addr_l);
> -		spin_unlock_bh(&msk->pm.lock);
> -		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
> -		mptcp_close_ssk(sk, ssk, subflow);
> -		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
> -		err = 0;

OK to drop 'err = 0' but...

> -	} else {
> +	if (!ssk) {
>  		err = -ESRCH;
> +		release_sock(sk);
> +		goto destroy_err;

... I think it is always best to reduce the number of exit path: so
here, I think it is best to add a new label before release_sock(sk)
below, instead of duplicating this release_sock(sk). Something like:

      ssk = mptcp_nl_find_ssk(...);
      if (!ssk) {
          err = -ESRCH;
          goto release_sock;
      }

      (...)

  release_sock:
      release_sock(sk);

  destroy_err:
      (...)

WDYT?

>  	}
> +
> +	spin_lock_bh(&msk->pm.lock);
> +	mptcp_userspace_pm_delete_local_addr(msk, &addr_l);
> +	spin_unlock_bh(&msk->pm.lock);
> +	mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN | SEND_SHUTDOWN);
> +	mptcp_close_ssk(sk, ssk, mptcp_subflow_ctx(ssk));
> +	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
>  	release_sock(sk);
>  
>  destroy_err:

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next v3 9/9] mptcp: drop useless "err = 0" in subflow_destroy

Posted by Geliang Tang 1 year, 2 months ago

On Wed, 2024-12-04 at 18:49 +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 07/11/2024 07:45, Geliang Tang wrote:
> > From: Geliang Tang <tanggeliang@kylinos.cn>
> > 
> > Upon successful return, mptcp_pm_parse_addr() returns 0. There is
> > no need
> > to set "err = 0" after this. So after mptcp_nl_find_ssk() returns,
> > just
> > need to set "err = -ESRCH", then release and free msk socket if it
> > returns
> > NULL.
> > 
> > Also, no need to define the veriable "subflow" in
> > subflow_destroy(), use
> > mptcp_subflow_ctx(ssk) directly.
> > 
> > This patch doesn't change the behaviour of the code, just
> > refactoring.
> > 
> > Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> > ---
> >  net/mptcp/pm_userspace.c | 21 ++++++++++-----------
> >  1 file changed, 10 insertions(+), 11 deletions(-)
> > 
> > diff --git a/net/mptcp/pm_userspace.c b/net/mptcp/pm_userspace.c
> > index 07e0c7259494..8545212f023e 100644
> > --- a/net/mptcp/pm_userspace.c
> > +++ b/net/mptcp/pm_userspace.c
> > @@ -535,19 +535,18 @@ int mptcp_pm_nl_subflow_destroy_doit(struct
> > sk_buff *skb, struct genl_info *info
> >  
> >  	lock_sock(sk);
> >  	ssk = mptcp_nl_find_ssk(msk, &addr_l.addr, &addr_r);
> > -	if (ssk) {
> > -		struct mptcp_subflow_context *subflow =
> > mptcp_subflow_ctx(ssk);
> > -
> > -		spin_lock_bh(&msk->pm.lock);
> > -		mptcp_userspace_pm_delete_local_addr(msk,
> > &addr_l);
> > -		spin_unlock_bh(&msk->pm.lock);
> > -		mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN |
> > SEND_SHUTDOWN);
> > -		mptcp_close_ssk(sk, ssk, subflow);
> > -		MPTCP_INC_STATS(sock_net(sk),
> > MPTCP_MIB_RMSUBFLOW);
> > -		err = 0;
> 
> OK to drop 'err = 0' but...
> 
> > -	} else {
> > +	if (!ssk) {
> >  		err = -ESRCH;
> > +		release_sock(sk);
> > +		goto destroy_err;
> 
> ... I think it is always best to reduce the number of exit path: so
> here, I think it is best to add a new label before release_sock(sk)
> below, instead of duplicating this release_sock(sk). Something like:
> 
>       ssk = mptcp_nl_find_ssk(...);
>       if (!ssk) {
>           err = -ESRCH;
>           goto release_sock;
>       }
> 
>       (...)
> 
>   release_sock:
>       release_sock(sk);
> 
>   destroy_err:
>       (...)
> 
> WDYT?

I agree. Updated in v4.

Thanks,
-Geliang

> 
> >  	}
> > +
> > +	spin_lock_bh(&msk->pm.lock);
> > +	mptcp_userspace_pm_delete_local_addr(msk, &addr_l);
> > +	spin_unlock_bh(&msk->pm.lock);
> > +	mptcp_subflow_shutdown(sk, ssk, RCV_SHUTDOWN |
> > SEND_SHUTDOWN);
> > +	mptcp_close_ssk(sk, ssk, mptcp_subflow_ctx(ssk));
> > +	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RMSUBFLOW);
> >  	release_sock(sk);
> >  
> >  destroy_err:
> 
> Cheers,
> Matt