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 <[email protected]>
Reviewed-by: Knut Omang <[email protected]>
---
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
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
> On 20 Jul 2017, at 13:02, Sowmini Varadhan <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <[email protected]>
> Reviewed-by: Knut Omang <[email protected]>
> ---
Having said that, {READ,WRITE}_ONCE usages seems to make
it clear and explicit. So its fine with me.
Acked-by: Santosh Shilimkar <[email protected]>
From: H?kon Bugge <[email protected]>
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 <[email protected]>
> Reviewed-by: Knut Omang <[email protected]>
Applied, thanks.