From nobody Thu Mar 6 14:56:27 2025 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Z7svF66fZz5pl4Q for ; Thu, 06 Mar 2025 14:56:41 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-pl1-x62a.google.com (mail-pl1-x62a.google.com [IPv6:2607:f8b0:4864:20::62a]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "WR4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Z7svF3BWTz3QgN for ; Thu, 06 Mar 2025 14:56:41 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-pl1-x62a.google.com with SMTP id d9443c01a7336-22359001f1aso17746095ad.3 for ; Thu, 06 Mar 2025 06:56:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20230601.gappssmtp.com; s=20230601; t=1741272999; x=1741877799; darn=freebsd.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=+HCPnkoeW0uXRkQ3PIJmRPtKK/op9+myfsondPk+9uE=; b=uKZZrI6sIjjj7TOlWo7IQu+pGTvyThnhizCdaQ4pzULgJ8PGOnz4tmdZm21vDrR/Ar 0ZwCJN+sP35d5WbTpPAJWeP3U4NPCjszSuxEMiJE/Ku1bdIpjf/8TgRJtQ93uLZsX6zC q3NbUvXH+o9sqlhxPACR1EXL+2N7ObzVuPumOZ00vDBn1ZHHzule4bHGUnZjECoIf/Q4 +k7fWhKtdORCVnaICPY+7TRjhe3YkmdeXU6LHLVG5eGBi+1dr42wz72imhoy+9Y+M3o7 ddSa4dt7bdbxuqlVWXgQBym+v5Iir0E1RB9vvKzMvil+FR5uAfTDUAZASpm/zkn97Aym HxKQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741272999; x=1741877799; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=+HCPnkoeW0uXRkQ3PIJmRPtKK/op9+myfsondPk+9uE=; b=epmJAIGyRsfKaW5SuaZJTR4K6NUv2rLVbuOd4GQNJP8s7Y4iRHVrLTbkyDtkS/5sYQ ln+A+Nkh0YYi1jjcDdoD1zUhSFfdFSv+hHBPZr5CdDncsCL2DJ9JqhMDckFtyKGKY5fN 1GxHgm30fRZeCz7U7bS32QI1X14SOzlMghxsriTFWOkWJfIGBRJBd29Z9R4IaqfxG6St Wma+ZBj1p8MSp7u4GaWrHu2XaXLxM05Xllx3gzVPcPPP7K7QkVrgyFu6XHk4sCcpOpta ioY3mtgln0QvHsnrI1wXuFlgd7NnOWqBX5QCeINC8Joeo/KP8fn8QCPb75q12mZY13Cu 3BVA== X-Forwarded-Encrypted: i=1; AJvYcCXmOhq2ktHEnkuegI9WImZbMib0cRhg9b/A0/5Xx6Dnga/dFwhjDtj1QudUZZG3VuB8jRAJiM2wIHPGn8PY0+byQWzl@freebsd.org X-Gm-Message-State: AOJu0YyMieLPOYjNjMvJ7YrsmFuqn2cE6sE4xMmaqP29CwFpKFSKAVL1 C9h9lqaD0NmH0hypQtvcr90furj4opzFDgNth/w4rFi5siZGsbXfkDymaa87FVcldad4WVsAsu6 rRAL37q1Tq5j6Y4Co6O87cOGbRgi31FUiKaRBYA== X-Gm-Gg: ASbGncsmiPdyKlu2MMTIpq2KT4f6Rb/Zfdr13boySaQPTd9SMDhjCEmCI8MhUbYeaO6 i2WOn2BpObTkmF3JXXbFf4VhPg+m89s1+dPFzOOc0EEjItPP0MI4yc0PI0td0lrCGJ3mYkWVw/P xnMugBQ/tqKx7jEouPK9Muf/gaIDTQEEzyVT7JDcYwkECKEPomhKRmxj8DpEW9 X-Google-Smtp-Source: AGHT+IF8VHjMwkI04Ur4cDkeXBdzwNli1AdP4b0L6kSk1oH58Y+6NYijwmZ59oZmpm5c5w87AafhPskY56Nooc9TFME= X-Received: by 2002:a05:6a00:2e9f:b0:736:6d4d:ffa6 with SMTP id d2e1a72fcca58-73682c52f85mr11096162b3a.15.1741272998706; Thu, 06 Mar 2025 06:56:38 -0800 (PST) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 References: <202503061103.526B32Id022652@gitrepo.freebsd.org> In-Reply-To: From: Warner Losh Date: Thu, 6 Mar 2025 06:56:27 -0800 X-Gm-Features: AQ5f1JppwZ2pTNwdQSmP4RmfNk2j3zkuqYnXfXK7PQihZwEQwVlaBrufiiPEIUI Message-ID: Subject: Re: git: 234683726708 - main - devclass: make devclass_alloc_unit use M_NOWAIT To: John Baldwin Cc: Mateusz Guzik , Zhenlei Huang , Mateusz Guzik , src-committers , "" , Warner Losh , "" Content-Type: multipart/alternative; boundary="0000000000008ba1a2062fadb614" X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US] X-Rspamd-Queue-Id: 4Z7svF3BWTz3QgN X-Spamd-Bar: ---- --0000000000008ba1a2062fadb614 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, Mar 6, 2025, 5:33=E2=80=AFAM John Baldwin wrote: > On 3/6/25 06:35, Mateusz Guzik wrote: > > On Thu, Mar 6, 2025 at 12:32=E2=80=AFPM Zhenlei Huang wrote: > >> > >> > >> > >> On Mar 6, 2025, at 7:03 PM, Mateusz Guzik wrote: > >> > >> The branch main has been updated by mjg: > >> > >> URL: > https://cgit.FreeBSD.org/src/commit/?id=3D234683726708cf5212d672d676d3005= 6d4133859 > >> > >> commit 234683726708cf5212d672d676d30056d4133859 > >> Author: Mateusz Guzik > >> AuthorDate: 2025-03-06 11:01:49 +0000 > >> Commit: Mateusz Guzik > >> 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, > Warner 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 > locks held: > > exclusive sleep mutex SD slot mtx (sdhci) r =3D 0 (0xd8dec028) locked @ > > > /var/jenkins/workspace/pfSense-Plus-snapshots-25_03-main/sources/FreeBSD-= 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_swi? > PCI hotplug defines its own thread taskqueue for adding and removing > devices. > > 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. > I should dust off the branch that i have this one. There's about a dozen places I had to change at the time... 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. > Its a holdover from spl days for sure. I will fix sdhci to use a proper taskqueue and then revert this commit. > Thanks. You can add me to the review Warner > -- > John Baldwin > > --0000000000008ba1a2062fadb614 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Thu, Mar 6, 2025, 5:33=E2=80= =AFAM John Baldwin <jhb@freebsd.org> wrote:
On 3/6/25 06:35, Mate= usz Guzik wrote:
> On Thu, Mar 6, 2025 at 12:32=E2=80=AFPM Zhenlei Huang <zlei@freebsd.o= rg> 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=3D234683726708cf5212d672d676= d30056d4133859
>>
>> commit 234683726708cf5212d672d676d30056d4133859
>> Author:=C2=A0 =C2=A0 =C2=A0Mateusz Guzik <mjg@FreeBSD.org> >> AuthorDate: 2025-03-06 11:01:49 +0000
>> Commit:=C2=A0 =C2=A0 =C2=A0Mateusz Guzik <mjg@FreeBSD.org> >> CommitDate: 2025-03-06 11:01:49 +0000
>>
>>=C2=A0 =C2=A0 =C2=A0devclass: make devclass_alloc_unit use M_NOWAIT=
>>
>>=C2=A0 =C2=A0 =C2=A0The only caller already does this.
>>
>>=C2=A0 =C2=A0 =C2=A0The routine can be called with a mutex held mak= ing M_WAITOK illegal.
>>
>>=C2=A0 =C2=A0 =C2=A0Sponsored by:=C2=A0 =C2=A0Rubicon Communication= s, 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,
>> -=C2=A0 =C2=A0 newsize * sizeof(*dc->devices), M_BUS, M_WAITOK)= ;
>> + devices =3D reallocf(dc->devices,
>> +=C2=A0 =C2=A0 newsize * sizeof(*dc->devices), M_BUS, M_NOWAIT)= ;
>>
>>
>> I'd recommend against this. From the commit message of f3d3c63= 442ff, Warner 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() shoul= d use 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-sl= eepable locks held:
> exclusive sleep mutex SD slot mtx (sdhci) r =3D 0 (0xd8dec028) locked = @
> /var/jenkins/workspace/pfSense-Plus-snapshots-25_03-main/sources/FreeB= SD-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_swi= ?
PCI hotplug defines its own thread taskqueue for adding and removing device= s.

The bug is here, IMO.=C2=A0 Eventually new-bus will need some sort of topol= ogy
lock and that will have to be an sx lock, so this code needs to change
anyway.=C2=A0 The sound code that tries to frob devices with a regular mute= x
also needs to change.

I should dust off the branch that i have this one. The= re's about a dozen places I had to change at the time...

In terms of taskqueue_swi, it's probably something that needs to go awa= y.
Generally speaking, code uses tasks for functions that need to sleep,
and taskqueue_swi breaks that.

Its a holdover from spl days for sure.
<= div dir=3D"auto">
I will fix sdhci to use a proper taskqueue and then revert this commit.
=

Than= ks. You can add me to the review=C2=A0

Warner
--
John Baldwin

--0000000000008ba1a2062fadb614--