[PATCH alsa-lib 2/2] pcm: dmix: make lockless operation optional

[Jaroslav Kysela]

Dne 22. 06. 20 v 11:31 Mark Hills napsal(a):
> On Fri, 19 Jun 2020, Takashi Iwai wrote:
> 
>> The recent overlooked bug about the unconditional semaphore usage in
>> the dmix implies that basically we've had no problem with the locking
>> in the practical usages over years.  Although the lockless operation
>> has a clear merit, it's a much higher CPU usage (especially on some
>> uncached pages), and it might lead to a potential deadlock in theory
>> (which is hard to reproduce at will, though).
>>
>> This patch introduces a new configure option "--enable-lockless-dmix"
>> or "--disable-lockless-dmix" to let user choose the default dmix
>> operation mode, then the default value for the dmix
>> "direct_memory_access" option is set based on it.
>>
>> A big question is which operation mode should be default: it's hard to
>> decide, but in this patch, I bet for disabling the lockless in the
>> end as the performance loss is significant.
>>
>> But the user can enable the lockless operation at any time; at build
>> time, with the configure option above, and at run time, by specifying
>> the dmix "direct_memory_access" option, too.
> 
> I would like to express some caution here.
> 
> Seems it must be essential (not just choice) that the semaphore
> implementation is the default. As per Maarten's information, there are
> libasound embedded in applications and containers. Differing defaults
> results in broken audio between applications.
> 
> Sadly there is, in effect, an ABI here; a practical risk that users
> suffering the consequences eventually.
> 
> Because of the history of sepahores, an application would need to signal
> its intent to use atomics, which is not a good thing as that is complex.
> 
> Instead I think it is smart here to consider the opportunity which Maarten
> has come here with.
> 
> Patches here are just the beginning to bring alive a lot of dormant
> functionality. It assumes hand-written assembly code will run concurrently
> that appears to not yet have been tested in that way. It is a joy to see
> hand written assembly, but my worry is that is influencing the decision
> making.
> 
> I am only recently looking at dmix/snoop code, so I do not aim to stand in
> the way. But I think it would be prudent to consider that bringing alive
> dormant functionality (vs. opportunity to remove code) as if it were
> adding the code explicitly. Would ALSA developers review and accept a
> 1000+ line patch adding architecture-specific assembly, changes to the
> ABI, based on the benchmarks which Maarten has presented?
> 
> Where I am more certain is: if options are to be provided to users then it
> should be because a user is in the best position to decide. In this case I
> think ALSA developers must equip users in understanding the pros/cons.
> That's why ideally there's no option and code just does the right thing.
> If not, at very least documentation must explain the tradeoff (and I think
> a better name should be chosen.)
> 
> I can certainly see interesting positives for mixing based on atomics. But
> there are many years without it, and this feels hasty and there are risks.
> 
> And Maarten has presented and benchmarked an excellent opportunity to
> simplify, which could be missed. It is one thing to leave the code dormant
> until a decision or clearer picture. But these patches risk meeting that
> opportunity and transforming it into complexity for developers and users.

Some comments: This code was designed when we worked with SMP machines with 2 
CPUs (not cores - physical CPUs). It's old, but I feel it's worth to keep it 
at least for the reference and testing. The one instruction racy time window 
described by Maarten seems true, but as I wrote, the errors were mostly zero 
(no hearable).

The locked variant (without atomic instructions) should be the default without 
any questions. It's more effective for the multi-cpu-core architectures used 
nowadays.

I would propose this:

1) use the locked variant as default (no atomic code even for x86)
2) add a configuration option to select the mixing code
3) change SND_PCM_DIRECT_MAGIC which will follow the mixing code selection
    to avoid locking / atomic mixing code mismatch / misuse

					Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.