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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
