[alsa-devel] [RFC][PATCH] ALSA: firewire-lib: permit process context only to flush queued packets for better PCM period granularity

[Takashi Sakamoto]

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.

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
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel at alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>