netdev - Re: [PATCH] net/smc: avoid atomic_set and smp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4b1c9303-9ad1-42f3-a1a2-b9ccfcafd022@linux.ibm.com>
Date: Thu, 2 Nov 2023 21:42:27 +0100
From: Wenjia Zhang <wenjia@...ux.ibm.com>
To: Li RongQing <lirongqing@...du.com>
Cc: linux-s390@...r.kernel.org, netdev@...r.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?

> Signed-off-by: Li RongQing <lirongqing@...du.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.