| Message ID | 20240628134648.1852041-1-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | ARM: vfp: Introduce vfp_lock() for VFP locking. | expand |
On Fri, Jun 28, 2024 at 03:46:15PM +0200, Sebastian Andrzej Siewior wrote: > There was a bug report on PREEMPT_RT which made me look into VFP locking > on ARM. The usage of local_bh_disable() to ensure exclusive access to > the VFP unit is not working on PREEMPT_RT because the softirq context is > preemptible. > > This series introduces vfp_lock() which does the right thing. I disagre with the term "lock" here - it isn't about any kind of locking, it's about: 1. preventing the thread moving onto a different CPU 2. preventing kernel-mode usage of VFP while a VFP fault is being handled (and thus changing the VFP state.) The proble with "lock" is that it makes it look like this code is single-threaded which it isn't. Many CPUs can be processing a VFP fault at the same time - remembering that each CPU has their own VFP unit. Maybe name it vfp_state_hold() ... vfp_state_release() instead? Hold as in "don't let anything interfere with the state we are going to be accessing". I'm open to better names if anyone can think of any!
On 2024-06-28 15:07:37 [+0100], Russell King (Oracle) wrote: > On Fri, Jun 28, 2024 at 03:46:15PM +0200, Sebastian Andrzej Siewior wrote: > > There was a bug report on PREEMPT_RT which made me look into VFP locking > > on ARM. The usage of local_bh_disable() to ensure exclusive access to > > the VFP unit is not working on PREEMPT_RT because the softirq context is > > preemptible. > > > > This series introduces vfp_lock() which does the right thing. > > I disagre with the term "lock" here - it isn't about any kind of > locking, it's about: > > 1. preventing the thread moving onto a different CPU > 2. preventing kernel-mode usage of VFP while a VFP fault is being > handled (and thus changing the VFP state.) > > The proble with "lock" is that it makes it look like this code is > single-threaded which it isn't. Many CPUs can be processing a VFP > fault at the same time - remembering that each CPU has their own > VFP unit. Yes, from that point of view it looks bad. Given that every CPU has a VFP unit it is more of a per-CPU lock. But this isn't obvious from just looking at it. > Maybe name it vfp_state_hold() ... vfp_state_release() instead? > Hold as in "don't let anything interfere with the state we are > going to be accessing". > > I'm open to better names if anyone can think of any! No, I'm fine with it. Sebastian
There was a bug report on PREEMPT_RT which made me look into VFP locking on ARM. The usage of local_bh_disable() to ensure exclusive access to the VFP unit is not working on PREEMPT_RT because the softirq context is preemptible. This series introduces vfp_lock() which does the right thing. v3…v4: - Repost. v2…v3: https://lore.kernel.org/all/20230926125502.1127015-1-bigeasy@linutronix.de - Repost with Ard's Reviewed-by tag. v1…v2: https://lore.kernel.org/all/20230628080516.798032-1-bigeasy@linutronix.de - Split the last patch into two patches (3 and 4 of this series). Suggested by Ard. v1: https://lore.kernel.org/all/20230615101656.1308942-1-bigeasy@linutronix.de Sebastian