Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 22 Jan 2022 14:04:20 +0300
From:      Vladimir Kondratyev <vladimir@kondratyev.su>
To:        Hans Petter Selasky <hps@selasky.org>, 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:  <105887a8-0ab0-e551-a883-79a055fb3c15@kondratyev.su>
In-Reply-To: <6b3eabe6-fcdf-e697-1295-e9ec9604ec41@selasky.org>
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> <6b3eabe6-fcdf-e697-1295-e9ec9604ec41@selasky.org>

next in thread | previous in thread | raw e-mail | index | archive | help
On 22.01.2022 12:49, Hans Petter Selasky wrote:
> 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=20
>>> locked, so instead of
>>>
>>> while(trylock());
>>>
>>> You have:
>>>
>>> assert (trylock() XXX)
>>>
>>
>> Unfortunately, vulnerable functions are called in too many code paths =
to patch=20
>> 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 suff=
ers from=20
>> this problem too. Some code that uses that lock expect it to be spinlo=
ck_t
>>
>> I think we can just drop critical section in seqlock-related part of d=
ma-buf=20
>> code and replace it with rwlock as kib@ and mjg@ suggested. Leave seql=
ock for=20
>> actual locking to preserve semantics as much as possible and add rwloc=
k to=20
>> implement reader's blocking. Following snippets show code conversion r=
equired=20
>> for this change:
>>
>>
>> Lock seqlock as reader:
>>
>> retry:
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0seq =3D read_seqcount_begin(&obj->seq);
>>
>> ... reader payload ...
>>
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (read_seqcount_retry(&obj->seq, seq))=
 {
>> #ifdef __FreeBSD__
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Wait for rwlock to=
 be released by writer */
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rw_rlock(&obj->rwlock=
);
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rw_runlock(&obj->rwlo=
ck);
>> #endif
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto retry;
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0}
>>
>>
>> Lock seqlock as writer:
>>
>> #ifdef __linux__
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0preempt_disable();
>> #elif defined (__FreeBSD__)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rw_wlock(&obj->rwlock);
>> #endif
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0write_seqcount_begin(&obj->seq);
>>
>> ... writer payload ...
>>
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0write_seqcount_end(&obj->seq);
>> #ifdef __linux__
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0preempt_enable();
>> #elif defined (__FreeBSD__)
>> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rw_wunlock(&obj->rwlock);
>> #endif
>>
>=20
> Hi Vladimir,
>=20
> Waiting for a differential revision.
>=20
> --HPS

See https://github.com/freebsd/drm-kmod/pull/138


--=20
WBR
Vladimir Kondratyev



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?105887a8-0ab0-e551-a883-79a055fb3c15>