Attention is currently required from: Raul Rangel, 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 9:
(2 comments)
File src/include/mutex.h:
https://review.coreboot.org/c/coreboot/+/59320/comment/8bd9a35a_879c2c8f PS9, Line 62: assert(__atomic_load_1(&mutex->locked, __ATOMIC_RELAXED)); I'm not sure if we actually want to inline these, since assert() takes a notable amount of instructions (and a big string constant where I'm not sure if it manages to deduplicate that for an inline function). My idea was to have mutex_lock()/mutex_unlock() static inline, but keep these other ones in the .c file.
File src/include/mutex.h:
https://review.coreboot.org/c/coreboot/+/59320/comment/199eca96_b172837a PS6, Line 32: }
AMD x86 romstages run APs.
Oh okay, I didn't know that. But then there needs to be a Kconfig for that, or we need to explicitly check for those platforms in the rules macros. We shouldn't have all x86 platforms include spinlock code in romstage if only some AMD boards need it.
Say you do some PCI configuration register read-modify-write ops. Would you put a spinlock or mutex around it? I would use a spinlock
I think that should probably be a mutex, honestly... I don't think we'd want to get into the business of having to analyze exactly which sequences of code can yield() and which can't. But does it happen often that you need to guard a single register access? I would assume most of the time there's just a big lock for every PCI device, and then the driver takes that at the start of the function and releases it at the end.
If you have SMP=y is it really worth the trouble to drop the atomicity for stages where you don't really need it? I am asking if it is really worth having _mutex_{lock/unlock]_coop() implemented separately. A better optimisation might be to have spinlock implememntations, here _mutex_(un)lock_{coop,smp} always inlined.
Is there any cost to it? Using different code paths with compile-time checks is free. Sure, it only saves a few instructions, but why shouldn't we do that if we can? (Mostly, we can turn the whole thing into a noop in platforms/stages where there's neither SMP nor COOP, which I think is worthwhile... with some platforms' pre-RAM stages, every byte of binary size help.)
-
Julius Werner (Code Review)