[Cbe-oss-dev] [PATCH] fix state_mutex leaks
Christoph Hellwig
hch at lst.de
Wed Feb 6 17:42:03 EST 2008
On Tue, Jan 29, 2008 at 01:59:19PM -0200, Luke Browning wrote:
>
> On Thu, 2008-01-17 at 14:51 +0100, Christoph Hellwig wrote:
> > Fix various state_mutex leaks. The worst one was introduced by the
> > interrutible state_mutex conversion but there've been a few before
> > aswell. Notably spufs_wait now returns without the state_mutex held
> > when returning an error, which actually cleans up some code aswell.
> >
> >
> > Signed-off-by: Christoph Hellwig <hch at lst.de>
> >
>
> I like this approach a lot better. You mispelled bookkeeping
> (bookkepping) in a couple of places and you have a bug below.
Thanks, that bug is quite nasty. Looks like none of out testsuite writes
to the mfc file.
As the original patch hasn't made it to powerpc.git or mainline yet an
updated version is below:
Index: powerpc/arch/powerpc/platforms/cell/spufs/fault.c
===================================================================
--- powerpc.orig/arch/powerpc/platforms/cell/spufs/fault.c 2008-02-06 07:21:50.000000000 +0100
+++ powerpc/arch/powerpc/platforms/cell/spufs/fault.c 2008-02-06 07:26:09.000000000 +0100
@@ -108,7 +108,7 @@ int spufs_handle_class1(struct spu_conte
u64 ea, dsisr, access;
unsigned long flags;
unsigned flt = 0;
- int ret, ret2;
+ int ret;
/*
* dar and dsisr get passed from the registers
@@ -148,13 +148,11 @@ int spufs_handle_class1(struct spu_conte
ret = spu_handle_mm_fault(current->mm, ea, dsisr, &flt);
/*
- * If spu_acquire fails due to a pending signal we just want to return
- * EINTR to userspace even if that means missing the dma restart or
- * updating the page fault statistics.
+ * This is nasty: we need the state_mutex for all the
+ * bookkeeping even if the syscall was interrupted by
+ * a signal. ewww.
*/
- ret2 = spu_acquire(ctx);
- if (ret2)
- goto out;
+ mutex_lock(&ctx->state_mutex);
/*
* Clear dsisr under ctxt lock after handling the fault, so that
@@ -185,7 +183,6 @@ int spufs_handle_class1(struct spu_conte
} else
spufs_handle_event(ctx, ea, SPE_EVENT_SPE_DATA_STORAGE);
- out:
spuctx_switch_state(ctx, SPU_UTIL_SYSTEM);
return ret;
}
Index: powerpc/arch/powerpc/platforms/cell/spufs/file.c
===================================================================
--- powerpc.orig/arch/powerpc/platforms/cell/spufs/file.c 2008-02-06 07:21:50.000000000 +0100
+++ powerpc/arch/powerpc/platforms/cell/spufs/file.c 2008-02-06 07:25:29.000000000 +0100
@@ -357,6 +357,7 @@ static unsigned long spufs_ps_nopfn(stru
{
struct spu_context *ctx = vma->vm_file->private_data;
unsigned long area, offset = address - vma->vm_start;
+ int ret = 0;
offset += vma->vm_pgoff << PAGE_SHIFT;
if (offset >= ps_size)
@@ -375,14 +376,15 @@ static unsigned long spufs_ps_nopfn(stru
if (ctx->state == SPU_STATE_SAVED) {
up_read(¤t->mm->mmap_sem);
- spufs_wait(ctx->run_wq, ctx->state == SPU_STATE_RUNNABLE);
+ ret = spufs_wait(ctx->run_wq, ctx->state == SPU_STATE_RUNNABLE);
down_read(¤t->mm->mmap_sem);
} else {
area = ctx->spu->problem_phys + ps_offs;
vm_insert_pfn(vma, address, (area + offset) >> PAGE_SHIFT);
}
- spu_release(ctx);
+ if (!ret)
+ spu_release(ctx);
return NOPFN_REFAULT;
}
@@ -749,23 +751,25 @@ static ssize_t spufs_ibox_read(struct fi
count = spu_acquire(ctx);
if (count)
- return count;
+ goto out;
/* wait only for the first element */
count = 0;
if (file->f_flags & O_NONBLOCK) {
- if (!spu_ibox_read(ctx, &ibox_data))
+ if (!spu_ibox_read(ctx, &ibox_data)) {
count = -EAGAIN;
+ goto out_unlock;
+ }
} else {
count = spufs_wait(ctx->ibox_wq, spu_ibox_read(ctx, &ibox_data));
+ if (count)
+ goto out;
}
- if (count)
- goto out;
/* if we can't write at all, return -EFAULT */
count = __put_user(ibox_data, udata);
if (count)
- goto out;
+ goto out_unlock;
for (count = 4, udata++; (count + 4) <= len; count += 4, udata++) {
int ret;
@@ -782,9 +786,9 @@ static ssize_t spufs_ibox_read(struct fi
break;
}
-out:
+out_unlock:
spu_release(ctx);
-
+out:
return count;
}
@@ -899,7 +903,7 @@ static ssize_t spufs_wbox_write(struct f
count = spu_acquire(ctx);
if (count)
- return count;
+ goto out;
/*
* make sure we can at least write one element, by waiting
@@ -907,14 +911,16 @@ static ssize_t spufs_wbox_write(struct f
*/
count = 0;
if (file->f_flags & O_NONBLOCK) {
- if (!spu_wbox_write(ctx, wbox_data))
+ if (!spu_wbox_write(ctx, wbox_data)) {
count = -EAGAIN;
+ goto out_unlock;
+ }
} else {
count = spufs_wait(ctx->wbox_wq, spu_wbox_write(ctx, wbox_data));
+ if (count)
+ goto out;
}
- if (count)
- goto out;
/* write as much as possible */
for (count = 4, udata++; (count + 4) <= len; count += 4, udata++) {
@@ -928,8 +934,9 @@ static ssize_t spufs_wbox_write(struct f
break;
}
-out:
+out_unlock:
spu_release(ctx);
+out:
return count;
}
@@ -1592,12 +1599,11 @@ static ssize_t spufs_mfc_read(struct fil
} else {
ret = spufs_wait(ctx->mfc_wq,
spufs_read_mfc_tagstatus(ctx, &status));
+ if (ret)
+ goto out;
}
spu_release(ctx);
- if (ret)
- goto out;
-
ret = 4;
if (copy_to_user(buffer, &status, 4))
ret = -EFAULT;
@@ -1726,6 +1732,8 @@ static ssize_t spufs_mfc_write(struct fi
int status;
ret = spufs_wait(ctx->mfc_wq,
spu_send_mfc_command(ctx, cmd, &status));
+ if (ret)
+ goto out;
if (status)
ret = status;
}
@@ -1779,7 +1787,7 @@ static int spufs_mfc_flush(struct file *
ret = spu_acquire(ctx);
if (ret)
- return ret;
+ goto out;
#if 0
/* this currently hangs */
ret = spufs_wait(ctx->mfc_wq,
@@ -1788,12 +1796,13 @@ static int spufs_mfc_flush(struct file *
goto out;
ret = spufs_wait(ctx->mfc_wq,
ctx->ops->read_mfc_tagstatus(ctx) == ctx->tagwait);
-out:
+ if (ret)
+ goto out;
#else
ret = 0;
#endif
spu_release(ctx);
-
+out:
return ret;
}
Index: powerpc/arch/powerpc/platforms/cell/spufs/run.c
===================================================================
--- powerpc.orig/arch/powerpc/platforms/cell/spufs/run.c 2008-02-06 07:21:50.000000000 +0100
+++ powerpc/arch/powerpc/platforms/cell/spufs/run.c 2008-02-06 07:26:23.000000000 +0100
@@ -354,8 +354,15 @@ long spufs_run_spu(struct spu_context *c
do {
ret = spufs_wait(ctx->stop_wq, spu_stopped(ctx, &status));
- if (unlikely(ret))
+ if (unlikely(ret)) {
+ /*
+ * This is nasty: we need the state_mutex for all the
+ * bookkeeping even if the syscall was interrupted by
+ * a signal. ewww.
+ */
+ mutex_lock(&ctx->state_mutex);
break;
+ }
spu = ctx->spu;
if (unlikely(test_and_clear_bit(SPU_SCHED_NOTIFY_ACTIVE,
&ctx->sched_flags))) {
Index: powerpc/arch/powerpc/platforms/cell/spufs/spufs.h
===================================================================
--- powerpc.orig/arch/powerpc/platforms/cell/spufs/spufs.h 2008-02-06 07:21:50.000000000 +0100
+++ powerpc/arch/powerpc/platforms/cell/spufs/spufs.h 2008-02-06 07:23:53.000000000 +0100
@@ -268,6 +268,9 @@ extern char *isolated_loader;
* Same as wait_event_interruptible(), except that here
* we need to call spu_release(ctx) before sleeping, and
* then spu_acquire(ctx) when awoken.
+ *
+ * Returns with state_mutex re-acquired when successfull or
+ * with -ERESTARTSYS and the state_mutex dropped when interrupted.
*/
#define spufs_wait(wq, condition) \
@@ -278,11 +281,11 @@ extern char *isolated_loader;
prepare_to_wait(&(wq), &__wait, TASK_INTERRUPTIBLE); \
if (condition) \
break; \
+ spu_release(ctx); \
if (signal_pending(current)) { \
__ret = -ERESTARTSYS; \
break; \
} \
- spu_release(ctx); \
schedule(); \
__ret = spu_acquire(ctx); \
if (__ret) \
More information about the cbe-oss-dev
mailing list