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>