Attention is currently required from: Jason Glenesk, Marshall Dawson, Tim Wawrzynczak, Julius Werner, Kyösti Mälkki, Karthikeyan Ramasubramanian, Felix Held. Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59094 )
Change subject: [WIP,RFC] Clean up spinlocks ......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/coreboot/+/59094/comment/da709b61_b372434b PS2, Line 14: IMO commit a98d302fe9 x86/smp/spinlock: Disable thread coop when taking spinlock : is wrong and should be reverted too. I think there is a disconnect between spinlock and thread_mutex.
spinlock was made to prevent more than 1 CPU from executing. As you said, yielding here is bad...
thread_mutex was only meant to work with coop multi-threading to guard a resource. It is safe to yield while holding the mutex because a `thread_mutex_lock` will yield if it can't get the lock.
So we have two options: 1) Make a multi-cpu aware thread_mutex and replace all spin_locks. 2) Disable thread_yield() while holding a spin lock. This can happen one of two ways: 1) Provide thread_yield() some mechanism to determine if a spin lock is being held on the BSP. 2) Have spin_lock just disable coop-multi-threading.
Honestly, 2.2 is the easiest to grasp. If you want, I will rename `spin_lock` to `spin_lock_disable_coop` and `spin_unlock_enable_coop`.
Now I am not going to -2 these threding and spinlock changes. Just understand your DMA performance currently relies on periodic context switches, and you really really should not rely on uart8250mem and printk() to satisfy that.
Right. In my case the DMA controller can perform 64 KiB transactions. Large things like the payload require being armed twice.
I'm not currently dependent on printk to satisfy that requirement. I'm adding more periodic yields to make it more predictable: https://review.coreboot.org/c/coreboot/+/58955
-
Raul Rangel (Code Review)