On 1/18/22, Vladimir Kondratyev <[email protected]> wrote: > On 19.01.2022 01:08, Konstantin Belousov wrote: >> On Wed, Jan 19, 2022 at 01:01:45AM +0300, Vladimir Kondratyev wrote: >>> On 19.01.2022 00:48, Konstantin Belousov wrote: >>>> On Wed, Jan 19, 2022 at 12:35:41AM +0300, Vladimir Kondratyev wrote: >>>>> On 18.01.2022 23:22, Konstantin Belousov wrote: >>>>>> On Tue, Jan 18, 2022 at 08:15:36PM +0000, Vladimir Kondratyev wrote: >>>>>>> The branch main has been updated by wulf: >>>>>>> >>>>>>> URL: >>>>>>> https://cgit.FreeBSD.org/src/commit/?id=02ea6033020e11afec6472bf560b0ddebd0fa97a >>>>>>> >>>>>>> commit 02ea6033020e11afec6472bf560b0ddebd0fa97a >>>>>>> Author: Vladimir Kondratyev <[email protected]> >>>>>>> AuthorDate: 2022-01-18 20:14:12 +0000 >>>>>>> Commit: Vladimir Kondratyev <[email protected]> >>>>>>> CommitDate: 2022-01-18 20:14:12 +0000 >>>>>>> >>>>>>> LinuxKPI: Allow spin_lock_irqsave to be called within a >>>>>>> critical section >>>>>>> with spinning on spin_trylock. dma-buf part of drm-kmod >>>>>>> depends on this >>>>>>> property and absence of it support results in "mi_switch: >>>>>>> switch in a >>>>>>> critical section" assertions [1][2]. >>>>>>> [1] https://github.com/freebsd/drm-kmod/issues/116 >>>>>>> [2] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=261166 >>>>>>> MFC after: 1 week >>>>>>> Reviewed by: manu >>>>>>> Differential Revision: https://reviews.freebsd.org/D33887 >>>>>>> --- >>>>>>> .../linuxkpi/common/include/linux/spinlock.h | 27 >>>>>>> ++++++++++++++++++---- >>>>>>> 1 file changed, 23 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> diff --git a/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>>>> b/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>>>> index a87cb7180b28..31d47fa73986 100644 >>>>>>> --- a/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>>>> +++ b/sys/compat/linuxkpi/common/include/linux/spinlock.h >>>>>>> @@ -37,6 +37,7 @@ >>>>>>> #include <sys/lock.h> >>>>>>> #include <sys/mutex.h> >>>>>>> #include <sys/kdb.h> >>>>>>> +#include <sys/proc.h> >>>>>>> #include <linux/compiler.h> >>>>>>> #include <linux/rwlock.h> >>>>>>> @@ -117,14 +118,32 @@ typedef struct { >>>>>>> local_bh_disable(); \ >>>>>>> } while (0) >>>>>>> -#define spin_lock_irqsave(_l, flags) do { \ >>>>>>> - (flags) = 0; \ >>>>>>> - spin_lock(_l); \ >>>>>>> +#define __spin_trylock_nested(_l, _n) ({ \ >>>>>>> + int __ret; \ >>>>>>> + if (SPIN_SKIP()) { \ >>>>>>> + __ret = 1; \ >>>>>>> + } else { \ >>>>>>> + __ret = mtx_trylock_flags(&(_l)->m, MTX_DUPOK); \ >>>>>>> + if (likely(__ret != 0)) \ >>>>>>> + local_bh_disable(); \ >>>>>>> + } \ >>>>>>> + __ret; \ >>>>>>> +}) >>>>>>> + >>>>>>> +#define spin_lock_irqsave(_l, flags) do { \ >>>>>>> + (flags) = 0; \ >>>>>>> + if (unlikely(curthread->td_critnest != 0)) \ >>>>>>> + while (!spin_trylock(_l)) {} \ >>>>>>> + else \ >>>>>>> + spin_lock(_l); \ >>>>>>> } while (0) >>>>>>> #define spin_lock_irqsave_nested(_l, flags, _n) do { \ >>>>>>> (flags) = 0; \ >>>>>>> - spin_lock_nested(_l, _n); \ >>>>>>> + if (unlikely(curthread->td_critnest != 0)) \ >>>>>>> + while (!__spin_trylock_nested(_l, _n)) {} \ >>>>>>> + else \ >>>>>>> + spin_lock_nested(_l, _n); \ >>>>>>> } while (0) >>>>>>> #define spin_unlock_irqrestore(_l, flags) do { \ >>>>>> You are spin-waiting for blockable mutex, am I right? >>>>> >>>>> Both, yes and no. On Linux spin_lock_irqsave is generally unblockable >>>>> as it >>>>> disables preemption and interrupts while our version does not do this >>>>> as >>>>> LinuxKPI is not ready for such a tricks. >>>>> It seems that we should explicitly add critical_enter()/critical_exit >>>>> calls >>>>> to related dma-buf parts to make it unblockable too. >>>> LinuxKPI does +1 to the level of locks comparing with Linux, so their >>>> spinlocks >>>> become our blockable mutexes. >>>> >>>> Can you please explain why dmabufs need critical section? What is >>>> achieved there by disabled preemption? >>>> >>> >>> dma-buf uses sequence locks for synchronization. If seqlock is taken for >>> write, than thread it holding enters in to critical section to not force >>> readers to spin if writer is preempted. Unfortunately, dma-buf writers >>> execute callbacks which requires locks and spin_lock_irqsave is used for >>> synchronize these callbacks. >> >> Then, it seems that locking should be changed either to rwlocks or >> rmlocks, >> not sure which. > > That can introduce LORs as seqlock's readers never block writers. It is > probably > easier to skip critical section at all at the cost of extra CPU cycles spent > on > reader spinning. >
The reader side could try to get a stable snapshot just once, and failing that, take the lock to get the data. Then you are set. -- Mateusz Guzik <mjguzik gmail.com>
