[Cbe-oss-dev] [PATCH 5:9] spufs: Fix synchronization of class 1 exception state

[Jeremy Kerr]

Hi Luke,

> @@ -159,11 +159,12 @@ int spufs_handle_class1(struct spu_conte
>  	mutex_lock(&ctx->state_mutex);
>
>  	/*
> -	 * Clear dsisr under ctxt lock after handling the fault, so that
> -	 * time slicing will not preempt the context while the page fault
> -	 * handler is running. Context switch code removes mappings.
> +	 * Clear the exception fields in the csa before restarting the dma. The
> +	 * interrupt handler sets these fields without a lock, so we must clear
> +	 * them at a point where we know a new exception cannot be generated. */
>  	ctx->csa.class_1_dar = ctx->csa.class_1_dsisr = 0;
> +	smp_mb();

The smp_mb() here shouldn't be required, as the DMA restart requires a
out_be64, which performs a sync anyway..

While looking at spufs_handle_class1 - should we be clearing the dar
and dsisr *before* we drop the state_mutex? Could it be possible that
the ctx has since been loaded (and run, and another class1 generated),
and we're now clearing the status for the new exception?

> @@ -181,9 +182,7 @@ int spufs_handle_class1(struct spu_conte
>  			else
>  				ctx->spu->stats.min_flt++;
>  		}
> -
> -		if (ctx->spu)
> -			ctx->ops->restart_dma(ctx);
> +		ctx->ops->restart_dma(ctx);
>  	} else
>  		spufs_handle_event(ctx, ea, SPE_EVENT_SPE_DATA_STORAGE);

Looks good, but this should probably be in a separate change.

> Index: linux-2.6.25/arch/powerpc/platforms/cell/spufs/run.c
> ===================================================================
> --- linux-2.6.25.orig/arch/powerpc/platforms/cell/spufs/run.c
> +++ linux-2.6.25/arch/powerpc/platforms/cell/spufs/run.c
> @@ -47,7 +47,7 @@ void spufs_stop_callback(struct spu *spu
>  		 * Ensure that the exception status has hit memory before a
>  		 * thread waiting on the context's stop queue is woken
>  		 */
> -		smp_wmb();
> +		smp_mb();
>
>  		wake_up_all(&ctx->stop_wq);
>  	}

Which reads are you trying to enforce order on here?

In fact, I don't think we even need the smp_wmb(), as we do that
explicitly in try_to_wake_up.

Cheers,


Jeremy