sequence lock in Linux
in [Kernel]
|
From: Mathieu Desnoyers on 11 Jun 2010 15:50 Hi Paul, (CCing lkml) Is it just me, or the following code: static __always_inline unsigned read_seqbegin(const seqlock_t *sl) { unsigned ret; repeat: ret = sl->sequence; smp_rmb(); if (unlikely(ret & 1)) { cpu_relax(); goto repeat; } return ret; } could use a ACCESS_ONCE() around the sl->sequence read ? I'm concerned about the compiler generating code that reads the sequence number chunkwise. The same apply to all other reads of the sequence number in seqlock.h (including the retry code). Thoughts ? Mathieu -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 11 Jun 2010 16:10 On Fri, Jun 11, 2010 at 12:40 PM, Mathieu Desnoyers <mathieu.desnoyers(a)efficios.com> wrote: > > Is it just me, or the following code: > > static __always_inline unsigned read_seqbegin(const seqlock_t *sl) > { > � � � �unsigned ret; > > repeat: > � � � �ret = sl->sequence; > � � � �smp_rmb(); > � � � �if (unlikely(ret & 1)) { > � � � � � � � �cpu_relax(); > � � � � � � � �goto repeat; > � � � �} > > � � � �return ret; > } > > could use a ACCESS_ONCE() around the sl->sequence read ? I'm concerned about the > compiler generating code that reads the sequence number chunkwise. What compiler would do that? That would seem to be a compiler bug, or a compiler that is just completely crazy. But it wouldn't be _wrong_ to make it do ACCESS_ONCE(). I just suspect that any compiler that cares is not a compiler worth worrying about, and the compiler should be shot in the head rather than us necessarily worrying about it. There is no way a sane compiler can do anything but one read anyway. We do end up using all the bits (for the "return ret") part, so a compiler that reads the low bit separately is just being a totally moronic one - we wouldn't want to touch such a stupid compiler with a ten-foot pole. But at the same time, ACCESS_ONCE() ends up being a reasonable hint to programmers, so I wouldn't object to it. I just don't think we should pander to "compilers can be crazy". If compilers are crazy, we shouldn't use them. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Paul E. McKenney on 11 Jun 2010 16:10 On Fri, Jun 11, 2010 at 03:40:16PM -0400, Mathieu Desnoyers wrote: > Hi Paul, > > (CCing lkml) > > Is it just me, or the following code: > > static __always_inline unsigned read_seqbegin(const seqlock_t *sl) > { > unsigned ret; > > repeat: > ret = sl->sequence; > smp_rmb(); > if (unlikely(ret & 1)) { > cpu_relax(); > goto repeat; > } > > return ret; > } > > could use a ACCESS_ONCE() around the sl->sequence read ? I'm concerned about the > compiler generating code that reads the sequence number chunkwise. > > The same apply to all other reads of the sequence number in seqlock.h (including > the retry code). > > Thoughts ? Doesn't gcc guarantee that accesses to aligned basic types that fit into a machine word are loaded and stored in one shot? Now, gcc might choose to load twice (or to merge loads) due to things like register pressure, but given that ->sequence is an int, gcc should not be accessing it (say) bytewise on any platform supporting 32-bit accesses. Or am I suffering from wishful thinking here? Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Paul E. McKenney on 11 Jun 2010 16:40 On Fri, Jun 11, 2010 at 01:07:55PM -0700, Linus Torvalds wrote: > On Fri, Jun 11, 2010 at 12:40 PM, Mathieu Desnoyers > <mathieu.desnoyers(a)efficios.com> wrote: > > > > Is it just me, or the following code: > > > > static __always_inline unsigned read_seqbegin(const seqlock_t *sl) > > { > > � � � �unsigned ret; > > > > repeat: > > � � � �ret = sl->sequence; > > � � � �smp_rmb(); > > � � � �if (unlikely(ret & 1)) { > > � � � � � � � �cpu_relax(); > > � � � � � � � �goto repeat; > > � � � �} > > > > � � � �return ret; > > } > > > > could use a ACCESS_ONCE() around the sl->sequence read ? I'm concerned about the > > compiler generating code that reads the sequence number chunkwise. > > What compiler would do that? That would seem to be a compiler bug, or > a compiler that is just completely crazy. The reason that the C standard permits this is to allow for things like 8-bit CPUs, which are simply unable to load or store 32-bit quantities except by doing it chunkwise. But I don't expect the Linux kernel to boot on these, and certainly not on any of the ones that I have used! I most definitely remember seeing a gcc guarantee that loads and stores would be done in one instruction whenever the hardware supported this, but I am not finding it today. :-( Thanx, Paul > But it wouldn't be _wrong_ to make it do ACCESS_ONCE(). I just suspect > that any compiler that cares is not a compiler worth worrying about, > and the compiler should be shot in the head rather than us necessarily > worrying about it. > > There is no way a sane compiler can do anything but one read anyway. > We do end up using all the bits (for the "return ret") part, so a > compiler that reads the low bit separately is just being a totally > moronic one - we wouldn't want to touch such a stupid compiler with a > ten-foot pole. > > But at the same time, ACCESS_ONCE() ends up being a reasonable hint to > programmers, so I wouldn't object to it. I just don't think we should > pander to "compilers can be crazy". If compilers are crazy, we > shouldn't use them. > > Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mathieu Desnoyers on 11 Jun 2010 16:50 * Paul E. McKenney (paulmck(a)linux.vnet.ibm.com) wrote: > On Fri, Jun 11, 2010 at 03:40:16PM -0400, Mathieu Desnoyers wrote: > > Hi Paul, > > > > (CCing lkml) > > > > Is it just me, or the following code: > > > > static __always_inline unsigned read_seqbegin(const seqlock_t *sl) > > { > > unsigned ret; > > > > repeat: > > ret = sl->sequence; > > smp_rmb(); > > if (unlikely(ret & 1)) { > > cpu_relax(); > > goto repeat; > > } > > > > return ret; > > } > > > > could use a ACCESS_ONCE() around the sl->sequence read ? I'm concerned about the > > compiler generating code that reads the sequence number chunkwise. > > > > The same apply to all other reads of the sequence number in seqlock.h (including > > the retry code). > > > > Thoughts ? > > Doesn't gcc guarantee that accesses to aligned basic types that fit into > a machine word are loaded and stored in one shot? Now, gcc might choose > to load twice (or to merge loads) due to things like register pressure, > but given that ->sequence is an int, gcc should not be accessing it > (say) bytewise on any platform supporting 32-bit accesses. > > Or am I suffering from wishful thinking here? Hopefully not. I might be the one suffering from extreme compiler distrust here. ;-) Mathieu > > Thanx, Paul -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
|
Next
|
Last
Pages: 1 2 3 Prev: Btrfs updates for 2.6.35 Next: drivers/net/e1000/e1000_main.c: Fix message logging defect |