Attention is currently required from: Raul Rangel, Nico Huber, Xiang W, Philipp Hug, Jonathan Neuschäfer, Tim Wawrzynczak, Angel Pons, Kyösti Mälkki, Rob Barnes, Karthik Ramasubramanian. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59320 )
Change subject: lib: Add a mutex ......................................................................
Patch Set 15:
(1 comment)
Patchset:
PS15:
All this review is happening in a no-op change, while CB:59321 is where spinlock/mutex change really takes place. It has not been rebased for couple months.
I was just trying to keep the discussion in one place, I'm aware there is more to this patch series and I assume Raul is going to start cleaning the later parts up again once the fundamental question of direction here is resolved.
I have provided pointers in my previous commentary (CB:60253 and CB:60280) that show it's easy to use compiler atomic builtins to provide a spinlock, and easy to build mutex with such a spinlock.
Right, I've seen your patches, but they go in a different direction than what I'm trying to argue here so I haven't commented on them yet while we're still hashing out the fundamental question. I understand that what you're proposing is possible, it's not a question about proving that we can do it that way. I just think Raul's direction is better.
This is known to not be SMP safe, as it turns out it completely lacks a spinlock currently, but why is mutex() ever better than spinlock() around a index/data IO access like these:
src/arch/x86/include/arch/pci_io_cfg.h
I don't think the question is "why is mutex better in this case", the question is "would a spinlock in this case really be so superior, or would a mutex be so harmful, that it outweighs the cost of extra complexity of keeping both concepts separate". Yes, you don't need thread-safety for those cases. But how much does the thread-safety cost? One extra function call that's not inlined, or something like that? Are we really running this code in tight enough loops that that makes any noticeable impact?
Requiring the programmer to think about whether a certain point of mutual exclusion needs to be thread-safe or "only" SMP-safe is a real maintenance cost, and it creates a real risk for making mistakes that create dangerous bugs (especially if threading is a niche feature that many people who add code that needs mutual exclusion may not care about). So I would tend towards preventing that issue, unless it creates a real problem elsewhere, and I don't think the miniscule amount of overhead from having this code over pure spinlock code does that (unless you know an example where it would be called often enough to really amplify that to notable levels?).
-
Julius Werner (Code Review)