Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Tim Wawrzynczak, Julius Werner, Karthikeyan Ramasubramanian, Felix Held. Kyösti Mälkki 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/79ac18a2_4be89f85 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:
- Make a multi-cpu aware thread_mutex and replace all spin_locks.
- Disable thread_yield() while holding a spin lock. This can happen one of two ways:
- Provide thread_yield() some mechanism to determine if a spin lock is being held on the BSP.
- 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`.
I am in favor of 1. This can also address the case with lack of udelay() from uart8250mem, since thread_mutex can be made a periodical yield. And I think 2.1 and 2.2 eat the DMA performance for romstage because there are few (none) calls to thread_yield() where console lock is not held.
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
That improves the case for ramstage only. One might say it's really ugly hack to depend on console for the periodical yields() but the next alternative would be to scatter those yields() here and there.