Re: anybody else OK with current CVS *and* SMP?

[Takashi Iwai <tiwai@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]

Hi Paul,

At Thu, 07 Aug 2003 09:22:57 -0400,
Paul Davis wrote:
> 
> >Do you get lockups if you use snd-dummy as the lowlevel driver? How about
> >without ardour (just jackd and some example clients running)?
> 
> i'll have to try snd-dummy. and yes, it will happen with just jackd
> alone. 

did you check this already?

> however, it will not happen unless jackd is run with SCHED_FIFO. my
> system has been running for several days now with jackd there the
> whole time. with SCHED_FIFO, i can't get more than about 10-15
> minutes, or so it seemed.
> 
> the worrying thing here is that the problem may go back further in
> time, because until qjackctl came along, my default was to run without
> SCHED_FIFO, whereas with it the "default" is with SCHED_FIFO. this
> means that SCHED_FIFO operation may not have been tested well enough
> for quite a while (on my system, i mean). i do know that in further
> back (months), i've run jackd+ardour with SCHED_FIFO for hours without
> problems on this system.

i became now a bit skeptical whether this is really related with the
new stream-linking codes, since this happens only with SCHED_FIFO.
in theory, the dead stream-link locks can happen at the XRUN status,
which may happen more often with SCHED_OTHERS.

anyway, the attached is a patch to clean up a bit the complicated
spin-lock paths.  this invokes always unlock -> lock instead of
spin_trylock for simplicity.  please give a try.


Takashi

[-- Attachment #2: spin-cleanup.dif --]
[-- Type: application/octet-stream, Size: 3788 bytes --]

Index: alsa-kernel/core/pcm_native.c
===================================================================
RCS file: /home/tiwai/cvs/alsa/alsa-kernel/core/pcm_native.c,v
retrieving revision 1.57
diff -u -r1.57 pcm_native.c
--- alsa-kernel/core/pcm_native.c	23 Jul 2003 10:45:21 -0000	1.57
+++ alsa-kernel/core/pcm_native.c	7 Aug 2003 21:26:19 -0000
@@ -629,8 +629,7 @@
 
 	snd_pcm_group_for_each(pos, substream) {
 		s = snd_pcm_group_substream_entry(pos);
-		if (s != substream)
-			spin_lock(&s->self_group.lock);
+		spin_lock(&s->self_group.lock);
 		res = ops->pre_action(s, state);
 		if (res < 0)
 			break;
@@ -645,16 +644,14 @@
 			} else {
 				ops->post_action(s, state);
 			}
-			if (s != substream)
-				spin_unlock(&s->self_group.lock);
+			spin_unlock(&s->self_group.lock);
 		}
 	} else {
 		snd_pcm_substream_t *s1;
 		/* unlock all streams */
 		snd_pcm_group_for_each(pos, substream) {
 			s1 = snd_pcm_group_substream_entry(pos);
-			if (s1 != substream)
-				spin_unlock(&s1->self_group.lock);
+			spin_unlock(&s1->self_group.lock);
 			if (s1 == s)	/* end */
 				break;
 		}
@@ -691,13 +688,11 @@
 	int res;
 
 	if (snd_pcm_stream_linked(substream)) {
-		if (!spin_trylock(&substream->group->lock)) {
-			spin_unlock(&substream->self_group.lock);
-			spin_lock(&substream->group->lock);
-			spin_lock(&substream->self_group.lock);
-		}
+		spin_unlock(&substream->self_group.lock);
+		spin_lock(&substream->group->lock);
 		res = snd_pcm_action_group(ops, substream, state);
 		spin_unlock(&substream->group->lock);
+		spin_lock(&substream->self_group.lock);
 	} else {
 		res = snd_pcm_action_single(ops, substream, state);
 	}
@@ -716,9 +711,7 @@
 	read_lock_irq(&snd_pcm_link_rwlock);
 	if (snd_pcm_stream_linked(substream)) {
 		spin_lock(&substream->group->lock);
-		spin_lock(&substream->self_group.lock);
 		res = snd_pcm_action_group(ops, substream, state);
-		spin_unlock(&substream->self_group.lock);
 		spin_unlock(&substream->group->lock);
 	} else {
 		spin_lock(&substream->self_group.lock);
@@ -779,10 +772,10 @@
 static int snd_pcm_pre_stop(snd_pcm_substream_t *substream, int state)
 {
 	snd_pcm_runtime_t *runtime = substream->runtime;
-	if (substream->runtime->status->state != SNDRV_PCM_STATE_RUNNING &&
-	    substream->runtime->status->state != SNDRV_PCM_STATE_DRAINING)
-		return -EBADFD;
 	runtime->trigger_master = substream;
+	/*
+	 * we don't check the status here.  run the stop operation unconditionally.
+	 */
 	return 0;
 }
 
@@ -790,6 +783,9 @@
 {
 	if (substream->runtime->trigger_master != substream)
 		return 0;
+	if (substream->runtime->status->state != SNDRV_PCM_STATE_RUNNING &&
+	    substream->runtime->status->state != SNDRV_PCM_STATE_DRAINING)
+		return 0;
 	return substream->ops->trigger(substream, SNDRV_PCM_TRIGGER_STOP);
 }
 
@@ -811,7 +807,7 @@
 };
 
 /**
- * snd_pcm_stop
+ * snd_pcm_stop; call with stream lock
  */
 int snd_pcm_stop(snd_pcm_substream_t *substream, int state)
 {
@@ -1141,20 +1137,16 @@
 	snd_pcm_substream_t *s;
 
 	if (snd_pcm_stream_linked(substream)) {
-		if (!spin_trylock(&substream->group->lock)) {
-			spin_unlock(&substream->self_group.lock);
-			spin_lock(&substream->group->lock);
-			spin_lock(&substream->self_group.lock);
-		}
+		spin_unlock(&substream->self_group.lock);
+		spin_lock(&substream->group->lock);
 		snd_pcm_group_for_each(pos, substream) {
 			s = snd_pcm_group_substream_entry(pos);
-			if (s != substream)
-				spin_lock(&s->self_group.lock);
+			spin_lock(&s->self_group.lock);
 			s->runtime->status->state = state;
-			if (s != substream)
-				spin_unlock(&s->self_group.lock);
+			spin_unlock(&s->self_group.lock);
 		}
 		spin_unlock(&substream->group->lock);
+		spin_lock(&substream->self_group.lock);
 	} else {
 		substream->runtime->status->state = state;
 	}