[alsa-devel] [RFC][PATCH] ALSA: firewire-lib: permit process context only to flush queued packets for better PCM period granularity
Takashi Sakamoto
o-takashi at sakamocchi.jp
Wed May 11 08:05:07 CEST 2016
On May 11 2016 14:35, Takashi Iwai wrote:
> On Wed, 11 May 2016 00:56:39 +0200,
> Takashi Sakamoto wrote:
>>
>> Hi,
>>
>> On May 11 2016 00:03, Takashi Iwai wrote:
>>> On Tue, 10 May 2016 16:07:50 +0200,
>>> Takashi Sakamoto wrote:
>>>>
>>>> Hi Clemens,
>>>>
>>>> On 2016年05月07日 21:46, Takashi Sakamoto wrote:
>>>>> Could I request your comment to this patch? It is to solve a race condition,
>>>>> but the condition is quite rare. Practically, it might have a less meanings,
>>>>> except for better program.
>>>>>
>>>>> And I think there's another race condition against processing each packets
>>>>> by calling out/in_stream_callback(), but I cannot observe the race. Software
>>>>> IRQ contexts of IR/IT contexts and process contexts are under the race
>>>>> condition, however I can see no problems related to it in my several trials
>>>>> in multi-core machine. I have no idea about the reason that packet sequence
>>>>> is processed correctly between the software IRQ contexts and the process
>>>>> contexts without any lock primitives. Do you have some ideas about it?
>>>>
>>>> I wrote an additional patch for this race issue. Would you please read
>>>> this, too?
>>>
>>> It's just a flag indicating of a busy task, right?
>>> If so, it doesn't have to be a spinlock, but a simple atomic_t.
>>
>> This function is called in both of software IRQ context and process
>> context. Thus, atomic_t causes kernel hungs in software IRQ context,
>> because We cannot call kernel APIs which call process scheduler in the
>> context.
>
> I guess you are confused. atomic ops can be safely used in any
> contexts. (I suggested atomic_t in the previous mail, but actually a
> lighter primitive for such a case is cmpxchg() and its variants. In
> anyway, all are found in atomic.h.)
Yes. In this morning, I stupidly confused 'atomic_t' and 'struct mutex',
sorry. (My brain might be still half awake...)
> In your code, you never call spin_lock() but only spin_trylock().
> That is, there is no part really spinning, but it's used only as a
> flag. So, it is equivalent with doing something like:
>
> if (cmpxchg(flag, 1)) /* concurrently running? */
> return;
> do_something;
> cmpxchg(flag, 0);
>
> in both callbacks.
Yep. It's suitable in my case to use atomic_t.
Well, I realize that this second patch is meaningless for its aim. I
overlooked that 'tasklet_disable()' is called in
'ohci_flush_iso_completions()' against software IRQ of IR/IT context.
http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/tree/drivers/firewire/ohci.c#n3477
The race condition between the software irq context and the process
context (calling 'fw_iso_context_flush_completions()') is already
intervened. So the packet processing is never executed in several
contexts in a time.
Now I'm back to the first patch.
Thanks
Takashi Sakamoto
>> For supplemental information, please refer to a patch which I posted
>> just now. Unfortunately, alsa-project.org doesn't blast the message as
>> of now... You can also see the message in an archive of ffado-devel.
>> https://sourceforge.net/p/ffado/mailman/message/35078341/
>>
>>
>> Thanks
>>
>> Takashi Sakamoto
>>
>>> thanks,
>>>
>>> Takashi
>>>
>>>>
>>>>
>>>> Regards
>>>>
>>>> ----- 8< -----
>>>>
>>>> From d2090cac868e718227596dbda31ea6333b72009c Mon Sep 17 00:00:00 2001
>>>> From: Takashi Sakamoto <o-takashi at sakamocchi.jp>
>>>> Date: Tue, 10 May 2016 22:02:05 +0900
>>>> Subject: [PATCH] firewire-lib: add locking for packet processing
>>>>
>>>> When packet streaming starts, packet processing is done in software IRQ
>>>> context of 1394 OHCI IR/IT contexts. This is a typical way. On the other
>>>> hand, process context of PCM application can also process packets in a
>>>> path to handle PCM frames. This is for better PCM pointer granularity.
>>>> The two execution context causes race condition against packet processing.
>>>>
>>>> When the race occurs, it's enough that just one of these two contexts
>>>> handles packet processing, because actual time dominates packet queueing.
>>>>
>>>> This commit adds spin lock to manage the race condition. When the race
>>>> occurs, second context returns immediately from critical section. Thus,
>>>> it has little overhead.
>>>> ---
>>>> sound/firewire/amdtp-stream.c | 39 ++++++++++++++++++++++++++++++---------
>>>> sound/firewire/amdtp-stream.h | 3 +++
>>>> 2 files changed, 33 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
>>>> index 92d5a16..80d5887 100644
>>>> --- a/sound/firewire/amdtp-stream.c
>>>> +++ b/sound/firewire/amdtp-stream.c
>>>> @@ -601,6 +601,14 @@ static void out_stream_callback(struct
>>>> fw_iso_context *context, u32 tstamp,
>>>> if (s->packet_index < 0)
>>>> return;
>>>>
>>>> + /*
>>>> + * It's enough for queued packets to be handled by process context of
>>>> + * PCM application or software IRQ context of 1394 OHCI IT context in a
>>>> + * time.
>>>> + */
>>>> + if (!spin_trylock(&s->lock_packetization))
>>>> + return;
>>>> +
>>>> cycle = compute_cycle_count(tstamp);
>>>>
>>>> /* Align to actual cycle count for the last packet. */
>>>> @@ -608,14 +616,18 @@ static void out_stream_callback(struct
>>>> fw_iso_context *context, u32 tstamp,
>>>>
>>>> for (i = 0; i < packets; ++i) {
>>>> cycle = increment_cycle_count(cycle, 1);
>>>> - if (handle_out_packet(s, cycle, i) < 0) {
>>>> - s->packet_index = -1;
>>>> - amdtp_stream_pcm_abort(s);
>>>> - return;
>>>> - }
>>>> + if (handle_out_packet(s, cycle, i) < 0)
>>>> + break;
>>>> }
>>>>
>>>> - fw_iso_context_queue_flush(s->context);
>>>> + if (i == packets) {
>>>> + fw_iso_context_queue_flush(s->context);
>>>> + } else {
>>>> + s->packet_index = -1;
>>>> + amdtp_stream_pcm_abort(s);
>>>> + }
>>>> +
>>>> + spin_unlock(&s->lock_packetization);
>>>> }
>>>>
>>>> static void in_stream_callback(struct fw_iso_context *context, u32 tstamp,
>>>> @@ -631,6 +643,14 @@ static void in_stream_callback(struct
>>>> fw_iso_context *context, u32 tstamp,
>>>> if (s->packet_index < 0)
>>>> return;
>>>>
>>>> + /*
>>>> + * It's enough for queued packets to be handled by process context of
>>>> + * PCM application or software IRQ context of 1394 OHCI IR context in a
>>>> + * time.
>>>> + */
>>>> + if (!spin_trylock(&s->lock_packetization))
>>>> + return;
>>>> +
>>>> /* The number of packets in buffer */
>>>> packets = header_length / IN_PACKET_HEADER_SIZE;
>>>>
>>>> @@ -660,13 +680,14 @@ static void in_stream_callback(struct
>>>> fw_iso_context *context, u32 tstamp,
>>>> }
>>>>
>>>> /* Queueing error or detecting invalid payload. */
>>>> - if (i < packets) {
>>>> + if (i == packets) {
>>>> + fw_iso_context_queue_flush(s->context);
>>>> + } else {
>>>> s->packet_index = -1;
>>>> amdtp_stream_pcm_abort(s);
>>>> - return;
>>>> }
>>>>
>>>> - fw_iso_context_queue_flush(s->context);
>>>> + spin_unlock(&s->lock_packetization);
>>>> }
>>>>
>>>> /* this is executed one time */
>>>> diff --git a/sound/firewire/amdtp-stream.h b/sound/firewire/amdtp-stream.h
>>>> index c1bc7fa..6be5feb 100644
>>>> --- a/sound/firewire/amdtp-stream.h
>>>> +++ b/sound/firewire/amdtp-stream.h
>>>> @@ -102,6 +102,9 @@ struct amdtp_stream {
>>>> struct iso_packets_buffer buffer;
>>>> int packet_index;
>>>>
>>>> + /* Packet processing can run in one context at a time. */
>>>> + spinlock_t lock_packetization;
>>>> +
>>>> /* For CIP headers. */
>>>> unsigned int source_node_id_field;
>>>> unsigned int data_block_quadlets;
>>>> --
>>>> 2.7.4
More information about the Alsa-devel
mailing list