[PATCH] net/smc: avoid atomic_set and smp_wmb in the tx path when possible

[PATCH] net/smc: avoid atomic_set and smp_wmb in the tx path when possible

[Li RongQing]

these is less opportunity that conn->tx_pushing is not 1, since
tx_pushing is just checked with 1, so move the setting tx_pushing
to 1 after atomic_dec_and_test() return false, to avoid atomic_set
and smp_wmb in tx path when possible

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 net/smc/smc_tx.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
index 3b0ff3b..72dbdee 100644
--- a/net/smc/smc_tx.c
+++ b/net/smc/smc_tx.c
@@ -667,8 +667,6 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn)
 		return 0;
 
 again:
-	atomic_set(&conn->tx_pushing, 1);
-	smp_wmb(); /* Make sure tx_pushing is 1 before real send */
 	rc = __smc_tx_sndbuf_nonempty(conn);
 
 	/* We need to check whether someone else have added some data into
@@ -677,8 +675,11 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn)
 	 * If so, we need to push again to prevent those data hang in the send
 	 * queue.
 	 */
-	if (unlikely(!atomic_dec_and_test(&conn->tx_pushing)))
+	if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) {
+		atomic_set(&conn->tx_pushing, 1);
+		smp_wmb(); /* Make sure tx_pushing is 1 before real send */
 		goto again;
+	}
 
 	return rc;
 }
-- 
2.9.4

Re: [PATCH] net/smc: avoid atomic_set and smp_wmb in the tx path when possible

[Dust Li]

On Thu, Nov 02, 2023 at 05:27:12PM +0800, Li RongQing wrote:
>these is less opportunity that conn->tx_pushing is not 1, since

these -> there ?

>tx_pushing is just checked with 1, so move the setting tx_pushing
>to 1 after atomic_dec_and_test() return false, to avoid atomic_set
>and smp_wmb in tx path when possible

The patch should add [PATCH net-next] subject-prefix since this is an optimization.

Besides, do you have any performance number ?

Thanks

>
>Signed-off-by: Li RongQing <lirongqing@baidu.com>
>---
> net/smc/smc_tx.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
>index 3b0ff3b..72dbdee 100644
>--- a/net/smc/smc_tx.c
>+++ b/net/smc/smc_tx.c
>@@ -667,8 +667,6 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn)
> 		return 0;
> 
> again:
>-	atomic_set(&conn->tx_pushing, 1);
>-	smp_wmb(); /* Make sure tx_pushing is 1 before real send */
> 	rc = __smc_tx_sndbuf_nonempty(conn);
> 
> 	/* We need to check whether someone else have added some data into
>@@ -677,8 +675,11 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn)
> 	 * If so, we need to push again to prevent those data hang in the send
> 	 * queue.
> 	 */
>-	if (unlikely(!atomic_dec_and_test(&conn->tx_pushing)))
>+	if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) {
>+		atomic_set(&conn->tx_pushing, 1);
>+		smp_wmb(); /* Make sure tx_pushing is 1 before real send */
> 		goto again;
>+	}
> 
> 	return rc;
> }
>-- 
>2.9.4

Re: [PATCH] net/smc: avoid atomic_set and smp_wmb in the tx path when possible

[Wenjia Zhang]

On 02.11.23 10:27, Li RongQing wrote:
> these is less opportunity that conn->tx_pushing is not 1, since
> tx_pushing is just checked with 1, so move the setting tx_pushing
> to 1 after atomic_dec_and_test() return false, to avoid atomic_set
> and smp_wmb in tx path when possible
> 
I think we should avoid to use argument like "less opportunity" in 
commit message. Because "less opportunity" does not mean "no 
opportunity". Once it occurs, does it mean that what the patch changes 
is useless or wrong?

> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>   net/smc/smc_tx.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c
> index 3b0ff3b..72dbdee 100644
> --- a/net/smc/smc_tx.c
> +++ b/net/smc/smc_tx.c
> @@ -667,8 +667,6 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn)
>   		return 0;
>   
>   again:
> -	atomic_set(&conn->tx_pushing, 1);
> -	smp_wmb(); /* Make sure tx_pushing is 1 before real send */
>   	rc = __smc_tx_sndbuf_nonempty(conn);
>   
>   	/* We need to check whether someone else have added some data into
> @@ -677,8 +675,11 @@ int smc_tx_sndbuf_nonempty(struct smc_connection *conn)
>   	 * If so, we need to push again to prevent those data hang in the send
>   	 * queue.
>   	 */
> -	if (unlikely(!atomic_dec_and_test(&conn->tx_pushing)))
> +	if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) {
> +		atomic_set(&conn->tx_pushing, 1);
> +		smp_wmb(); /* Make sure tx_pushing is 1 before real send */
>   		goto again;
> +	}
>   
>   	return rc;
>   }
I'm afraid that the *if* statement would never be true, without setting 
the value of &conn->tx_pushing firstly.

RE: [PATCH] net/smc: avoid atomic_set and smp_wmb in the tx path when possible

[Li,Rongqing]

> -----Original Message-----
> From: Dust Li <dust.li@linux.alibaba.com>
> Sent: Thursday, November 2, 2023 10:54 PM
> To: Li,Rongqing <lirongqing@baidu.com>; linux-s390@vger.kernel.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH] net/smc: avoid atomic_set and smp_wmb in the tx path
> when possible
> 
> On Thu, Nov 02, 2023 at 05:27:12PM +0800, Li RongQing wrote:
> >these is less opportunity that conn->tx_pushing is not 1, since
> 
> these -> there ?

Yes, thanks

> 
> >tx_pushing is just checked with 1, so move the setting tx_pushing to 1
> >after atomic_dec_and_test() return false, to avoid atomic_set and
> >smp_wmb in tx path when possible
> 
> The patch should add [PATCH net-next] subject-prefix since this is an
> optimization.
> 

OK

> Besides, do you have any performance number ?

Just try a simple performance test,  seems same.

-Li

RE: [PATCH] net/smc: avoid atomic_set and smp_wmb in the tx path when possible

[Li,Rongqing]

> -----Original Message-----
> From: Wenjia Zhang <wenjia@linux.ibm.com>
> Sent: Friday, November 3, 2023 4:42 AM
> To: Li,Rongqing <lirongqing@baidu.com>
> Cc: linux-s390@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH] net/smc: avoid atomic_set and smp_wmb in the tx path
> when possible
> 
> 
> 
> On 02.11.23 10:27, Li RongQing wrote:
> > these is less opportunity that conn->tx_pushing is not 1, since
> > tx_pushing is just checked with 1, so move the setting tx_pushing to 1
> > after atomic_dec_and_test() return false, to avoid atomic_set and
> > smp_wmb in tx path when possible
> >
> I think we should avoid to use argument like "less opportunity" in commit
> message. Because "less opportunity" does not mean "no opportunity". Once it
> occurs, does it mean that what the patch changes is useless or wrong?
> 

I will reword the message.
I think this is a question of probability. even tx_pushing is not 1, this is still not a problem, atomic_dec_and_test(&conn->tx_pushing) will return false, transmit will be looped again, and tx_pushing will be added at any time

> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> >   net/smc/smc_tx.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/smc/smc_tx.c b/net/smc/smc_tx.c index
> > 3b0ff3b..72dbdee 100644
> > --- a/net/smc/smc_tx.c
> > +++ b/net/smc/smc_tx.c
> > @@ -667,8 +667,6 @@ int smc_tx_sndbuf_nonempty(struct smc_connection
> *conn)
> >   		return 0;
> >
> >   again:
> > -	atomic_set(&conn->tx_pushing, 1);
> > -	smp_wmb(); /* Make sure tx_pushing is 1 before real send */
> >   	rc = __smc_tx_sndbuf_nonempty(conn);
> >
> >   	/* We need to check whether someone else have added some data
> into
> > @@ -677,8 +675,11 @@ int smc_tx_sndbuf_nonempty(struct
> smc_connection *conn)
> >   	 * If so, we need to push again to prevent those data hang in the send
> >   	 * queue.
> >   	 */
> > -	if (unlikely(!atomic_dec_and_test(&conn->tx_pushing)))
> > +	if (unlikely(!atomic_dec_and_test(&conn->tx_pushing))) {
> > +		atomic_set(&conn->tx_pushing, 1);
> > +		smp_wmb(); /* Make sure tx_pushing is 1 before real send */
> >   		goto again;
> > +	}
> >
> >   	return rc;
> >   }
> I'm afraid that the *if* statement would never be true, without setting the
> value of &conn->tx_pushing firstly.

I think conn->tx_pushing do not need to be set in this condition, and this patch is trying to avoid setting it 

Thanks

-Li