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 -- 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
