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

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.

> Do you mean our seqlocks as presented in sys/seqc.h, or something Linuxish?


-- 
WBR
Vladimir Kondratyev