sound: Introduce global driver lock

[christos]

Summary

Introduce a new global driver lock to use it in place of Giant. Also
lock around uses around uses of devclass_get_maxunit(), replace leftover
CTLFLAG_NEEDGIANTs with CTLFLAG_MPSAFE.

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

[markj]

Anything which is calling devclass_get_maxunit() should be holding bus_topo_lock, AKA, Giant. This change is incomplete.

[imp]

In D46700#1064689, @markj wrote:

Anything which is calling devclass_get_maxunit() should be holding bus_topo_lock, AKA, Giant. This change is incomplete.

Quibble: Giant is a detail now that consumers need not know. So removing NEEDSGIANT is good, so long as one explicitly uses bus_topo_lock() around references to devclass_get_maxunit the whole loop in sysctl_hw_snd_maxautovchans.

I should add asserts to devclass_get_maxunit()... It's not strictly required for it, but to use the results, one must hold the topo lock.

[christos]

Extend previous patch.

[markj]

sys/dev/sound/pcm/channel.c

64

bus_topo lock is for the newbus subsystem, i.e., bits of the kernel which control device driver attach, detach, etc..

Its use here doesn't make sense. A store to an integer value is already atomic.

110

Ditto.

164

Ditto.

184

Ditto.

[christos]

sys/dev/sound/pcm/channel.c

64

Is the change from CTLFLAG_NEEDGIANT to CTLFLAG_MPSAFE for sysctls that also only modify integers okay?

[markj]

sys/dev/sound/pcm/channel.c

64

"Maybe."

What is the purpose of the lock? Are you worried about multiple threads concurrently writing a value? Then there's no real problem - all of the stores will be atomic, and the last thread to write the value will win.

But, what about the code which reads this value? Is it reading the value multiple times in a function? If so, does it matter if the value changes in between reads? If so, then you need a lock to synchronize readers and writers.

A concrete example: feeder_register() loads the value chn_latency_profile multiple times. That code is wrong if chn_latency_profile changes in between loads. How to fix it? Either:

• use a lock to serialize readers and writers,
• tell the compiler that the value is to be loaded only once:

int val = atomic_load_int(&chn_latency_profile);
if (val < CHN_LATENCY_PROFILE_MIN || val > CHN_LATENCY_PROFILE_MAX)
    atomic_store_int(&chn_latency_profile, CHN_LATENCY_PROFILE_DEFAULT);

It is usually better to use a mutex. Code which uses mutexes is usually easier to read and understand.

Now, that said, I think this particular example is a bit bogus: the sysctl_hw_snd_latency_profile() handler returns EINVAL if the value is outside of those bounds, and I don't see any other way to change the value. So I think that statement can just be replaced with an assertion that the latency profile is between max and min. But, the general idea is there.

[christos]

Lock only around uses of devclass_get_maxunit(). Will write a separate patches
to 1) add assertions around reads of global variables, 2) remove
initializations from feeder.c.

[christos]

Get rid of locking inside sysctl_hw_snd_default_unit().

[markj]

sys/dev/sound/pcm/feeder_rate.c

282

Extra newline here.

sys/dev/sound/pcm/sound.c

185

This is a bit dodgy. You're setting two variables here, and the readers are not locked at all, as far as I can see. This sysctl handler can race if it runs at the same time as pcm_setstatus(), which is called from various driver attach routines, which are usually invoked with Giant held.

Rather than relying on Giant, the sound subsystem should perhaps have some global driver init lock that can serialize sysctl updates. However, driver attach is a rare event, so these races are very unlikely to be problematic; it's a low-priority problem.

[christos]

sys/dev/sound/pcm/sound.c

185

At EuroBSDCon we were discussing with @imp whether it's better to get rid of bus_topo_* calls in general in sound(4), and instead use a global driver lock which we'll create on sound(4) load. Is this what you're referring to?

[markj]

sys/dev/sound/pcm/sound.c

185

Something like that, yeah. In general you want/need *something* to synchronize setting global variables (usually via sysctl) and consumers (drivers at initialization time). In the past, that was provided by Giant. Today, it's buggy, but the races are hard to hit so no one notices, but that makes the code brittle and hard to reason about.

[christos]

Use new global driver lock instead of bus_topo_*.

However, sound(4) still relies on the PCM_GIANT_* macros. Should those be
replaced with the new lock as well?

[christos]

@markj any opinion (see previous comment)?

[christos]

Retire PCM_GIANT_ENTER() and PCM_GIANT_LEAVE/PCM_GIANT_EXIT() and replace with
2 two PCM_SND_LOCK() and PCM_SND_UNLOCK() macros (I'm open to propositions for
a better name).

[christos]

sys/dev/sound/pcm/sound.h

353 ↗

(On Diff #144990)

@markj Is it better to initialize the lock as MTX_RECURSE or just check that mtx_owned(snd_mtx) == 0 here?

[christos]

Use sx(9) instead of mtx(9) so that we can sleep with the lock held.

[christos]

Use a shared lock instead. Fixes some bugs I missed (didn't realize I wasn't
using a non-MPSAFE driver when testing...).

[christos]

After discussion with markj@, keep PCM_GIANT_* and just use the new
SND_LOCK for uses of devclass_get_maxunit() and where needed, global variable
setting in sysctl handlers.

[christos]

Bump.