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

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

[Li RongQing]

There is rare possibility 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

Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
diff v3: improvements in the commit body and comments
diff v2: fix a typo in commit body and add net-next subject-prefix
 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..2c2933f 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 send again */
 		goto again;
+	}
 
 	return rc;
 }
-- 
2.9.4

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

[Alexandra Winter]

On 17.11.23 12:16, Li RongQing wrote:
> There is rare possibility 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
> 
> Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> diff v3: improvements in the commit body and comments
> diff v2: fix a typo in commit body and add net-next subject-prefix
>  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..2c2933f 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 send again */
>  		goto again;
> +	}
>  
>  	return rc;
>  }

It seems to me that the purpose of conn->tx_pushing is
a) Serve as a mutex, so only one thread per conn will call __smc_tx_sndbuf_nonempty().
b) Repeat, in case some other thread has added data to sndbuf concurrently.

I agree that this patch does not change the behaviour of this function and removes an
atomic_set() in the likely path.

I wonder however: All callers of smc_tx_sndbuf_nonempty() must hold the socket lock.
So how can we ever run in a concurrency situation?
Is this handling of conn->tx_pushing necessary at all?

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

[Dust Li]

On Fri, Nov 17, 2023 at 01:27:57PM +0100, Alexandra Winter wrote:
>
>
>On 17.11.23 12:16, Li RongQing wrote:
>> There is rare possibility 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
>> 
>> Reviewed-by: Dust Li <dust.li@linux.alibaba.com>
>> Signed-off-by: Li RongQing <lirongqing@baidu.com>
>> ---
>> diff v3: improvements in the commit body and comments
>> diff v2: fix a typo in commit body and add net-next subject-prefix
>>  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..2c2933f 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 send again */
>>  		goto again;
>> +	}
>>  
>>  	return rc;
>>  }
>
>It seems to me that the purpose of conn->tx_pushing is
>a) Serve as a mutex, so only one thread per conn will call __smc_tx_sndbuf_nonempty().
>b) Repeat, in case some other thread has added data to sndbuf concurrently.
>
>I agree that this patch does not change the behaviour of this function and removes an
>atomic_set() in the likely path.
>
>I wonder however: All callers of smc_tx_sndbuf_nonempty() must hold the socket lock.
>So how can we ever run in a concurrency situation?
>Is this handling of conn->tx_pushing necessary at all?

Hi Sandy,

Overall, I think you are right. But there is something we need to take care.

Before commit 6b88af839d20 ("net/smc: don't send in the BH context if
sock_owned_by_user"), we used to call smc_tx_pending() in the soft IRQ,
without checking sock_owned_by_user(), which would caused a race condition
because bh_lock_sock() did not honor sock_lock(). To address this issue,
I have added the tx_pushing mechanism. However, with commit 6b88af839d20,
we now defer the transmission if sock_lock() is held by the user.
Therefore, there should no longer be a race condition. Nevertheless, if
we remove the tx_pending mechanism, we must always remember not to call
smc_tx_sndbuf_nonempty() in the soft IRQ when the user holds the sock lock.

Thanks
Dust

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

[Alexandra Winter]

On 20.11.23 04:20, Dust Li wrote:
>> It seems to me that the purpose of conn->tx_pushing is
>> a) Serve as a mutex, so only one thread per conn will call __smc_tx_sndbuf_nonempty().
>> b) Repeat, in case some other thread has added data to sndbuf concurrently.
>>
>> I agree that this patch does not change the behaviour of this function and removes an
>> atomic_set() in the likely path.
>>
>> I wonder however: All callers of smc_tx_sndbuf_nonempty() must hold the socket lock.
>> So how can we ever run in a concurrency situation?
>> Is this handling of conn->tx_pushing necessary at all?
> Hi Sandy,
> 
> Overall, I think you are right. But there is something we need to take care.
> 
> Before commit 6b88af839d20 ("net/smc: don't send in the BH context if
> sock_owned_by_user"), we used to call smc_tx_pending() in the soft IRQ,
> without checking sock_owned_by_user(), which would caused a race condition
> because bh_lock_sock() did not honor sock_lock(). To address this issue,
> I have added the tx_pushing mechanism. However, with commit 6b88af839d20,
> we now defer the transmission if sock_lock() is held by the user.
> Therefore, there should no longer be a race condition. Nevertheless, if
> we remove the tx_pending mechanism, we must always remember not to call
> smc_tx_sndbuf_nonempty() in the soft IRQ when the user holds the sock lock.
> 
> Thanks
> Dust


ok, I understand.
So whoever is willing to give it a try and simplify smc_tx_sndbuf_nonempty(),
should remember to document that requirement/precondition.
Maybe in a Function context section of a kernel-doc function decription?
(as described in https://docs.kernel.org/doc-guide/kernel-doc.html)
Although smc_tx_sndbuf_nonempty() is not exported, this format is helpful.

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

[Alexandra Winter]

On 20.11.23 10:11, Alexandra Winter wrote:
> 
> 
> On 20.11.23 04:20, Dust Li wrote:
>>> It seems to me that the purpose of conn->tx_pushing is
>>> a) Serve as a mutex, so only one thread per conn will call __smc_tx_sndbuf_nonempty().
>>> b) Repeat, in case some other thread has added data to sndbuf concurrently.
>>>
>>> I agree that this patch does not change the behaviour of this function and removes an
>>> atomic_set() in the likely path.
>>>
>>> I wonder however: All callers of smc_tx_sndbuf_nonempty() must hold the socket lock.
>>> So how can we ever run in a concurrency situation?
>>> Is this handling of conn->tx_pushing necessary at all?
>> Hi Sandy,
>>
>> Overall, I think you are right. But there is something we need to take care.
>>
>> Before commit 6b88af839d20 ("net/smc: don't send in the BH context if
>> sock_owned_by_user"), we used to call smc_tx_pending() in the soft IRQ,
>> without checking sock_owned_by_user(), which would caused a race condition
>> because bh_lock_sock() did not honor sock_lock(). To address this issue,
>> I have added the tx_pushing mechanism. However, with commit 6b88af839d20,
>> we now defer the transmission if sock_lock() is held by the user.
>> Therefore, there should no longer be a race condition. Nevertheless, if
>> we remove the tx_pending mechanism, we must always remember not to call
>> smc_tx_sndbuf_nonempty() in the soft IRQ when the user holds the sock lock.
>>
>> Thanks
>> Dust
> 
> 
> ok, I understand.
> So whoever is willing to give it a try and simplify smc_tx_sndbuf_nonempty(),
> should remember to document that requirement/precondition.
> Maybe in a Function context section of a kernel-doc function decription?
> (as described in https://docs.kernel.org/doc-guide/kernel-doc.html)
> Although smc_tx_sndbuf_nonempty() is not exported, this format is helpful.
> 


Tony Lu <tonylu@linux.alibaba.com> ' mail address has been corrupted in this whole thread.
Please reply to this message (corrected address) or take care, if replying to
other messages in this thread.

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

[Tony Lu]

On Mon, Nov 20, 2023 at 10:17:15AM +0100, Alexandra Winter wrote:
> 
> 
> On 20.11.23 10:11, Alexandra Winter wrote:
> > 
> > 
> > On 20.11.23 04:20, Dust Li wrote:
> >>> It seems to me that the purpose of conn->tx_pushing is
> >>> a) Serve as a mutex, so only one thread per conn will call __smc_tx_sndbuf_nonempty().
> >>> b) Repeat, in case some other thread has added data to sndbuf concurrently.
> >>>
> >>> I agree that this patch does not change the behaviour of this function and removes an
> >>> atomic_set() in the likely path.
> >>>
> >>> I wonder however: All callers of smc_tx_sndbuf_nonempty() must hold the socket lock.
> >>> So how can we ever run in a concurrency situation?
> >>> Is this handling of conn->tx_pushing necessary at all?
> >> Hi Sandy,
> >>
> >> Overall, I think you are right. But there is something we need to take care.
> >>
> >> Before commit 6b88af839d20 ("net/smc: don't send in the BH context if
> >> sock_owned_by_user"), we used to call smc_tx_pending() in the soft IRQ,
> >> without checking sock_owned_by_user(), which would caused a race condition
> >> because bh_lock_sock() did not honor sock_lock(). To address this issue,
> >> I have added the tx_pushing mechanism. However, with commit 6b88af839d20,
> >> we now defer the transmission if sock_lock() is held by the user.
> >> Therefore, there should no longer be a race condition. Nevertheless, if
> >> we remove the tx_pending mechanism, we must always remember not to call
> >> smc_tx_sndbuf_nonempty() in the soft IRQ when the user holds the sock lock.
> >>
> >> Thanks
> >> Dust
> > 
> > 
> > ok, I understand.
> > So whoever is willing to give it a try and simplify smc_tx_sndbuf_nonempty(),
> > should remember to document that requirement/precondition.
> > Maybe in a Function context section of a kernel-doc function decription?
> > (as described in https://docs.kernel.org/doc-guide/kernel-doc.html)
> > Although smc_tx_sndbuf_nonempty() is not exported, this format is helpful.
> > 
> 
> 
> Tony Lu <tonylu@linux.alibaba.com> ' mail address has been corrupted in this whole thread.
> Please reply to this message (corrected address) or take care, if replying to
> other messages in this thread.

Yes, that's true. Thanks Alexandra.

Please use my correct address, RongQing: Tony Lu <tonylu@linux.alibaba.com>.

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

[Dust Li]

On Mon, Nov 20, 2023 at 10:11:17AM +0100, Alexandra Winter wrote:
>
>
>On 20.11.23 04:20, Dust Li wrote:
>>> It seems to me that the purpose of conn->tx_pushing is
>>> a) Serve as a mutex, so only one thread per conn will call __smc_tx_sndbuf_nonempty().
>>> b) Repeat, in case some other thread has added data to sndbuf concurrently.
>>>
>>> I agree that this patch does not change the behaviour of this function and removes an
>>> atomic_set() in the likely path.
>>>
>>> I wonder however: All callers of smc_tx_sndbuf_nonempty() must hold the socket lock.
>>> So how can we ever run in a concurrency situation?
>>> Is this handling of conn->tx_pushing necessary at all?
>> Hi Sandy,
>> 
>> Overall, I think you are right. But there is something we need to take care.
>> 
>> Before commit 6b88af839d20 ("net/smc: don't send in the BH context if
>> sock_owned_by_user"), we used to call smc_tx_pending() in the soft IRQ,
>> without checking sock_owned_by_user(), which would caused a race condition
>> because bh_lock_sock() did not honor sock_lock(). To address this issue,
>> I have added the tx_pushing mechanism. However, with commit 6b88af839d20,
>> we now defer the transmission if sock_lock() is held by the user.
>> Therefore, there should no longer be a race condition. Nevertheless, if
>> we remove the tx_pending mechanism, we must always remember not to call
>> smc_tx_sndbuf_nonempty() in the soft IRQ when the user holds the sock lock.
>> 
>> Thanks
>> Dust
>
>
>ok, I understand.
>So whoever is willing to give it a try and simplify smc_tx_sndbuf_nonempty(),
>should remember to document that requirement/precondition.
>Maybe in a Function context section of a kernel-doc function decription?
>(as described in https://docs.kernel.org/doc-guide/kernel-doc.html)
>Although smc_tx_sndbuf_nonempty() is not exported, this format is helpful.

I double checked this and realized that I may have missed something
previously. The original goal of introducing tx_push was to maximize the
amount of data that could be corked in order to achieve the best
performance.

__smc_tx_sndbuf_nonempty() is thread and context safe, meaning that
it can be called simultaneously in both user context and softirq without
causing any bugs, just some CPU waste. Although I think we should remove
all the atomics & locks in the data path and only use sock_lock in the
long-term.

I will collaborate with Li RongQing on a new version that eliminates the
tx_pushing.

Best regards,
Dust