[Cbe-oss-dev] [PATCH 2/3] spufs context switch - make SPU_CONTEXT_SWITCH_PENDING atomic

[Jeremy Kerr]

Luke,

> The test_bit operation does not use atomic memory primitives

How so? It's a single load.

> so put this flag under the spu register_lock.  SPU exception handlers
> must be tightly integrated into spu context switching, so that they
> don't restart DMA while the MFC queue is being saved or restored.

Are you seeing a particular error case here?

>  static inline void spu_load_slb(struct spu *spu, int slbe, struct
> spu_slb *slb) @@ -346,14 +348,14 @@ spu_irq_class_1(int irq, void
> *data)
>  	if (stat & CLASS1_STORAGE_FAULT_INTR)
>  		spu_mfc_dsisr_set(spu, 0ul);
>  	spu_int_stat_clear(spu, 1, stat);
> -
> -	if (stat & CLASS1_SEGMENT_FAULT_INTR)
> -		__spu_trap_data_seg(spu, dar);
> -
>  	spin_unlock(&spu->register_lock);
> +
>  	pr_debug("%s: %lx %lx %lx %lx\n", __FUNCTION__, mask, stat,
>  			dar, dsisr);
>
> +	if (stat & CLASS1_SEGMENT_FAULT_INTR)
> +		__spu_trap_data_seg(spu, dar);
> +

This is now racy - the SLB updates need to be performed with 
register_lock held. See c92a1acb.

>  static inline void save_mfc_cntl(struct spu_state *csa, struct spu
> *spu) @@ -732,10 +736,9 @@ static inline void set_switch_active(str *
>     Change the software context switch pending flag
>  	 *     to context switch active.
>  	 *
> -	 *     This implementation does not uses a switch active flag.
> +	 * This step is performed in enable_interrupts so that the flag
> +	 * can be read atomically by interrupt handling code.
>  	 */
> -	clear_bit(SPU_CONTEXT_SWITCH_PENDING, &spu->flags);
> -	mb();
>  }

From what I can see, you're changing SPU_CONTEXT_SWITCH_PENDING to 
include the time that we used to have SPU_CONTEXT_SWITCH_ACTIVE set, 
but have since removed.

We shouldn't see any DMA restarts during this entire period, as the SLBs 
are pre-initialised for the context switch, and we do the check for 
SPU_CONTEXT_SWITCH_PENDING. Page misses for user mappings are deferred, 
so we won't be hitting the restart path there either.

Are you actually seeing DMA restarts being issued during context switch?

Cheers,


Jeremy