| Message ID | 20170720102855.21961-1-Haakon.Bugge@oracle.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 <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
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>
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.
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;
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(-)