Date: Wed, 24 Jul 2024 14:20:23 -0600 From: Warner Losh <imp@bsdimp.com> To: mmel@freebsd.org Cc: Konstantin Belousov <kib@freebsd.org>, John F Carr <jfc@mit.edu>, Mark Millard <marklmi@yahoo.com>, FreeBSD Current <freebsd-current@freebsd.org>, "freebsd-arm@freebsd.org" <freebsd-arm@freebsd.org> Subject: Re: armv7-on-aarch64 stuck at urdlck Message-ID: <CANCZdfqQxUtjN6u=t08UovSMK1y_OPvEYA2SFjT08zgs9Vpabw@mail.gmail.com> In-Reply-To: <28484869-05fd-4391-9501-10b93280f7a4@freebsd.org> References: <33251aa3-681f-4d17-afe9-953490afeaf0@gmail.com> <0DD19771-3AAB-469E-981B-1203F1C28233@yahoo.com> <be023545-2b25-49ec-b6f1-9e05cd402646@gmail.com> <Zp95qtxK0CeDdp-d@kib.kiev.ua> <6a969609-fa0e-419d-83d5-e4fcf0f6ec35@freebsd.org> <FABF7440-70D2-4BAB-8B0B-4CA950CFFA60@mit.edu> <ZqDWSU_h5J1fYCrz@kib.kiev.ua> <f39b16b5-bbfb-4011-92fb-834330841533@freebsd.org> <ZqDcamh6r3B-oEB-@kib.kiev.ua> <A7348370-0BEE-4EA4-8521-03C07F025F40@mit.edu> <ZqEiBJF7UxcUWCQ2@kib.kiev.ua> <28484869-05fd-4391-9501-10b93280f7a4@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--000000000000bab606061e040273 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, Jul 24, 2024 at 11:34=E2=80=AFAM mmel@freebsd.org <meloun.michal@gm= ail.com> wrote: > > > On 24.07.2024 17:47, Konstantin Belousov wrote: > > On Wed, Jul 24, 2024 at 01:07:39PM +0000, John F Carr wrote: > >> > >> > >>> On Jul 24, 2024, at 06:50, Konstantin Belousov <kib@freebsd.org> > wrote: > >>> > >>> On Wed, Jul 24, 2024 at 12:34:57PM +0200, mmel@freebsd.org wrote: > >>>> > >>>> > >>>> On 24.07.2024 12:24, Konstantin Belousov wrote: > >>>>> On Tue, Jul 23, 2024 at 08:11:13PM +0000, John F Carr wrote: > >>>>>> On Jul 23, 2024, at 13:46, Michal Meloun <meloun.michal@gmail.com> > wrote: > >>>>>>> > >>>>>>> On 23.07.2024 11:36, Konstantin Belousov wrote: > >>>>>>>> On Tue, Jul 23, 2024 at 09:53:41AM +0200, Michal Meloun wrote: > >>>>>>>>> The good news is that I'm finally able to generate a > working/locking > >>>>>>>>> test case. The culprit (at least for me) is if "-mcpu" is used > when > >>>>>>>>> compiling libthr (e.g. indirectly injected via CPUTYPE in > /etc/make.conf). > >>>>>>>>> If it is not used, libthr is broken (regardless of -O level or > debug/normal > >>>>>>>>> build), but -mcpu=3Dcortex-a15 will always produce a working > libthr. > >>>>>>>> I think this is very significant progress. > >>>>>>>> Do you plan to drill down more to see what is going on? > >>>>>>> > >>>>>>> So the problem is now clear, and I fear it may apply to other > architectures as well. > >>>>>>> dlopen_object() (from rtld_elf), > >>>>>>> https://cgit.freebsd.org/src/tree/libexec/rtld-elf/rtld.c#n3766, > >>>>>>> holds the rtld_bind_lock write lock for almost the entire time a > new library is loaded. > >>>>>>> If the code uses a yet unresolved symbol to load the library, the > rtl_bind() function attempts to get read lock of rtld_bind_lock and a > deadlock occurs. > >>>>>>> > >>>>>>> In this case, it round_up() in _thr_stack_fix_protection, > >>>>>>> > https://cgit.freebsd.org/src/tree/lib/libthr/thread/thr_stack.c#n136. > >>>>>>> Issued by __aeabi_uidiv (since not all armv7 processors support H= W > divide). > >>>>>>> > >>>>>>> Unfortunately, I'm not sure how to fix it. The compiler can emit > __aeabi_<> in any place, and I'm not sure if it can resolve all the symbo= ls > used by rtld_eld and libthr beforehand. > >>>>>>> > >>>>>>> > >>>>>>> Michal > >>>>>>> > >>>>>> > >>>>>> In this case (but not for all _aeabi_ functions) we can avoid > division > >>>>>> as long as page size is a power of 2. > >>>>>> > >>>>>> The function is > >>>>>> > >>>>>> static inline size_t > >>>>>> round_up(size_t size) > >>>>>> { > >>>>>> if (size % _thr_page_size !=3D 0) > >>>>>> size =3D ((size / _thr_page_size) + 1) * > >>>>>> _thr_page_size; > >>>>>> return size; > >>>>>> } > >>>>>> > >>>>>> The body can be condensed to > >>>>>> > >>>>>> return (size + _thr_page_size - 1) & ~(_thr_page_size - 1); > >>>>>> > >>>>>> This is shorter in both lines of code and instruction bytes. > >>>>> > >>>>> Lets not allow this to be lost. Could anybody confirm that the pat= ch > >>>>> below fixes the issue? > >>>>> > >>>>> commit d560f4f6690a48476565278fd07ca131bf4eeb3c > >>>>> Author: Konstantin Belousov <kib@FreeBSD.org> > >>>>> Date: Wed Jul 24 13:17:55 2024 +0300 > >>>>> > >>>>> rtld: avoid division in __thr_map_stacks_exec() > >>>>> The function is called by rtld with the rtld bind lock > write-locked, > >>>>> when fixing the stack permission during dso load. Not every > ARMv7 CPU > >>>>> supports the div, which causes the recursive entry into rtld t= o > resolve > >>>>> the __aeabi_uidiv symbol, causing self-lock. > >>>>> Workaround the problem by using roundup2() instead of > open-coding less > >>>>> efficient formula. > >>>>> Diagnosed by: mmel > >>>>> Based on submission by: John F Carr <jfc@mit.edu> > >>>>> Sponsored by: The FreeBSD Foundation > >>>>> MFC after: 1 week > >>>>> > >>> Just realized that it is wrong. Stack size is user-controlled and it > does > >>> not need to be power of two. > >> > >> Your change is correct. _thr_page_size is set to getpagesize(), > >> which is a power of 2. The call to roundup2 takes a user-provided > >> size and rounds it up to a multiple of the system page size. > >> > >> I tested the change and it works. My change also works and > >> should compile to identical code. I forgot there was a standard > >> function to do the rounding. > > Right, my bad, thank you for correcting my thinko. > > > >> > >>> For final resolving of deadlocks, after a full day of digging, I'm > very much > >>>> incline of adding -znow to the linker flags for libthr.so (and mayb= e > also > >>>> for ld-elf.so). The runtime cost of resolving all symbols at startup > is very > >>>> low. Direct pre-solving in _thr_rtld_init() is problematic for the > _aeabi_* > >>>> symbols, since they don't have an official C prototypes, and some ar= e > not > >>>> compatible with C calling conventions. > >>> I do not like it. `-z now' changes (breaks) the ABI and makes some > symbols > >>> not preemtible. > >>> > >>> In the worst case, we would need a call to the asm routine which > causes the > >>> resolution of the _eabi_* symbols on arm. > >>> > >> > >> It would also be possible to link libthr with libgcc.a and use a linke= r > map > >> to hide the _eabi_ symbols. > > In principle yes, but if the ARM ABI states that _eabi symbols must be > used, > > and exported from libc, then this is also some form of ABI breakage. > > I hope that https://reviews.freebsd.org/D46104 is acceptable :) > Can't speak for kib, but it looks good to my eye (though I agree with his naming quibble). And helps avoid -znow, though I could have gone either way on that. It's also simple enough not to be a burden. Warner --000000000000bab606061e040273 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable <div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">= <div dir=3D"ltr" class=3D"gmail_attr">On Wed, Jul 24, 2024 at 11:34=E2=80= =AFAM <a href=3D"mailto:mmel@freebsd.org">mmel@freebsd.org</a> <<a href= =3D"mailto:meloun.michal@gmail.com">meloun.michal@gmail.com</a>> wrote:<= br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8e= x;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br> <br> On 24.07.2024 17:47, Konstantin Belousov wrote:<br> > On Wed, Jul 24, 2024 at 01:07:39PM +0000, John F Carr wrote:<br> >><br> >><br> >>> On Jul 24, 2024, at 06:50, Konstantin Belousov <<a href=3D"= mailto:kib@freebsd.org" target=3D"_blank">kib@freebsd.org</a>> wrote:<br= > >>><br> >>> On Wed, Jul 24, 2024 at 12:34:57PM +0200, <a href=3D"mailto:mm= el@freebsd.org" target=3D"_blank">mmel@freebsd.org</a> wrote:<br> >>>><br> >>>><br> >>>> On 24.07.2024 12:24, Konstantin Belousov wrote:<br> >>>>> On Tue, Jul 23, 2024 at 08:11:13PM +0000, John F Carr = wrote:<br> >>>>>> On Jul 23, 2024, at 13:46, Michal Meloun <<a hr= ef=3D"mailto:meloun.michal@gmail.com" target=3D"_blank">meloun.michal@gmail= .com</a>> wrote:<br> >>>>>>><br> >>>>>>> On 23.07.2024 11:36, Konstantin Belousov wrote= :<br> >>>>>>>> On Tue, Jul 23, 2024 at 09:53:41AM +0200, = Michal Meloun wrote:<br> >>>>>>>>> The good news is that I'm finally = able to generate a working/locking<br> >>>>>>>>> test case.=C2=A0 The culprit (at least= for me) is if "-mcpu" is used when<br> >>>>>>>>> compiling libthr (e.g. indirectly inje= cted via CPUTYPE in /etc/make.conf).<br> >>>>>>>>> If it is not used, libthr is broken (r= egardless of -O level or debug/normal<br> >>>>>>>>> build), but -mcpu=3Dcortex-a15 will al= ways produce a working libthr.<br> >>>>>>>> I think this is very significant progress.= <br> >>>>>>>> Do you plan to drill down more to see what= is going on?<br> >>>>>>><br> >>>>>>> So the problem is now clear, and I fear it may= apply to other architectures as well.<br> >>>>>>> dlopen_object() (from rtld_elf),<br> >>>>>>> <a href=3D"https://cgit.freebsd.org/src/tree/l= ibexec/rtld-elf/rtld.c#n3766" rel=3D"noreferrer" target=3D"_blank">https://= cgit.freebsd.org/src/tree/libexec/rtld-elf/rtld.c#n3766</a>,<br> >>>>>>> holds the rtld_bind_lock write lock for almost= the entire time a new library is loaded.<br> >>>>>>> If the code uses a yet unresolved symbol to lo= ad the library, the rtl_bind() function attempts to get read lock of=C2=A0 = rtld_bind_lock and a deadlock occurs.<br> >>>>>>><br> >>>>>>> In this case, it round_up() in _thr_stack_fix_= protection,<br> >>>>>>> <a href=3D"https://cgit.freebsd.org/src/tree/l= ib/libthr/thread/thr_stack.c#n136" rel=3D"noreferrer" target=3D"_blank">htt= ps://cgit.freebsd.org/src/tree/lib/libthr/thread/thr_stack.c#n136</a>.<br> >>>>>>> Issued by __aeabi_uidiv (since not all armv7 p= rocessors support HW divide).<br> >>>>>>><br> >>>>>>> Unfortunately, I'm not sure how to fix it.= =C2=A0 The compiler can emit __aeabi_<> in any place, and I'm not= sure if it can resolve all the symbols used by rtld_eld and libthr beforeh= and.<br> >>>>>>><br> >>>>>>><br> >>>>>>> Michal<br> >>>>>>><br> >>>>>><br> >>>>>> In this case (but not for all _aeabi_ functions) w= e can avoid division<br> >>>>>> as long as page size is a power of 2.<br> >>>>>><br> >>>>>> The function is<br> >>>>>><br> >>>>>>=C2=A0 =C2=A0 static inline size_t<br> >>>>>>=C2=A0 =C2=A0 round_up(size_t size)<br> >>>>>>=C2=A0 =C2=A0 {<br> >>>>>>=C2=A0 =C2=A0 =C2=A0if (size % _thr_page_size !=3D = 0)<br> >>>>>>=C2=A0 =C2=A0 =C2=A0size =3D ((size / _thr_page_siz= e) + 1) *<br> >>>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0_thr_page_size;<b= r> >>>>>>=C2=A0 =C2=A0 =C2=A0return size;<br> >>>>>>=C2=A0 =C2=A0 }<br> >>>>>><br> >>>>>> The body can be condensed to<br> >>>>>><br> >>>>>>=C2=A0 =C2=A0 return (size + _thr_page_size - 1) &a= mp; ~(_thr_page_size - 1);<br> >>>>>><br> >>>>>> This is shorter in both lines of code and instruct= ion bytes.<br> >>>>><br> >>>>> Lets not allow this to be lost.=C2=A0 Could anybody co= nfirm that the patch<br> >>>>> below fixes the issue?<br> >>>>><br> >>>>> commit d560f4f6690a48476565278fd07ca131bf4eeb3c<br> >>>>> Author: Konstantin Belousov <kib@FreeBSD.org><br= > >>>>> Date:=C2=A0 =C2=A0Wed Jul 24 13:17:55 2024 +0300<br> >>>>><br> >>>>>=C2=A0 =C2=A0 =C2=A0 rtld: avoid division in __thr_map_= stacks_exec()<br> >>>>>=C2=A0 =C2=A0 =C2=A0 The function is called by rtld wit= h the rtld bind lock write-locked,<br> >>>>>=C2=A0 =C2=A0 =C2=A0 when fixing the stack permission d= uring dso load.=C2=A0 Not every ARMv7 CPU<br> >>>>>=C2=A0 =C2=A0 =C2=A0 supports the div, which causes the= recursive entry into rtld to resolve<br> >>>>>=C2=A0 =C2=A0 =C2=A0 the=C2=A0 __aeabi_uidiv symbol, ca= using self-lock.<br> >>>>>=C2=A0 =C2=A0 =C2=A0 Workaround the problem by using ro= undup2() instead of open-coding less<br> >>>>>=C2=A0 =C2=A0 =C2=A0 efficient formula.<br> >>>>>=C2=A0 =C2=A0 =C2=A0 Diagnosed by:=C2=A0 =C2=A0mmel<br> >>>>>=C2=A0 =C2=A0 =C2=A0 Based on submission by: John F Car= r <<a href=3D"mailto:jfc@mit.edu" target=3D"_blank">jfc@mit.edu</a>><= br> >>>>>=C2=A0 =C2=A0 =C2=A0 Sponsored by:=C2=A0 =C2=A0The Free= BSD Foundation<br> >>>>>=C2=A0 =C2=A0 =C2=A0 MFC after:=C2=A0 =C2=A0 =C2=A0 1 w= eek<br> >>>>><br> >>> Just realized that it is wrong.=C2=A0 Stack size is user-contr= olled and it does<br> >>> not need to be power of two.<br> >><br> >> Your change is correct.=C2=A0 _thr_page_size is set to getpagesize= (),<br> >> which is a power of 2.=C2=A0 =C2=A0The call to roundup2 takes a us= er-provided<br> >> size and rounds it up to a multiple of the system page size.<br> >><br> >> I tested the change and it works.=C2=A0 My change also works and<b= r> >> should compile to identical code.=C2=A0 I forgot there was a stand= ard<br> >> function to do the rounding.<br> > Right, my bad, thank you for correcting my thinko.<br> > <br> >><br> >>> For final resolving of deadlocks, after a full day of digging,= I'm very much<br> >>>> incline=C2=A0 of adding -znow to the linker flags for libt= hr.so (and maybe also<br> >>>> for ld-elf.so). The runtime cost of resolving all symbols = at startup is very<br> >>>> low. Direct pre-solving in _thr_rtld_init() is problematic= for the _aeabi_*<br> >>>> symbols, since they don't have an official C prototype= s, and some are not<br> >>>> compatible with C calling conventions.<br> >>> I do not like it. `-z now' changes (breaks) the ABI and ma= kes some symbols<br> >>> not preemtible.<br> >>><br> >>> In the worst case, we would need a call to the asm routine whi= ch causes the<br> >>> resolution of the _eabi_* symbols on arm.<br> >>><br> >><br> >> It would also be possible to link libthr with libgcc.a and use a l= inker map<br> >> to hide the _eabi_ symbols.<br> > In principle yes, but if the ARM ABI states that _eabi symbols must be= used,<br> > and exported from libc, then this is also some form of ABI breakage.<b= r> <br> I hope that <a href=3D"https://reviews.freebsd.org/D46104" rel=3D"noreferre= r" target=3D"_blank">https://reviews.freebsd.org/D46104</a>; is acceptable := )<br></blockquote><div><br></div><div>Can't speak for kib, but it looks= good to my eye (though I agree with his naming</div><div>quibble). And hel= ps avoid -znow, though I could have gone either way on that.</div><div>It&#= 39;s also simple enough not to be a burden.</div><div><br></div><div>Warner= =C2=A0</div></div></div> --000000000000bab606061e040273--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfqQxUtjN6u=t08UovSMK1y_OPvEYA2SFjT08zgs9Vpabw>