Date: Sat, 22 Jan 2022 10:49:09 +0100 From: Hans Petter Selasky <hps@selasky.org> To: Vladimir Kondratyev <vladimir@kondratyev.su>, Konstantin Belousov <kostikbel@gmail.com>, Vladimir Kondratyev <wulf@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 02ea6033020e - main - LinuxKPI: Allow spin_lock_irqsave to be called within a critical section Message-ID: <6b3eabe6-fcdf-e697-1295-e9ec9604ec41@selasky.org> In-Reply-To: <3f62b9e2-214b-b1d4-f682-9318f77f315d@kondratyev.su> References: <202201182015.20IKFaWL053942@gitrepo.freebsd.org> <YechbCTSWuUs%2BNr5@kib.kiev.ua> <540a6a93-3101-02e8-b86a-50caa19f9653@kondratyev.su> <f83b405b-43c9-2dd5-00f0-2dcceb7d132f@selasky.org> <3f62b9e2-214b-b1d4-f682-9318f77f315d@kondratyev.su>
next in thread | previous in thread | raw e-mail | index | archive | help
On 1/19/22 23:02, Vladimir Kondratyev wrote:
> On 19.01.2022 12:50, Hans Petter Selasky wrote:
>> On 1/18/22 22:35, Vladimir Kondratyev wrote:
>>>
>>> Any ideas how to avoid it in a generic way?
>>
>> Hi,
>>
>> On a non-SMP system this will lead to deadlock.
>>
>> Is it possible you can pre-lock this spin lock earlier, so that it is
>> already locked, so instead of
>>
>> while(trylock());
>>
>> You have:
>>
>> assert (trylock() XXX)
>>
>
> Unfortunately, vulnerable functions are called in too many code paths to
> patch them all.
>
>> Or else,
>>
>> convert this particular lock to a native FreeBSD spinlock mutex.
>>
> It can be done for wake_up() but not for dma_fence_signal() which
> suffers from this problem too. Some code that uses that lock expect it
> to be spinlock_t
>
> I think we can just drop critical section in seqlock-related part of
> dma-buf code and replace it with rwlock as kib@ and mjg@ suggested.
> Leave seqlock for actual locking to preserve semantics as much as
> possible and add rwlock to implement reader's blocking. Following
> snippets show code conversion required for this change:
>
>
> Lock seqlock as reader:
>
> retry:
> seq = read_seqcount_begin(&obj->seq);
>
> ... reader payload ...
>
> if (read_seqcount_retry(&obj->seq, seq)) {
> #ifdef __FreeBSD__
> /* Wait for rwlock to be released by writer */
> rw_rlock(&obj->rwlock);
> rw_runlock(&obj->rwlock);
> #endif
> goto retry;
> }
>
>
> Lock seqlock as writer:
>
> #ifdef __linux__
> preempt_disable();
> #elif defined (__FreeBSD__)
> rw_wlock(&obj->rwlock);
> #endif
> write_seqcount_begin(&obj->seq);
>
> ... writer payload ...
>
> write_seqcount_end(&obj->seq);
> #ifdef __linux__
> preempt_enable();
> #elif defined (__FreeBSD__)
> rw_wunlock(&obj->rwlock);
> #endif
>
Hi Vladimir,
Waiting for a differential revision.
--HPS
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?6b3eabe6-fcdf-e697-1295-e9ec9604ec41>