On Thu, 18 Oct 2012, Steven Rostedt wrote: > On Thu, 2012-10-18 at 10:18 +0800, Lai Jiangshan wrote: > > > > > > Looking at the patch, you are correct. The read side doesn't need the > > > memory barrier as the worse thing that will happen is that it sees the > > > locked = false, and will just grab the mutex unnecessarily. > > > > --------------------- > > A memory barrier can be added iff these two things are known: > > 1) it disables the disordering between what and what. > > 2) what is the corresponding mb() that it pairs with. > > > > OK, I was just looking at the protection and actions of the locked flag, > but I see what you are saying with the data itself. > > > You tried to add a mb() in percpu_up_write(), OK, I know it disables the disordering > > between the writes to the protected data and the statement "p->locked = false", > > But I can't find out the corresponding mb() that it pairs with. > > > > percpu_down_read() writes to the data > > The cpu cache/prefetch the data writes to the data > > which is chaos writes to the data > > percpu_up_write() > > mb() > > p->locked = false; > > unlikely(p->locked) > > the cpu see p->lock = false, > > don't discard the cached/prefetch data > > this_cpu_inc(*p->counters); > > the code of read-access to the data > > ****and we use the chaos data***** > > > > So you need to add a mb() after "unlikely(p->locked)". > > Does it need a full mb() or could it be just a rmb()? The down_read I > wouldn't think would need to protect against stores, would it? The > memory barrier should probably go in front of the unlikely() too. The > write to p->counters is handled by the synchronized sched, and adding a > rmb() in front of the unlikely check would keep prefetched data from > passing this barrier. > > This is a perfect example why this primitive should be vetted outside of > mainline before it gets merged. > > -- Steve If we do synchronize_rcu() in percpu_up_write, we don't need a barrier in percpu_down_read(). So I would do that. Mikulas -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
