Attention is currently required from: Raul Rangel, Rob Barnes, Kyösti Mälkki, 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 3:
(4 comments)
File src/include/mutex.h:
https://review.coreboot.org/c/coreboot/+/59320/comment/75fbaf22_ab79474b PS2, Line 11: void mutex_unlock(struct mutex *mutex);
So I wanted to make it so the lock/unlock asserts were checked regardless of threading or SMP. […]
I don't think we should add all these calls to builds where they really do nothing at all, just to support assertions. They may get called pretty often in some contexts. The assertions will still get noticed on the SMP systems.
I think these should become nothing (not even a call instruction to an empty function) on those platforms, so the checks should be in static inlines, or preprocessor guards.
File src/lib/mutex.c:
https://review.coreboot.org/c/coreboot/+/59320/comment/38b699ab_173de68d PS2, Line 24: #if ENV_X86
I haven't specifically looked at the ARM assembly generated by this. […]
Hmmm... well, how sure are we that this is correct for RISC-V? I'm not familiar with that architecture but I'd be surprised if they don't have some sort of PAUSE instruction equivalent.
We can do it this way for now if you want but I doubt it would really hold up for a lot of architectures.
File src/lib/mutex.c:
https://review.coreboot.org/c/coreboot/+/59320/comment/b537d39a_a8e90d0b PS3, Line 27: continue; If we're keeping this code I find the continue here pretty weird -- the effect is the same as not having it. I'd just leave out the #else here.
https://review.coreboot.org/c/coreboot/+/59320/comment/898cf794_68f789ba PS3, Line 31: } while (__atomic_load_1(&mutex->locked, __ATOMIC_RELAXED)); Are we sure this is guaranteed to make forward progress eventually, even with the relaxed model (i.e. does the memory model guarantee that a store by another CPU will eventually become visible to a relaxed load by this CPU within a finite amount of time even if there is no formal dependency declared)?
-
Julius Werner (Code Review)