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

[Benjamin Herrenschmidt]

On Mon, 2008-04-21 at 17:06 -0300, Luke Browning wrote:
> 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.    

I fail to understand your logic here. A load and a larx are exactly
equivalent -unless- you also attempt to modify with a stwcx. There is no
ordering guarantee provided by either. A load is atomic, as a larx is
and achitecturally at least both can return "old" data.

However, that isn't necessarily an issue in this context.

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

I spent some time with Jeremy auditing that stuff (we found bugs and
fixed them). I believe the current code is safe from that happening. The
reason is that we disable all MFC interrupts after quiescing it and
before saving the csa, and we use synchronize_irq() which guarantees
that any previously received interrupt has completed execution of its
handler.

There is still a potentialy race if the previously issued interrupt
hasn't reached the kernel yet but that is covered by setting the masks
which are then tested by the interrupt handler.

Now there is one case we saw for which there was an issue, I don't know
if Jeremy has addressed it yet, and your patch wouldn't fix it neither I
believe.

The issue is that a pending fault condition is present when we quiesce.
We quiesce the MFC, set the mask, save state etc... Later on, the
context switch code re-enables interrupts for it's own use (the loading
and saving of the local store requires interrupts to service kernel hash
faults). At that point, at least in Mambo, we've seen the MFC "remember"
it had a pending fault for the previous context and re-issue that
interrupt.

We can't use your flag for that as we do need to handle class 1
interrupts for the kernel.

I think the solution that worked for us back then we to make sure we
clear the MFC's DSISR when quiescing, though it remains to be verified
that this works on HW and not only in mambo. 

> 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 

Bitops used as a synchronization facility are very error prone and in
general don't clarify things much. Besides, we do need to handle kernel
faults during context switches. I don't think your approach is buying us
anything at this stage.

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

I'm not sure I understand the exact scenario you are talking about here.

Cheers,
Ben.