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

[Luke Browning]

On Thu, 2008-04-17 at 17:41 +1000, Jeremy Kerr wrote:
> Luke,
> 
> > The test_bit operation does not use atomic memory primitives
> 
> How so? It's a single load.

load is not enough in this case as the code needs to be MP safe.  Load
does not include the cache line reservation that occurs with the larx
instruction.  You could get a stale cache value.    

> 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?

Since the existing code is not atomic, mfc queue processing could be
restarted in the middle of a save sequence.  Not only would this cause
the wrong data to be saved, but it would put the hardware into a
different state from what was expected.    

> >  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.

This is easy to fix.  I will submit another patch.

> 
> >  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?

I added protection to the restore part of the algorithm for two reasons:

1) I think it makes the code cleaner and easier to debug.  This is
primarily a style issue but having the same protection apply to a field
regardless of where it is used in the algorithm makes it easier to
understand.  Eliminating this protection in the restore part of the
algorithm is an optimization.  I think there is an assumption in the
documented sequence that a stray dma woill not 

2) a dma restart could occur in the restore sequence if the exception
was delayed long enough - probably around 80 microseconds.  The
exception would have to be related to pre-saved state.  This would
provide enough time for the context to be saved and unlocked, so that
the scheduler could find it and try to re-schedule it.

Luke