Paths

Table of Contentst

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

Authored by christos on Sep 15 2024, 6:39 PM.

Details

Reviewers

markj
emaste
dev_submerge.ch
kib

Commits

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

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

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 59500
Build 56387: arc lint + arc unit

Event Timeline

christos created this revision.Sep 15 2024, 6:39 PM

Herald added a subscriber: imp. · View Herald TranscriptSep 15 2024, 6:39 PM

christos requested review of this revision.Sep 15 2024, 6:39 PM

Harbormaster completed remote builds in B59498: Diff 143366.Sep 15 2024, 6:39 PM

christos mentioned this in D46550: sound: Make sure chn_init() produces unique unit numbers.Sep 15 2024, 6:39 PM

kib added inline comments.Sep 15 2024, 6:46 PM

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 added inline comments.Sep 15 2024, 7:04 PM

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 added inline comments.Sep 15 2024, 8:07 PM

sys/dev/sound/pcm/sound.c
845	delete_unr() panics if there are allocated unit numbers.

christos added inline comments.Sep 15 2024, 9:44 PM

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.

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

Harbormaster completed remote builds in B59500: Diff 143372.Sep 15 2024, 9:52 PM

kib added inline comments.Sep 16 2024, 7:02 AM

sys/dev/sound/pcm/sound.c
845	This is fine, I pointed the common issue with unr on destroy.

christos marked 3 inline comments as done.Sep 16 2024, 8:47 AM

christos added inline comments.

sys/dev/sound/pcm/sound.c
845	Is it good to go?

kib added inline comments.Sep 16 2024, 10:43 AM

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 marked an inline comment as done.Sep 16 2024, 10:47 AM

christos added inline comments.

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 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?

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.

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.

This revision is now accepted and ready to land.Sep 28 2024, 4:37 PM

Make unr(9) per-softc. I don't know how I missed this one...

This revision now requires review to proceed.Oct 13 2024, 9:17 PM

Harbormaster completed remote builds in B59961: Diff 144787.Oct 13 2024, 9:17 PM

christos marked 2 inline comments as done.Oct 13 2024, 9:23 PM

christos added inline comments.