[Cbe-oss-dev] [PATCH] spusched: extend list_mutex coverage

[Christoph Hellwig]

spu->ctx is changed under the list_mutex and state_mutex of the loaded
context.  To guarantee that a context doesn't go away or get unloaded
we need to make sure to lock the state mutex without previously
unlocking the list_mutex when traversing the spu lists in find_victim
and spusched_tick.

That means we have to switch to trylock operations on state_mutex in these
two functions and opencode spu_schedule in spusched_tick.  Both is
rather ugly, but I can't find a good way around it.


Signed-off-by: Christoph Hellwig <hch at lst.de>

Index: linux-2.6/arch/powerpc/platforms/cell/spufs/sched.c
===================================================================
--- linux-2.6.orig/arch/powerpc/platforms/cell/spufs/sched.c	2008-07-23 10:18:04.000000000 +0200
+++ linux-2.6/arch/powerpc/platforms/cell/spufs/sched.c	2008-07-23 10:45:22.000000000 +0200
@@ -633,7 +633,6 @@ static struct spu *find_victim(struct sp
 			    (!victim || tmp->prio > victim->prio))
 				victim = spu->ctx;
 		}
-		mutex_unlock(&cbe_spu_info[node].list_mutex);
 
 		if (victim) {
 			/*
@@ -647,28 +646,15 @@ static struct spu *find_victim(struct sp
 			 * look at another context or give up after X retries.
 			 */
 			if (!mutex_trylock(&victim->state_mutex)) {
-				victim = NULL;
-				goto restart;
-			}
-
-			spu = victim->spu;
-			if (!spu || victim->prio <= ctx->prio) {
-				/*
-				 * This race can happen because we've dropped
-				 * the active list mutex.  Not a problem, just
-				 * restart the search.
-				 */
-				mutex_unlock(&victim->state_mutex);
+				mutex_unlock(&cbe_spu_info[node].list_mutex);
 				victim = NULL;
 				goto restart;
 			}
 
 			spu_context_trace(__spu_deactivate__unload, ctx, spu);
 
-			mutex_lock(&cbe_spu_info[node].list_mutex);
 			cbe_spu_info[node].nr_active--;
 			spu_unbind_context(spu, victim);
-			mutex_unlock(&cbe_spu_info[node].list_mutex);
 
 			victim->stats.invol_ctx_switch++;
 			spu->stats.invol_ctx_switch++;
@@ -676,9 +662,10 @@ static struct spu *find_victim(struct sp
 				spu_add_to_rq(victim);
 
 			mutex_unlock(&victim->state_mutex);
-
+			mutex_unlock(&cbe_spu_info[node].list_mutex);
 			return spu;
 		}
+		mutex_unlock(&cbe_spu_info[node].list_mutex);
 	}
 
 	return NULL;
@@ -715,16 +702,21 @@ static void spu_schedule(struct spu *spu
 	spu_release(ctx);
 }
 
-static void spu_unschedule(struct spu *spu, struct spu_context *ctx)
+static void __spu_unschedule(struct spu *spu, struct spu_context *ctx)
 {
-	int node = spu->node;
-
-	mutex_lock(&cbe_spu_info[node].list_mutex);
-	cbe_spu_info[node].nr_active--;
+	cbe_spu_info[spu->node].nr_active--;
 	spu->alloc_state = SPU_FREE;
 	spu_unbind_context(spu, ctx);
 	ctx->stats.invol_ctx_switch++;
 	spu->stats.invol_ctx_switch++;
+}
+
+static void spu_unschedule(struct spu *spu, struct spu_context *ctx)
+{
+	int node = spu->node;
+
+	mutex_lock(&cbe_spu_info[node].list_mutex);
+	__spu_unschedule(spu, ctx);
 	mutex_unlock(&cbe_spu_info[node].list_mutex);
 }
 
@@ -875,8 +867,11 @@ static noinline void spusched_tick(struc
 	struct spu_context *new = NULL;
 	struct spu *spu = NULL;
 
-	if (spu_acquire(ctx))
-		BUG();	/* a kernel thread never has signals pending */
+	/*
+	 * If we can't lock the context wait for the next scheduler tick.
+	 */
+	if (!mutex_trylock(&ctx->state_mutex))
+		return;
 
 	if (ctx->state != SPU_STATE_RUNNABLE)
 		goto out;
@@ -892,9 +887,16 @@ static noinline void spusched_tick(struc
 
 	spu_context_trace(spusched_tick__preempt, ctx, spu);
 
-	new = grab_runnable_context(ctx->prio + 1, spu->node);
+	/*
+	 * Try to find a new context on the runqueue that we can lock
+	 * without sleeping.
+	 */
+	do {
+		new = grab_runnable_context(ctx->prio + 1, spu->node);
+	} while (new && !mutex_trylock(&new->state_mutex));
+
 	if (new) {
-		spu_unschedule(spu, ctx);
+		__spu_unschedule(spu, ctx);
 		if (test_bit(SPU_SCHED_SPU_RUN, &ctx->sched_flags))
 			spu_add_to_rq(ctx);
 	} else {
@@ -905,8 +907,24 @@ static noinline void spusched_tick(struc
 out:
 	spu_release(ctx);
 
-	if (new)
-		spu_schedule(spu, new);
+	/*
+	 * This is an opencoded spu_schedule accounting for the fact
+	 * that we have the list_lock already taken.
+	 *
+	 * Not pretty.
+	 */
+	if (new) {
+		int node = spu->node;
+
+		BUG_ON(spu->ctx);
+
+		spu_set_timeslice(new);
+		spu_bind_context(spu, new);
+		cbe_spu_info[node].nr_active++;
+		spu->alloc_state = SPU_USED;
+		wake_up_all(&new->run_wq);
+		spu_release(new);
+	}
 }
 
 /**
@@ -973,11 +991,8 @@ static int spusched_thread(void *unused)
 					cbe_list) {
 				struct spu_context *ctx = spu->ctx;
 
-				if (ctx) {
-					mutex_unlock(mtx);
+				if (ctx)
 					spusched_tick(ctx);
-					mutex_lock(mtx);
-				}
 			}
 			mutex_unlock(mtx);
 		}