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

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.

   This means a deadlock.
> Just for example, on UP machine the spinning thread could starve the thread
> that owns the mutex, which never gets to the CPU.

Any ideas how to avoid it in a generic way?


-- 
WBR
Vladimir Kondratyev