From: AMEET M. PARANJAPE <aparanja@redhat.com>
Date: Fri, 24 Oct 2008 09:26:20 -0500
Subject: [ppc64] spufs: missing context switch notification log-2
Message-id: 4901DB0C.9010607@REDHAT.COM
O-Subject: Re: [PATCH 1/1 bz462622 ] spufs - missing context switch notification log (additional patch)
Bugzilla: 462622
RH-Acked-by: David Howells <dhowells@redhat.com>

This is the second patch for this issue based on the comments from
RKML.  The patch has been tested and verified to work and it was built
on all platforms in brew at
http://brewweb.devel.redhat.com/brew/taskinfo?taskID=1538485.

Upstream Status:
================

commit f5ed0eb6fe131e8f3847323b4aa569a6f7b36f56
Author: Jeremy Kerr <jk@ozlabs.org>
Date:   Thu Oct 16 10:03:46 2008 +1100

    powerpc/spufs: Use state_mutex for switch_log locking, and prevent multiple openers

commit 14f693eeb5b16bc47ffa38d8b8838a654aedd53f
Author: Jeremy Kerr <jk@ozlabs.org>
Date:   Thu Oct 16 10:51:46 2008 +1100

    powerpc/spufs: Don't require full buffer in switch_log read

commit 837ef884b702edd1c4514eaed1dbecd48721bd22
Author: Jeremy Kerr <jk@ozlabs.org>
Date:   Fri Oct 17 12:02:31 2008 +1100

    powerpc/spufs: Use kmalloc rather than kzalloc for switch log buffer

Thanks,

--
Ameet M. Paranjape
aparanja@redhat.com
IBM/Red Hat POWER Liason
IRC name: aparanja

diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c
index edfd084..9bde808 100644
--- a/arch/powerpc/platforms/cell/spufs/file.c
+++ b/arch/powerpc/platforms/cell/spufs/file.c
@@ -2395,38 +2395,49 @@ static inline int spufs_switch_log_avail(struct spu_context *ctx)
 static int spufs_switch_log_open(struct inode *inode, struct file *file)
 {
 	struct spu_context *ctx = SPUFS_I(inode)->i_ctx;
+	int rc;
+
+	rc = spu_acquire(ctx);
+	if (rc)
+		return rc;
 
-	/*
-	 * We (ab-)use the mapping_lock here because it serves the similar
-	 * purpose for synchronizing open/close elsewhere.  Maybe it should
-	 * be renamed eventually.
-	 */
-	mutex_lock(&ctx->mapping_lock);
 	if (ctx->switch_log) {
-		spin_lock(&ctx->switch_log->lock);
-		ctx->switch_log->head = 0;
-		ctx->switch_log->tail = 0;
-		spin_unlock(&ctx->switch_log->lock);
-	} else {
-		/*
-		 * We allocate the switch log data structures on first open.
-		 * They will never be free because we assume a context will
-		 * be traced until it goes away.
-		 */
-		ctx->switch_log = kzalloc(sizeof(struct switch_log) +
-			SWITCH_LOG_BUFSIZE * sizeof(struct switch_log_entry),
-			GFP_KERNEL);
-		if (!ctx->switch_log)
-			goto out;
-		spin_lock_init(&ctx->switch_log->lock);
-		init_waitqueue_head(&ctx->switch_log->wait);
+		rc = -EBUSY;
+		goto out;
 	}
-	mutex_unlock(&ctx->mapping_lock);
+
+	ctx->switch_log = kmalloc(sizeof(struct switch_log) +
+		SWITCH_LOG_BUFSIZE * sizeof(struct switch_log_entry),
+		GFP_KERNEL);
+
+	if (!ctx->switch_log) {
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	ctx->switch_log->head = ctx->switch_log->tail = 0;
+	init_waitqueue_head(&ctx->switch_log->wait);
+	rc = 0;
+
+out:
+	spu_release(ctx);
+	return rc;
+}
+
+static int spufs_switch_log_release(struct inode *inode, struct file *file)
+{
+	struct spu_context *ctx = SPUFS_I(inode)->i_ctx;
+	int rc;
+
+	rc = spu_acquire(ctx);
+	if (rc)
+		return rc;
+
+	kfree(ctx->switch_log);
+	ctx->switch_log = NULL;
+	spu_release(ctx);
 
 	return 0;
- out:
-	mutex_unlock(&ctx->mapping_lock);
-	return -ENOMEM;
 }
 
 static int switch_log_sprint(struct spu_context *ctx, char *tbuf, int n)
@@ -2454,42 +2465,54 @@ static ssize_t spufs_switch_log_read(struct file *file, char __user *buf,
 	if (!buf || len < 0)
 		return -EINVAL;
 
+	error = spu_acquire(ctx);
+	if (error)
+		return error;
+
 	while (cnt < len) {
 		char tbuf[128];
 		int width;
 
-		if (file->f_flags & O_NONBLOCK) {
-			if (spufs_switch_log_used(ctx) <= 0)
-				return cnt ? cnt : -EAGAIN;
-		} else {
-			/* Wait for data in buffer */
-			error = wait_event_interruptible(ctx->switch_log->wait,
-					spufs_switch_log_used(ctx) > 0);
-			if (error)
+		if (spufs_switch_log_used(ctx) == 0) {
+			if (cnt > 0) {
+				/* If there's data ready to go, we can
+				 * just return straight away */
 				break;
-		}
 
-		spin_lock(&ctx->switch_log->lock);
-		if (ctx->switch_log->head == ctx->switch_log->tail) {
-			/* multiple readers race? */
-			spin_unlock(&ctx->switch_log->lock);
-			continue;
+			} else if (file->f_flags & O_NONBLOCK) {
+				error = -EAGAIN;
+				break;
+
+			} else {
+				/* spufs_wait will drop the mutex and
+				 * re-acquire, but since we're in read(), the
+				 * file cannot be _released (and so
+				 * ctx->switch_log is stable).
+				 */
+				error = spufs_wait(ctx->switch_log->wait,
+						spufs_switch_log_used(ctx) > 0);
+
+				/* On error, spufs_wait returns without the
+				 * state mutex held */
+				if (error)
+					return error;
+
+				/* We may have had entries read from underneath
+				 * us while we dropped the mutex in spufs_wait,
+				 * so re-check */
+				if (spufs_switch_log_used(ctx) == 0)
+					continue;
+			}
 		}
 
 		width = switch_log_sprint(ctx, tbuf, sizeof(tbuf));
-		if (width < len) {
+		if (width < len)
 			ctx->switch_log->tail =
 				(ctx->switch_log->tail + 1) %
 				 SWITCH_LOG_BUFSIZE;
-		}
-
-		spin_unlock(&ctx->switch_log->lock);
-
-		/*
-		 * If the record is greater than space available return
-		 * partial buffer (so far)
-		 */
-		if (width >= len)
+		else
+			/* If the record is greater than space available return
+			 * partial buffer (so far) */
 			break;
 
 		error = copy_to_user(buf + cnt, tbuf, width);
@@ -2498,6 +2521,8 @@ static ssize_t spufs_switch_log_read(struct file *file, char __user *buf,
 		cnt += width;
 	}
 
+	spu_release(ctx);
+
 	return cnt == 0 ? error : cnt;
 }
 
@@ -2506,29 +2531,41 @@ static unsigned int spufs_switch_log_poll(struct file *file, poll_table *wait)
 	struct inode *inode = file->f_dentry->d_inode;
 	struct spu_context *ctx = SPUFS_I(inode)->i_ctx;
 	unsigned int mask = 0;
+	int rc;
 
 	poll_wait(file, &ctx->switch_log->wait, wait);
 
+	rc = spu_acquire(ctx);
+	if (rc)
+		return rc;
+
 	if (spufs_switch_log_used(ctx) > 0)
 		mask |= POLLIN;
 
+	spu_release(ctx);
+
 	return mask;
 }
 
 static const struct file_operations spufs_switch_log_fops = {
-	.owner	= THIS_MODULE,
-	.open	= spufs_switch_log_open,
-	.read	= spufs_switch_log_read,
-	.poll	= spufs_switch_log_poll,
+	.owner		= THIS_MODULE,
+	.open		= spufs_switch_log_open,
+	.read		= spufs_switch_log_read,
+	.poll		= spufs_switch_log_poll,
+	.release	= spufs_switch_log_release,
 };
 
+/**
+ * Log a context switch event to a switch log reader.
+ *
+ * Must be called with ctx->state_mutex held.
+ */
 void spu_switch_log_notify(struct spu *spu, struct spu_context *ctx,
 		u32 type, u32 val)
 {
 	if (!ctx->switch_log)
 		return;
 
-	spin_lock(&ctx->switch_log->lock);
 	if (spufs_switch_log_avail(ctx) > 1) {
 		struct switch_log_entry *p;
 
@@ -2542,7 +2579,6 @@ void spu_switch_log_notify(struct spu *spu, struct spu_context *ctx,
 		ctx->switch_log->head =
 			(ctx->switch_log->head + 1) % SWITCH_LOG_BUFSIZE;
 	}
-	spin_unlock(&ctx->switch_log->lock);
 
 	wake_up(&ctx->switch_log->wait);
 }
diff --git a/arch/powerpc/platforms/cell/spufs/run.c b/arch/powerpc/platforms/cell/spufs/run.c
index 87a0e95..395da1a 100644
--- a/arch/powerpc/platforms/cell/spufs/run.c
+++ b/arch/powerpc/platforms/cell/spufs/run.c
@@ -245,6 +245,7 @@ static int spu_run_fini(struct spu_context *ctx, u32 *npc,
 
 	spuctx_switch_state(ctx, SPU_UTIL_IDLE_LOADED);
 	clear_bit(SPU_SCHED_SPU_RUN, &ctx->sched_flags);
+	spu_switch_log_notify(NULL, ctx, SWITCH_LOG_EXIT, *status);
 	spu_release(ctx);
 
 	if (signal_pending(current))
@@ -413,8 +414,6 @@ long spufs_run_spu(struct spu_context *ctx, u32 *npc, u32 *event)
 	ret = spu_run_fini(ctx, npc, &status);
 	spu_yield(ctx);
 
-	spu_switch_log_notify(NULL, ctx, SWITCH_LOG_EXIT, status);
-
 	if ((status & SPU_STATUS_STOPPED_BY_STOP) &&
 	    (((status >> SPU_STOP_STATUS_SHIFT) & 0x3f00) == 0x2100))
 		ctx->stats.libassist++;
diff --git a/arch/powerpc/platforms/cell/spufs/spufs.h b/arch/powerpc/platforms/cell/spufs/spufs.h
index ee6932d..14a61ec 100644
--- a/arch/powerpc/platforms/cell/spufs/spufs.h
+++ b/arch/powerpc/platforms/cell/spufs/spufs.h
@@ -58,7 +58,6 @@ enum {
 };
 
 struct switch_log {
-	spinlock_t		lock;
 	wait_queue_head_t	wait;
 	unsigned long		head;
 	unsigned long		tail;