sound: Use unr(9) to produce unique channel unit numbers

[christos]

Summary

Currently it is possible to assign a unit number that already exists.
Suppose the following channel list:

[vp2]

If we create 3 channels, we'll end up with the following list:

[vp0, vp1, vp2, vp2]

This happens because chn_init() does not check if the unit number we are
assigning already exists. While fixing this is trivial when the channel
list is sorted in ascending order, it is way more involved when sorted
in descending order. Even though sorting the list in descending order
would require deliberately modifying pcm_chn_add(), and is most likely
not going to happen, make the mechanism more robust by using a unr(9)
allocator for each channel type.

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

[kib]

sys/dev/sound/pcm/channel.c

1173

returning NULL there is equivalent to more obscure NULL-dereference panic at consumer. Might be, put __unreachable() there?

sys/dev/sound/pcm/sound.c

845

Did you tried unloading the module?

[christos]

sys/dev/sound/pcm/channel.c

1173

chn_init() makes sure only valid types are assigned, so returning NULL shouldn't be a problem, but __unreachable() should be better indeed.

sys/dev/sound/pcm/sound.c

845

Yes, unloads fine. Why?

[kib]

sys/dev/sound/pcm/sound.c

845

delete_unr() panics if there are allocated unit numbers.

[christos]

sys/dev/sound/pcm/sound.c

845

The channels are deallocated before getting here, so free_unr() has been called already. Am I missing something? I haven't encountered a panic.

[christos]

chn_getunr(): Use __assert_unreachable() instead of returning NULL.

[kib]

sys/dev/sound/pcm/sound.c

845

This is fine, I pointed the common issue with unr on destroy.

[christos]

sys/dev/sound/pcm/sound.c

845

Is it good to go?

[kib]

sys/dev/sound/pcm/sound.c

845

I do not see problems from the unr(9) KPI usage. OTOH I never looked at the sound(4) code to say that the change is fine or not. Since you maintain it now de-facto, you decide.

[christos]

sys/dev/sound/pcm/sound.c

845

@dev_submerge.ch Any comments? As I mention in the commit message, wanting to sort in descending order (and as a result, breaking the mechanism proposed in D46550) is highly unlikely, but I think it's good to be on the safe side.

[dev_submerge.ch]

@dev_submerge.ch Any comments? As I mention in the commit message, wanting to sort in descending order (and as a result, breaking the mechanism proposed in D46550) is highly unlikely, but I think it's good to be on the safe side.

Using existing KPI is a plus, but the *_unr() is quite complex. It introduces another mutex and additional allocations in our case. Regarding the issue discussed in D46520, this makes the runtime implications non-obvious. We'll have to test that. Also this change trades the list order requirement in for the complexity of an additional unr allocation that we must not leak.

sys/dev/sound/pcm/sound.c

834–837

Any chance we could reuse an existing mutex? Like one that we have to lock anyway to insert / remove when manipulating the channel lists?

[christos]

In D46680#1066259, @dev_submerge.ch wrote:

@dev_submerge.ch Any comments? As I mention in the commit message, wanting to sort in descending order (and as a result, breaking the mechanism proposed in D46550) is highly unlikely, but I think it's good to be on the safe side.

Using existing KPI is a plus, but the *_unr() is quite complex. It introduces another mutex and additional allocations in our case. Regarding the issue discussed in D46520, this makes the runtime implications non-obvious. We'll have to test that. Also this change trades the list order requirement in for the complexity of an additional unr allocation that we must not leak.

I am aware of this. The main rationale is to make the code more fail-proof. That being said, I am not totally against simply adding a warning comment in pcm_chn_add() and just going with the initial approach, as well as the assumption that the list will always be sorted in ascending order.

sys/dev/sound/pcm/sound.c

834–837

Since we are at module load, pcm_register() hasn't been called yet to initialize the PCM lock. So we'd need to create a new lock anyway.

[dev_submerge.ch]

In D46680#1067378, @christos wrote:

In D46680#1066259, @dev_submerge.ch wrote:

@dev_submerge.ch Any comments? As I mention in the commit message, wanting to sort in descending order (and as a result, breaking the mechanism proposed in D46550) is highly unlikely, but I think it's good to be on the safe side.

Using existing KPI is a plus, but the *_unr() is quite complex. It introduces another mutex and additional allocations in our case. Regarding the issue discussed in D46520, this makes the runtime implications non-obvious. We'll have to test that. Also this change trades the list order requirement in for the complexity of an additional unr allocation that we must not leak.

I am aware of this. The main rationale is to make the code more fail-proof. That being said, I am not totally against simply adding a warning comment in pcm_chn_add() and just going with the initial approach, as well as the assumption that the list will always be sorted in ascending order.

I don't have a preference one way or the other. In my tests, the unr(9) implementation was faster with contiguous allocation of channels, but slightly slower when garbage collecting them. They're still in the same ball park, though. Therefore I'd recommend to cap the total number of vchans at around 10000, to keep the allocation / deallocation time for a single vchan below ~1ms.