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

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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?


Regards

----- 8< -----

From d2090cac868e718227596dbda31ea6333b72009c Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@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@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel