Re: git: 234683726708 - main - devclass: make devclass_alloc_unit use M_NOWAIT

On 3/6/25 06:35, Mateusz Guzik wrote:
> On Thu, Mar 6, 2025 at 12:32 PM 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=234683726708cf5212d672d676d30056d4133859
>>
>> 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 illegal.
>>
>>     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 = *unitp;
>>
>> @@ -1264,8 +1265,11 @@ devclass_alloc_unit(devclass_t dc, device_t dev, int *unitp)
>> int newsize;
>>
>> newsize = unit + 1;
>> - dc->devices = reallocf(dc->devices,
>> -    newsize * sizeof(*dc->devices), M_BUS, M_WAITOK);
>> + devices = 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 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-sleepable locks held:
> exclusive sleep mutex SD slot mtx (sdhci) r = 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.

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.

-- 
John Baldwin