snd_hdsp*: Free up channel resources in case of CHANNEL_INIT() failure

[christos]

Summary

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

[christos]

@dev_submerge.ch You are the only one I know who owns a HDSP card, so could you give this a quick test?

[dev_submerge.ch]

In D45982#1048497, @christos wrote:

@dev_submerge.ch You are the only one I know who owns a HDSP card, so could you give this a quick test?

I suppose it's ok, although the code change does not run in normal circumstances. I'll try to provoke that with a hack before I give you a go.

While testing with snd_hdspe I ran into the following warning, but I can't really make sense of it:

Jul 21 17:36:14 current kernel: uma_zalloc_debug: zone "malloc-16384" with the following non-sleepable locks
held:
Jul 21 17:36:14 current kernel: exclusive sleep mutex pcm6:play:dsp6.p0 (pcm play channel) r = 0 (0xfffff8000
128dd80) locked @ /usr/src/sys/dev/sound/pcm/sndstat.c:1237
Jul 21 17:36:14 current kernel: stack backtrace:
Jul 21 17:36:14 current kernel: #0 0xffffffff80bc1b8c at witness_debugger+0x6c
Jul 21 17:36:14 current kernel: #1 0xffffffff80bc2d83 at witness_warn+0x403
Jul 21 17:36:14 current kernel: #2 0xffffffff80ee2e74 at uma_zalloc_debug+0x34
Jul 21 17:36:14 current kernel: #3 0xffffffff80ee2997 at uma_zalloc_arg+0x27
Jul 21 17:36:14 current kernel: #4 0xffffffff80b1fcbd at malloc+0x7d
Jul 21 17:36:14 current kernel: #5 0xffffffff80ba9cab at sbuf_extend+0x7b
Jul 21 17:36:14 current kernel: #6 0xffffffff80baa26a at sbuf_put_byte+0xda
Jul 21 17:36:14 current kernel: #7 0xffffffff80ba2e56 at kvprintf+0xe6
Jul 21 17:36:14 current kernel: #8 0xffffffff80baa0ad at sbuf_vprintf+0x6d
Jul 21 17:36:14 current kernel: #9 0xffffffff80baa15f at sbuf_printf+0x3f
Jul 21 17:36:14 current kernel: #10 0xffffffff808dd793 at sndstat_read+0x783
Jul 21 17:36:14 current kernel: #11 0xffffffff809d70e4 at devfs_read_f+0xe4
Jul 21 17:36:14 current kernel: #12 0xffffffff80bc6890 at dofileread+0x80
Jul 21 17:36:14 current kernel: #13 0xffffffff80bc6417 at sys_read+0xb7
Jul 21 17:36:14 current kernel: #14 0xffffffff81068888 at amd64_syscall+0x158
Jul 21 17:36:14 current kernel: #15 0xffffffff8103aaeb at fast_syscall_common+0xf8

[dev_submerge.ch]

In D45982#1049978, @dev_submerge.ch wrote:

In D45982#1048497, @christos wrote:

@dev_submerge.ch You are the only one I know who owns a HDSP card, so could you give this a quick test?

I suppose it's ok, although the code change does not run in normal circumstances. I'll try to provoke that with a hack before I give you a go.

Ok, this change fails gracefully AFAICT, the pcm devices are still created, but as mixer only.

While testing with snd_hdspe I ran into the following warning, but I can't really make sense of it:

Jul 21 17:36:14 current kernel: uma_zalloc_debug: zone "malloc-16384" with the following non-sleepable locks
held:
Jul 21 17:36:14 current kernel: exclusive sleep mutex pcm6:play:dsp6.p0 (pcm play channel) r = 0 (0xfffff8000
128dd80) locked @ /usr/src/sys/dev/sound/pcm/sndstat.c:1237
...

This is certainly provoked by the length of cat /dev/sndstat output with the lot of HDSPe pcm devices. Not related to this change, but should we make sure that sbuf_printf() does not malloc while holding the mutex, or can this be ignored?

[markj]

In D45982#1049978, @dev_submerge.ch wrote:

While testing with snd_hdspe I ran into the following warning, but I can't really make sense of it:

Jul 21 17:36:14 current kernel: uma_zalloc_debug: zone "malloc-16384" with the following non-sleepable locks
held:

This is a consequence of the fact that sndstat_read() works by allocating an sbuf, which may grow as data is added, then copying out the resulting buffer via uiomove().

In general, blocking memory allocations are not permitted while holding a mtx(9) mutex, hence the warning. To work around this, one would typically add a "drain" callback function to the sbuf, that it calls to write out data once the buffer is full. In this case, the callback would write out buffered data via uiomove(), after which the buffer can be reused and thus doesn't need to grow. See sbuf_set_drain(9).

[christos]

In D45982#1049997, @markj wrote:

In D45982#1049978, @dev_submerge.ch wrote:

While testing with snd_hdspe I ran into the following warning, but I can't really make sense of it:

Jul 21 17:36:14 current kernel: uma_zalloc_debug: zone "malloc-16384" with the following non-sleepable locks
held:

This is a consequence of the fact that sndstat_read() works by allocating an sbuf, which may grow as data is added, then copying out the resulting buffer via uiomove().

In general, blocking memory allocations are not permitted while holding a mtx(9) mutex, hence the warning. To work around this, one would typically add a "drain" callback function to the sbuf, that it calls to write out data once the buffer is full. In this case, the callback would write out buffered data via uiomove(), after which the buffer can be reused and thus doesn't need to grow. See sbuf_set_drain(9).

I will take care of this. Thanks.

[markj]

In D45982#1050048, @christos wrote:

In D45982#1049997, @markj wrote:

In D45982#1049978, @dev_submerge.ch wrote:

While testing with snd_hdspe I ran into the following warning, but I can't really make sense of it:

Jul 21 17:36:14 current kernel: uma_zalloc_debug: zone "malloc-16384" with the following non-sleepable locks
held:

This is a consequence of the fact that sndstat_read() works by allocating an sbuf, which may grow as data is added, then copying out the resulting buffer via uiomove().

In general, blocking memory allocations are not permitted while holding a mtx(9) mutex, hence the warning. To work around this, one would typically add a "drain" callback function to the sbuf, that it calls to write out data once the buffer is full. In this case, the callback would write out buffered data via uiomove(), after which the buffer can be reused and thus doesn't need to grow. See sbuf_set_drain(9).

I will take care of this. Thanks.

Thinking about this again, my suggestion won't work since uiomove() also can't be called with a mutex held. (It's possible for copyout() to take a page fault, and the page fault handler may sleep.)

One other approach is to build the output twice, the first time is used to get the length of the output, without writing to the buffer. Then one allocates the buffer using the provided size, and then actually fills it.

[christos]

In D45982#1050109, @markj wrote:

In D45982#1050048, @christos wrote:

In D45982#1049997, @markj wrote:

In D45982#1049978, @dev_submerge.ch wrote:

While testing with snd_hdspe I ran into the following warning, but I can't really make sense of it:

Jul 21 17:36:14 current kernel: uma_zalloc_debug: zone "malloc-16384" with the following non-sleepable locks
held:

This is a consequence of the fact that sndstat_read() works by allocating an sbuf, which may grow as data is added, then copying out the resulting buffer via uiomove().

In general, blocking memory allocations are not permitted while holding a mtx(9) mutex, hence the warning. To work around this, one would typically add a "drain" callback function to the sbuf, that it calls to write out data once the buffer is full. In this case, the callback would write out buffered data via uiomove(), after which the buffer can be reused and thus doesn't need to grow. See sbuf_set_drain(9).

I will take care of this. Thanks.

Thinking about this again, my suggestion won't work since uiomove() also can't be called with a mutex held. (It's possible for copyout() to take a page fault, and the page fault handler may sleep.)

One other approach is to build the output twice, the first time is used to get the length of the output, without writing to the buffer. Then one allocates the buffer using the provided size, and then actually fills it.

That would complicate the code significantly and make it more fragile - we'd need to create 2 length-counting functions, one for sndstat_build_sound4_nvlist() (although it's more unlikely that we'll hit the warning there, as the sbuf is re-created in each channel iteration, and is used only for the feederchain string, but still), and one for sndstat_prepare_pcm(), and duplicate their code so that we can count the lengths of the final outputs. This would make making changes to the output functions tedious and error prone, as we'd need to change the length-counting functions as well.

I think it would be better to re-structure the code a bit so that we can pass the uio and pcm_channel to the sbuf_set_drain() callback and call uiomove() there.