[Cbe-oss-dev] [PATCH] spufs: avoid using backing operation for wbox write

[André Detsch]

Arnd Bergmann wrote:
> On Friday 03 August 2007, Andre Detsch wrote:
>> Subject: spufs: avoid using backing operation for wbox write
> 
> I'm not convinced this is the right solution. It should really be
> possible to emulate this correctly and fix the bug that caused the
> problem.

I agree that may not be the optimal fix. I've really tried to provide a 
fix by changing the backing operation and the context restore functions. 
However, I could not find a way to adjust the value of the 
SPU_WrEventAck mask explicitly (the mask is read-only from the 
PPU-side). When a context is restore to a SPU, 
MFC_SPU_MAILBOX_WRITTEN_EVENT bit at SPU_WrEventAck may be 1, telling 
the SPU not to process the MB event (mechanism used to avoid 
re-processing of a same event). The bit is forced to 0 (internally in 
the SPU) only when doing an out_be32(&prob->spu_mb_W, data), the direct 
hw operation for writing to the SPU MB.

Given that limitation of interaction with SPU_WrEventAck, the two ways I 
see to deal with mb write operations on unscheduled ctxs are:
- Block/EAGAIN, like done with my patch; or
- Bufferize the operation, and perform an out_be32(&prob->spu_mb_W, 
data) after the context is restored.

Both are somewhat intrusive.

> I believe you introduce two new bugs in this patch by fixing one:
> 
> * Writing to the mailbox can now block/fail even if the status tells
>   us that it can't. This can break existing applications.

The patch may interfere on applications that do non-blocking MB write 
operations _and_ assume that the MB being full is the only reason why a 
write would return 0. This last assumption is not stated in the libspe 
documentation, but is something to discuss. Maybe the documentation, 
implicitly, gives the impression that this assumption is valid. Or, 
maybe, I've just failed to find the explicit reference to this assumption :)

> * I don't see how the mb write operation gets resumed when the context
>   is restored. Instead of the spu side blocking indefinitely, you now
>   have the ppu side blocking indefinitely.

That's why I had to add
+	wake_up_all(&ctx->wbox_wq);
to spufs/sched.c. First, I was expecting to have to change spufs/file.c, 
but the wait conditions there established did not had to change with my 
patch.


Basic operation after the patch seems to be ok. I'm telling this based 
on some tests I've did with up to 100 ctxs - a lot of operations done on 
unscheduled ctxs - using both blocking and non-blocking operations. 
Before the patch, all spu_backing_wbox_write operations have always failed.

--
Andre Detsch