[alsa-devel] [RFC][PATCH] ALSA: firewire-lib: permit process context only to flush queued packets for better PCM period granularity
Hi Clemens,
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?
Regards
Takashi Sakamoto
----- 8< -----
Subject: ALSA: firewire-lib: permit process context only to flush queued packets for better PCM period granularity
These three commits were merged to improve PCM pointer granularity. commit 76fb87894828 ("ALSA: firewire-lib: taskletize the snd_pcm_period_elapsed() call") commit e9148dddc3c7 ("ALSA: firewire-lib: flush completed packets when reading PCM position") commit 92b862c7d685 ("ALSA: firewire-lib: optimize packet flushing")
The point of them is to handle queued packets not only in software IRQ context of IR/IT contexts, but also in process context. This idea introduced a cyclic call of 'struct snd_pcm_ops.pointer()'. The last commit adds 'pointer_flush' member to 'struct amdtp_stream' to avoid the situation. On the other hand, This solution is weak at race condition between the process context and the software IRQ context of IR/IT contexts.
Practically, this race is not so critical because it influences process context to skip flushing queued packets and to get worse granularity of PCM pointer. This is quite rare because 'struct snd_pcm_ops.pointer()' is frequently called by process contexts while IT/IR contexts run less frequently. However, it's better to solve the race.
The similar solution can be achieved by 'in_interrupt()' macro. This commit applies the macro to solve the race condition against 'pointer_flush'.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/firewire/amdtp-stream.c | 23 +++++++++++++++++------ sound/firewire/amdtp-stream.h | 1 - 2 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c index 4484242..132c7f4 100644 --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -248,7 +248,6 @@ void amdtp_stream_pcm_prepare(struct amdtp_stream *s) tasklet_kill(&s->period_tasklet); s->pcm_buffer_pointer = 0; s->pcm_period_pointer = 0; - s->pointer_flush = true; } EXPORT_SYMBOL(amdtp_stream_pcm_prepare);
@@ -353,7 +352,6 @@ static void update_pcm_pointers(struct amdtp_stream *s, s->pcm_period_pointer += frames; if (s->pcm_period_pointer >= pcm->runtime->period_size) { s->pcm_period_pointer -= pcm->runtime->period_size; - s->pointer_flush = false; tasklet_hi_schedule(&s->period_tasklet); } } @@ -798,11 +796,24 @@ EXPORT_SYMBOL(amdtp_stream_start); */ unsigned long amdtp_stream_pcm_pointer(struct amdtp_stream *s) { - /* this optimization is allowed to be racy */ - if (s->pointer_flush && amdtp_stream_running(s)) + /* + * This function is called in software IRQ context of period_tasklet or + * process context. + * + * When the software IRQ context was scheduled by software IRQ context + * of IR/IT contexts, queued packets were already handled. Therefore, + * no need to flush the queue in buffer anymore. + * + * When the process context reach here, some packets wll be queued in + * the buffer. These packets should be handled immediately to keep + * better granularity of PCM pointer. + * + * Later, the process context will sometimes schedules software IRQ + * context of the period_tasklet. Then, no need to flush the queue by + * the same reason as described for IR/IT contexts. + */ + if (!in_interrupt() && amdtp_stream_running(s)) fw_iso_context_flush_completions(s->context); - else - s->pointer_flush = true;
return ACCESS_ONCE(s->pcm_buffer_pointer); } diff --git a/sound/firewire/amdtp-stream.h b/sound/firewire/amdtp-stream.h index 8775704..ab697cc 100644 --- a/sound/firewire/amdtp-stream.h +++ b/sound/firewire/amdtp-stream.h @@ -132,7 +132,6 @@ struct amdtp_stream { struct tasklet_struct period_tasklet; unsigned int pcm_buffer_pointer; unsigned int pcm_period_pointer; - bool pointer_flush;
/* To wait for first packet. */ bool callbacked;
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;
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.
thanks,
Takashi
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
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@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
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.)
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.
Takashi
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@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
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/fir...
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@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
Sorry for the delay.
Takashi Sakamoto wrote:
These three commits were merged to improve PCM pointer granularity. commit 76fb87894828 ("ALSA: firewire-lib: taskletize the snd_pcm_period_elapsed() call") commit e9148dddc3c7 ("ALSA: firewire-lib: flush completed packets when reading PCM position") commit 92b862c7d685 ("ALSA: firewire-lib: optimize packet flushing")
The point of them is to handle queued packets not only in software IRQ context of IR/IT contexts, but also in process context. This idea introduced a cyclic call of 'struct snd_pcm_ops.pointer()'.
There is no recursion because of the tasklet. The only problem is that the tasklet could be scheduled repeatedly because new packets continue to arrive. But even when this happens, it's harmless.
The last commit adds 'pointer_flush' member to 'struct amdtp_stream' to avoid the situation. On the other hand, This solution is weak at race condition between the process context and the software IRQ context of IR/IT contexts.
Practically, this race is not so critical because it influences process context to skip flushing queued packets and to get worse granularity of PCM pointer.
When a race causes pointer_flush to be read as true although it should be false, there is simply a superfluous call to flush_completions.
When pointer_flush is read as false although it should be true, then the buffer_pointer value _might_ not be the most current one (due to the missing flush), but this is unlikely to happen because the buffer_pointer was just updated by update_pcm_pointers(). In any case, the just- scheduled tasklet will inform the application about the new pointer.
The similar solution can be achieved by 'in_interrupt()' macro. This commit applies the macro to solve the race condition against 'pointer_flush'.
unsigned long amdtp_stream_pcm_pointer(struct amdtp_stream *s) {
- /* this optimization is allowed to be racy */
- if (s->pointer_flush && amdtp_stream_running(s))
...
- if (!in_interrupt() && amdtp_stream_running(s)) fw_iso_context_flush_completions(s->context);
else
s->pointer_flush = true;return ACCESS_ONCE(s->pcm_buffer_pointer);
}
Looks good.
Acked-by: Clemens Ladisch clemens@ladisch.de
And I think there's another race condition against processing each packets by calling out/in_stream_callback(), but I cannot observe the race.
As you already found out, fw_iso_context_flush_completions() is thread safe.
Regards, Clemens
Hi Clemens,
On May 11 2016 16:31, Clemens Ladisch wrote:
Sorry for the delay.
Takashi Sakamoto wrote:
These three commits were merged to improve PCM pointer granularity. commit 76fb87894828 ("ALSA: firewire-lib: taskletize the snd_pcm_period_elapsed() call") commit e9148dddc3c7 ("ALSA: firewire-lib: flush completed packets when reading PCM position") commit 92b862c7d685 ("ALSA: firewire-lib: optimize packet flushing")
The point of them is to handle queued packets not only in software IRQ context of IR/IT contexts, but also in process context. This idea introduced a cyclic call of 'struct snd_pcm_ops.pointer()'.
There is no recursion because of the tasklet. The only problem is that the tasklet could be scheduled repeatedly because new packets continue to arrive. But even when this happens, it's harmless.
The last commit adds 'pointer_flush' member to 'struct amdtp_stream' to avoid the situation. On the other hand, This solution is weak at race condition between the process context and the software IRQ context of IR/IT contexts.
Practically, this race is not so critical because it influences process context to skip flushing queued packets and to get worse granularity of PCM pointer.
When a race causes pointer_flush to be read as true although it should be false, there is simply a superfluous call to flush_completions.
When pointer_flush is read as false although it should be true, then the buffer_pointer value _might_ not be the most current one (due to the missing flush), but this is unlikely to happen because the buffer_pointer was just updated by update_pcm_pointers(). In any case, the just- scheduled tasklet will inform the application about the new pointer.
The similar solution can be achieved by 'in_interrupt()' macro. This commit applies the macro to solve the race condition against 'pointer_flush'.
unsigned long amdtp_stream_pcm_pointer(struct amdtp_stream *s) {
- /* this optimization is allowed to be racy */
- if (s->pointer_flush && amdtp_stream_running(s))
...
- if (!in_interrupt() && amdtp_stream_running(s)) fw_iso_context_flush_completions(s->context);
else
s->pointer_flush = true;return ACCESS_ONCE(s->pcm_buffer_pointer);
}
Looks good.
Acked-by: Clemens Ladisch clemens@ladisch.de
OK. Thanks for reviewing. I've posted a patch with updated comments and the tag.
And I think there's another race condition against processing each packets by calling out/in_stream_callback(), but I cannot observe the race.
As you already found out, fw_iso_context_flush_completions() is thread safe.
Regards
Takashi Sakamoto
participants (3)
-
Clemens Ladisch -
Takashi Iwai -
Takashi Sakamoto