On 04/16/12 11:15, Torvald Riegel wrote: > Richard, Andrew, > > I had a look at libatomic yesterday, focusing primarily on > synchronization issues in it. Here are some comments. And I think > there is a bug in it too. Notes are in no particular order. Let me know > what you think. > > - seq_cst fences are always used whenever stuff is not relaxed. I > suppose this is a potential future TODO? (default host_config.c)
Fixed. I think this only really affected powerpc. > - for acq/rel memorders, we don't need seq_cst fences whenever the > atomicity is implemented with a lock > (hostconfig.c:pre_barrier/post_barrier) Err.. where? This also seems to conflict with... > - protect_start|end: > - BUG: those are called without additional post|pre_barrier calls in > cas_n.c, exch_n.c, fop_n.c, load_n.c, store_n.c > The lock membar guarantees are too weak if memorder for the atomic > access is seq_cst, and if you use more than one lock. > (assumes that lock implementation uses typical acq/rel barriers). ... this. I don't see what multiple locks has to do with seq vs acqrel. > - only ptr, no size used, and so atomic types must always be accessed > by same-sized ops. This does make sense of course, but should we > document this explicitly somewhere? Possibly. It's also a side-effect of using a WATCH_SIZE that is at least as large as the largest atomic type supported. > - load_n.c: > - I'm concerned about the CAS on read-only mprotected pages? > Why again do we think this is safe? Does the standard explicitly > allow this? Or should we just use a lock in this case? Andrew, you had a bit of back-and-forth with someone about this. Can you dig that up? > - cas_n.c: > - I suppose this is a strong cmpxchg? Of course. Weak is restricted to the builtin interface; the library does not support it. > - config/posix/lock.c: > - I'm not sure that WATCH_SIZE should almost certainly be the same as > the cacheline size. Why? Which assumptions about the program? See above. Plus I assumed that if we have to take a lock, that's got to dominate over any cache ping-pong that would be played. > - page_size optzn: AFAICT, according to the standard (29.4.3), only > lock-free atomics are supposed to also be address-free. So > libatomic wouldn't need to be constrained by the page size, or we > can use a different hash function too to try to avoid false > sharing / conflicts. Hmm. Ok, I'll think about this. > - Do locks need to care about priority inversion? At the moment I'm leaving this with the pthread implementation. > - other notes: > - might be better to prefer locks over a kernel cmpxchg perhaps? any > archs/platforms that have a lock Almost certainly not. When kernel cmpxchg is implemented, that's also how locks get implemented in libc. No point using a lock to avoid the kernel in those cases. I've updated the tree at git://repo.or.cz/gcc/rth.git rth/libatomic r~
