On Mon, 20 Oct 2008, Mathieu Desnoyers wrote: > * Peter Zijlstra (a.p.zijlstra@xxxxxxxxx) wrote: > > Have you looked at the existing 32->63 extention code in > > include/linux/cnt32_to_63.h and considered unifying it? > > > > Yep, I felt this code was dangerous on SMP given it could suffer from > the following type of race due to lack of proper barriers : You are wrong. > CPU A B > read hw cnt low > read __m_cnt_hi > read hw cnt low > (wrap detected) > write __m_cnt_hi (incremented) > read __m_cnt_hi > (wrap detected) > write __m_cnt_hi (incremented) > > we therefore increment the high bits twice in the given race. No. Either you do the _same_ incrementation twice effectively writing the _same_ high value twice, or you don't have simultaneous wrap detections. Simulate it on paper with real numbers if you are not convinced. > On UP, the same race could happen if the code is called with preemption > enabled. Wrong again. > I don't think the "volatile" statement would necessarily make sure the > compiler and CPU would do the __m_cnt_hi read before the hw cnt low > read. A real memory barrier to order mmio reads wrt memory reads (or > instruction sync barrier if the value is taken from the cpu registers) > would be required to insure such order. That's easy enough to fix, right? > I also felt it would be more solid to have per-cpu structures to keep > track of 32->64 bits TSC updates, given the TSCs can always be slightly > out-of-sync : If the low part is a per CPU value then the high part has to be a per CPU value too. There only needs to be a per-CPU variant of the same algorithm where the only difference is that __m_cnt_hi would be per CPU. If the low part is global then __m_cnt_hi has to be global, even on SMP. Nicolas -- 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
