Re: git: 02ea6033020e - main - LinuxKPI: Allow spin_lock_irqsave to be called within a critical section

On 1/18/22, Vladimir Kondratyev <vladimir@kondratyev.su> 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 <wulf@FreeBSD.org>
>>>>>>> AuthorDate: 2022-01-18 20:14:12 +0000
>>>>>>> Commit:     Vladimir Kondratyev <wulf@FreeBSD.org>
>>>>>>> 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>