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

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

[Takashi Sakamoto]

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;
-- 
2.7.4

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

[Takashi Sakamoto]

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

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

[Takashi Iwai]

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

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

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

[Takashi Iwai]

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

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

[Takashi Sakamoto]

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@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

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

[Clemens Ladisch]

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

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

[Takashi Sakamoto]

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