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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux