[PATCH net] rds: Make sure updates to cp_send_gen can be observed

[PATCH net] rds: Make sure updates to cp_send_gen can be observed

[Håkon Bugge]

cp->cp_send_gen is treated as a normal variable, although it may be
used by different threads.

This is fixed by using {READ,WRITE}_ONCE when it is incremented and
READ_ONCE when it is read outside the {acquire,release}_in_xmit
protection.

Normative reference from the Linux-Kernel Memory Model:

    Loads from and stores to shared (but non-atomic) variables should
    be protected with the READ_ONCE(), WRITE_ONCE(), and
    ACCESS_ONCE().

Clause 5.1.2.4/25 in the C standard is also relevant.

Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
Reviewed-by: Knut Omang <knut.omang@oracle.com>
---
 net/rds/send.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/rds/send.c b/net/rds/send.c
index 5cc6403..fa0368c 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -170,8 +170,8 @@ int rds_send_xmit(struct rds_conn_path *cp)
 	 * The acquire_in_xmit() check above ensures that only one
 	 * caller can increment c_send_gen at any time.
 	 */
-	cp->cp_send_gen++;
-	send_gen = cp->cp_send_gen;
+	send_gen = READ_ONCE(cp->cp_send_gen) + 1;
+	WRITE_ONCE(cp->cp_send_gen, send_gen);
 
 	/*
 	 * rds_conn_shutdown() sets the conn state and then tests RDS_IN_XMIT,
@@ -431,7 +431,7 @@ int rds_send_xmit(struct rds_conn_path *cp)
 		smp_mb();
 		if ((test_bit(0, &conn->c_map_queued) ||
 		     !list_empty(&cp->cp_send_queue)) &&
-		    send_gen == cp->cp_send_gen) {
+			send_gen == READ_ONCE(cp->cp_send_gen)) {
 			rds_stats_inc(s_send_lock_queue_raced);
 			if (batch_count < send_batch_count)
 				goto restart;
-- 
2.9.3

Re: [PATCH net] rds: Make sure updates to cp_send_gen can be observed

[Sowmini Varadhan]

On (07/20/17 12:28), H??kon Bugge wrote:
> cp->cp_send_gen is treated as a normal variable, although it may be
> used by different threads.

I'm confused by that assertion. If you look at the comments right
above the change in your patch, there is a note that 
acquire_in_xmit/release_in_xmit are the synchronization/serialization 
points.

Can you please clarify?

> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -170,8 +170,8 @@ int rds_send_xmit(struct rds_conn_path *cp)
>  	 * The acquire_in_xmit() check above ensures that only one
>  	 * caller can increment c_send_gen at any time.
>  	 */
> -	cp->cp_send_gen++;
> -	send_gen = cp->cp_send_gen;
> +	send_gen = READ_ONCE(cp->cp_send_gen) + 1;
> +	WRITE_ONCE(cp->cp_send_gen, send_gen);
>  

--Sowmini

Re: [PATCH net] rds: Make sure updates to cp_send_gen can be observed

[Håkon Bugge]

> On 20 Jul 2017, at 13:02, Sowmini Varadhan <sowmini.varadhan@oracle.com> wrote:
> 
> On (07/20/17 12:28), H??kon Bugge wrote:
>> cp->cp_send_gen is treated as a normal variable, although it may be
>> used by different threads.
> 
> I'm confused by that assertion. If you look at the comments right
> above the change in your patch, there is a note that 
> acquire_in_xmit/release_in_xmit are the synchronization/serialization 
> points.
> 
> Can you please clarify?

The way the original code works, is that it is allowed for the compiler to keep the value of “cp->cp_send_gen + 1” in a register. The compiler has no requirement to store this value to memory, before leaving the function or calling another one.

Further, said register can be used in the comparison outside the acquire_in_xmit/release_in_xmit, at which point another thread may have changed its value.


Thxs, Håkon

> 
>> --- a/net/rds/send.c
>> +++ b/net/rds/send.c
>> @@ -170,8 +170,8 @@ int rds_send_xmit(struct rds_conn_path *cp)
>> 	 * The acquire_in_xmit() check above ensures that only one
>> 	 * caller can increment c_send_gen at any time.
>> 	 */
>> -	cp->cp_send_gen++;
>> -	send_gen = cp->cp_send_gen;
>> +	send_gen = READ_ONCE(cp->cp_send_gen) + 1;
>> +	WRITE_ONCE(cp->cp_send_gen, send_gen);
>> 
> 
> --Sowmini
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net] rds: Make sure updates to cp_send_gen can be observed

[Santosh Shilimkar]

On 7/20/2017 3:28 AM, Håkon Bugge wrote:
> cp->cp_send_gen is treated as a normal variable, although it may be
> used by different threads.
> 
> This is fixed by using {READ,WRITE}_ONCE when it is incremented and
> READ_ONCE when it is read outside the {acquire,release}_in_xmit
> protection.
>
There is explicit memory barrier before the value is read outside
the {acquire,release}_in_xmit so it takes care of load/store sync.

> Normative reference from the Linux-Kernel Memory Model:
> 
>      Loads from and stores to shared (but non-atomic) variables should
>      be protected with the READ_ONCE(), WRITE_ONCE(), and
>      ACCESS_ONCE().
> 
> Clause 5.1.2.4/25 in the C standard is also relevant.
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Reviewed-by: Knut Omang <knut.omang@oracle.com>
> ---
Having said that, {READ,WRITE}_ONCE usages seems to make
it clear and explicit. So its fine with me.

Acked-by: Santosh Shilimkar <santosh.shilimkar@oracle.com>

Re: [PATCH net] rds: Make sure updates to cp_send_gen can be observed

[David Miller]

From: Håkon Bugge <Haakon.Bugge@oracle.com>
Date: Thu, 20 Jul 2017 12:28:55 +0200

> cp->cp_send_gen is treated as a normal variable, although it may be
> used by different threads.
> 
> This is fixed by using {READ,WRITE}_ONCE when it is incremented and
> READ_ONCE when it is read outside the {acquire,release}_in_xmit
> protection.
> 
> Normative reference from the Linux-Kernel Memory Model:
> 
>     Loads from and stores to shared (but non-atomic) variables should
>     be protected with the READ_ONCE(), WRITE_ONCE(), and
>     ACCESS_ONCE().
> 
> Clause 5.1.2.4/25 in the C standard is also relevant.
> 
> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> Reviewed-by: Knut Omang <knut.omang@oracle.com>

Applied, thanks.