Date: Thu, 6 Mar 2025 15:28:45 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: Zhenlei Huang <zlei@freebsd.org>, Mateusz Guzik <mjg@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, Warner Losh <imp@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org> Subject: Re: git: 234683726708 - main - devclass: make devclass_alloc_unit use M_NOWAIT Message-ID: <CAGudoHFS5-AcLZxN=rzidu0PK3Ar6QpUZ0QQeWKdb=Htmf41Ow@mail.gmail.com> In-Reply-To: <d09b8cd1-ac71-43a4-9625-e796fd142f16@FreeBSD.org> References: <202503061103.526B32Id022652@gitrepo.freebsd.org> <F1B3652E-0D0C-402A-8509-D510992DAC15@FreeBSD.org> <CAGudoHF=eRaHCcjRrvd4sG4-OBu0GrmVRpiHEeU1ayG=M9oXrg@mail.gmail.com> <d09b8cd1-ac71-43a4-9625-e796fd142f16@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Mar 6, 2025 at 2:33=E2=80=AFPM John Baldwin <jhb@freebsd.org> wrote= : > > On 3/6/25 06:35, Mateusz Guzik wrote: > > On Thu, Mar 6, 2025 at 12:32=E2=80=AFPM Zhenlei Huang <zlei@freebsd.org= > wrote: > >> > >> > >> > >> On Mar 6, 2025, at 7:03 PM, Mateusz Guzik <mjg@FreeBSD.org> wrote: > >> > >> The branch main has been updated by mjg: > >> > >> URL: https://cgit.FreeBSD.org/src/commit/?id=3D234683726708cf5212d672d= 676d30056d4133859 > >> > >> commit 234683726708cf5212d672d676d30056d4133859 > >> Author: Mateusz Guzik <mjg@FreeBSD.org> > >> AuthorDate: 2025-03-06 11:01:49 +0000 > >> Commit: Mateusz Guzik <mjg@FreeBSD.org> > >> CommitDate: 2025-03-06 11:01:49 +0000 > >> > >> devclass: make devclass_alloc_unit use M_NOWAIT > >> > >> The only caller already does this. > >> > >> The routine can be called with a mutex held making M_WAITOK illega= l. > >> > >> Sponsored by: Rubicon Communications, LLC ("Netgate") > >> --- > >> sys/kern/subr_bus.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c > >> index 9506e471705c..0422352bba51 100644 > >> --- a/sys/kern/subr_bus.c > >> +++ b/sys/kern/subr_bus.c > >> @@ -1208,6 +1208,7 @@ devclass_get_sysctl_tree(devclass_t dc) > >> static int > >> devclass_alloc_unit(devclass_t dc, device_t dev, int *unitp) > >> { > >> + device_t *devices; > >> const char *s; > >> int unit =3D *unitp; > >> > >> @@ -1264,8 +1265,11 @@ devclass_alloc_unit(devclass_t dc, device_t dev= , int *unitp) > >> int newsize; > >> > >> newsize =3D unit + 1; > >> - dc->devices =3D reallocf(dc->devices, > >> - newsize * sizeof(*dc->devices), M_BUS, M_WAITOK); > >> + devices =3D reallocf(dc->devices, > >> + newsize * sizeof(*dc->devices), M_BUS, M_NOWAIT); > >> > >> > >> I'd recommend against this. From the commit message of f3d3c63442ff, W= arner said, > >>> In addition, transition to M_WAITOK since this is a sleepable context > >> So, the M_WAITOK is intentional. > >> > >> Rather than reverting this, the caller devclass_add_device() should us= e M_WAITOK. > >> > > > > Per my commit message this is callable from a *NOT* sleepable context. > > > > Here is a splat we got at Netgate: > > > > uma_zalloc_debug: zone "malloc-16" with the following non-sleepable loc= ks held: > > exclusive sleep mutex SD slot mtx (sdhci) r =3D 0 (0xd8dec028) locked @ > > /var/jenkins/workspace/pfSense-Plus-snapshots-25_03-main/sources/FreeBS= D-src-plus-RELENG_25_03/sys/dev/sdhci/sdhci.c:688 > > stack backtrace: > > #0 0xc0330ebc at witness_debugger+0x78 > > #1 0xc033217c at witness_warn+0x428 > > #2 0xc05b0a58 at uma_zalloc_debug+0x34 > > #3 0xc05b067c at uma_zalloc_arg+0x30 > > #4 0xc0291760 at malloc+0x8c > > #5 0xc02920ec at reallocf+0x14 > > #6 0xc02f8894 at devclass_add_device+0x1e8 > > #7 0xc02f6c78 at make_device+0xe0 > > #8 0xc02f6abc at device_add_child_ordered+0x30 > > #9 0xc0156e0c at sdhci_card_task+0x238 > > #10 0xc0324090 at taskqueue_run_locked+0x1b4 > > #11 0xc0323ea0 at taskqueue_run+0x50 > > #12 0xc0275f88 at ithread_loop+0x264 > > Just use a regular taskqueue like taskqueue_thread instead of taskqueue_s= wi? > PCI hotplug defines its own thread taskqueue for adding and removing devi= ces. > > The bug is here, IMO. Eventually new-bus will need some sort of topology > lock and that will have to be an sx lock, so this code needs to change > anyway. The sound code that tries to frob devices with a regular mutex > also needs to change. > > In terms of taskqueue_swi, it's probably something that needs to go away. > Generally speaking, code uses tasks for functions that need to sleep, > and taskqueue_swi breaks that. > > I will fix sdhci to use a proper taskqueue and then revert this commit. > That's fine with me, but if you do, also make sure to change the M_NOWAIT into M_WAITOK in devclass_add_device: dev->nameunit =3D malloc(buflen, M_BUS, M_NOWAIT|M_ZERO); --=20 Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHFS5-AcLZxN=rzidu0PK3Ar6QpUZ0QQeWKdb=Htmf41Ow>