lkml.org 
[lkml]   [2014]   [Jul]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: [RFC PATCH 1/1] rcu: use atomic_read(v) instead of atomic_add_return(0, v)
On Tue, Jul 8, 2014 at 8:07 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Tue, Jul 08, 2014 at 06:55:45PM -0400, Pranith Kumar wrote:
>> atomic_add_return() invalidates the cache line in other processors where-as
>> atomic_read does not. I don't see why we would need invalidation in this case.
>> If indeed it was need a comment would be helpful for readers. Otherwise doesn't
>> using atomic_read() make more sense here? RFC!
>>
>> replace atomic_add_return(0, v) with atomic_read(v) as the latter is better.
>>
>> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
>
> This will break RCU -- the full memory barriers implied both before
> and after atomic_add_return() are needed in order for RCU to be able to
> avoid death due to memory reordering.
>
> That said, I have considered replacing the atomic_add_return() with:
>
> smp_mb();
> ... = atomic_read(...);
> smp_mb();
>
> However, this is a very ticklish change, and would need serious thought
> and even more serious testing.
>

Thank you for looking at the RFC. I tried understanding the code
deeper and found that the ordering which is needed here is actually on
dynticks_snap.
The ordering currently (by way of atomic_add_return) is on
rdp->dynticks->dynticks which I think is not right.

Looking at the history of the code led me to rev. 23b5c8fa01b723 which
makes me think that the above statement is true. I think providing
ordering guarantees on dynticks_snap should be enough.

I have added an updated patch below. However, it is really hard(and
error prone) to convince oneself that this is right. So I will not
pursue this further if you think this is wrong. You definitely know
better than me :)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 1b70cb6..bbccd0a 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -891,7 +891,7 @@ static int rcu_is_cpu_rrupt_from_idle(void)
static int dyntick_save_progress_counter(struct rcu_data *rdp,
bool *isidle, unsigned long *maxj)
{
- rdp->dynticks_snap = atomic_add_return(0, &rdp->dynticks->dynticks);
+ smp_store_release(&rdp->dynticks_snap,
atomic_read(&rdp->dynticks->dynticks));
rcu_sysidle_check_cpu(rdp, isidle, maxj);
if ((rdp->dynticks_snap & 0x1) == 0) {
trace_rcu_fqs(rdp->rsp->name, rdp->gpnum, rdp->cpu, TPS("dti"));
@@ -920,8 +920,8 @@ static int rcu_implicit_dynticks_qs(struct rcu_data *rdp,
int *rcrmp;
unsigned int snap;

- curr = (unsigned int)atomic_add_return(0, &rdp->dynticks->dynticks);
- snap = (unsigned int)rdp->dynticks_snap;
+ curr = (unsigned int)atomic_read(&rdp->dynticks->dynticks);
+ snap = (unsigned int)smp_load_acquire(&rdp->dynticks_snap);

/*
* If the CPU passed through or entered a dynticks idle phase with

--
Pranith


\
 
 \ /
  Last update: 2014-07-09 06:43    [W:0.105 / U:5.012 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site