OS_UpCall 6

I don’t like the way that OS_UpCall 6 doesn’t sleep until the pollword changes. It means that anything that wants to sleep has to implement its own sleep logic (or worst-case, just sit in a loop and spin).

Can anyone see any problem with implementing a new SWI, which implements suitable sleep logic? E.g.

_kernel_os_error *Sleep(volatile int *pollword)
{
  while (!*pollword) {
    // Give OS_UpCall 6 a chance
    _swix(OS_UpCall,_INR(0,1),6,pollword);
    if (!*pollword)
    {
      // Upcall didn't work, go to sleep
      __asm { wfe; } // Replace with suitable Portable_Idle call if WFE not supported
    }
  }
  return NULL;
}

It would also be trivial to implement a version which has a timeout.

AIUI this code should work equally well for code which is waiting for an event (WFE is used for the sleep) and code which is waiting for an interrupt (assuming the interrupt isn’t masked in the PSR, WFE will wake in order to service the interrupt, and then the exception return after the interrupt will set the event register and the WFE will exit. If an IRQ fires inbetween checking the pollword and executing the WFE, the event register will still be set, so there’s no danger of needlessly sleeping)

Possibly we could also modify Portable_Idle to use WFE instead of WFI, but that would require it to enable IRQs (+ FIQs?) internally so that the WFE will actually wake the processor (the current recommendation is to call Portable_Idle with IRQs disabled, so that the check of your pollword/event flag is atomic with the decision to go to sleep).

First up – which OS_UpCall handler are we talking about here? Like all UpCalls, UpCall 6 is made from deeply-nested OS code “up” to the runtime environment. In the case of UpCall 6, the element of runtime environment which will have installed a handler is part of the scheduling system. I’m aware of TaskWindow and RTSupport both supplying UpCall 6 handlers for threads that they manage; I wouldn’t be surprised if UnixLib’s pthread library does likewise, but I’m not intimately familiar with that. If a future RISC OS kernel implemented pre-emptive multitasking, then it would provide the UpCall 6 handler itself.

I don’t think the function you wrote actually saves the caller from having to implement a loop at all, because there’s still a race condition between the end of your while loop and whatever the caller want to do with the pollword (most often, that would be to zero it to indicate that a mutex was now locked).

TaskWindow implements UpCall 6 by calling Wimp_Poll, and Wimp_Poll already has logic to call Portable_Idle (and there should probably be a WFE call either there or in the Wimp or perhaps both (as in, the Wimp does WFE if Portable_Idle fails: you ideally only want one WFE per iteration of the loop or you could end up being more sluggish to wake up than you need to be).

I can see there being an argument for there being a better default UpCall 6 handler for programs that aren’t running under TaskWindow, RTSupport or whatever, now that WFE exists.

First up – which OS_UpCall handler are we talking about here?

Just OS_UpCall 6 in general. The fact that it’s not guaranteed that the handlers will sleep until the pollword changes means that general-purpose code which can’t assume a specific handler is present will have to implement its own sleep loop.

I don’t think the function you wrote actually saves the caller from having to implement a loop at all, because there’s still a race condition between the end of your while loop and whatever the caller want to do with the pollword (most often, that would be to zero it to indicate that a mutex was now locked).

Yes, they’d still need a loop, but they wouldn’t have to worry about implementing sleep logic in that loop. Currently the sleep logic would have to be architecture-specific, which would be messy. It would be tidier if Portable_Idle could be used (i.e. if it was changed to use WFE), but I still think the best option would be to have all the sleep logic in one place. That way we can easily tweak and change it in future as required.

Maybe I’m just bitter because I’ve had a bad experience with RTSupport – I wrote some code which was calling RT_Yield to wait for a pollword to change, but it was doing so with interrupts disabled. In some situations this code was running while all the other threads were blocked, so RT_Yield was returning straight back to me, still with interrupts disabled, so the IRQ which I was waiting for was never going to arrive and the system was deadlocked. Easily avoidable once you know how RT_Yield operates internally, but the fact that I have to implement extra code to deal with that kind of situation seems wrong to me, and it’s the kind of mistake that other people could easily make when writing their own code.

I can see there being an argument for there being a better default UpCall 6 handler for programs that aren’t running under TaskWindow, RTSupport or whatever, now that WFE exists.

RTSupport’s UpCall handler always claims UpCall 6, even if it couldn’t find another runnable thread and is returning with the pollword still zero. Considering that most machines now have RTSupport in ROM, we’d probably want to fix RTSupport as well (maybe have it pass on the call if it failed to switch to another thread? or if the pollword is still clear? I’m not quite sure which would be best)

I’d worry that trying to second-guess whether the UpCall 6 handler did a WFE is a bit fragile. In this example, your thread could be pre-empted sometime between the Wimp/RTSupport checking the pollword, and you doing so within your if statement: during that pre-emption, some other thread could have zeroed the pollword again, so you would incorrectly deduce that you had to do WFE yourself, potentially wasting a complete timeslice.

I assumed from your initial paragraph that you were concerned with UpCall 6 in cases where no handler was installed (or just a no-op handler). In this case, the UpCall has always returned immediately irrespective of the pollword value, but since the caller was always mandated to repeatedly call UpCall 6 anyway, there wouldn’t be any functional change to doing WFE in the kernel’s default handler, apart from energy saving.

Now I gather you were actually thinking of RTSupport, I do have a vague recollection of it only bothering to do one pass of all the registered threads if they’re all blocked, so I can seewhat you’re saying. Yes, perhaps RTSupport could do WFE if all threads are blocked, though that could mean a little bit of extra overhead in the thread-checking loop. But that wouldn’t fix the problem you described – if the caller of RT_Yield has IRQs disabled and all other threads are blocked, then inserting WFE (anywhere) won’t solve the deadlock, only briefly re-enabling interrupts will.

I’d worry that trying to second-guess whether the UpCall 6 handler did a WFE is a bit fragile. In this example, your thread could be pre-empted sometime between the Wimp/RTSupport checking the pollword, and you doing so within your if statement: during that pre-emption, some other thread could have zeroed the pollword again, so you would incorrectly deduce that you had to do WFE yourself, potentially wasting a complete timeslice.

True – if the UpCall handler is doing WFE then the proposed logic may result in more sleeping than desired.

Now I gather you were actually thinking of RTSupport, I do have a vague recollection of it only bothering to do one pass of all the registered threads if they’re all blocked, so I can seewhat you’re saying.

Yes, I think I was focusing too much on RTSupport. I assumed that because RTSupport implemented its UpCall handler as a “yield” operation that that was the correct thing to do, hence the suggestion of a new SWI. But now it’s clear to me that the UpCall handlers are expected to implement “sleep” functionality (with the caveat that – from the caller’s perspective – they may wake up randomly for no discernible reason)

So with that in mind, how about this for a solution. I think it’s basically what you’re suggesting, but it’s useful to spell it all out:

• The spec for OS_UpCall 6 will be revised to make it clear that the intention of the call is to only return once the pollword is set (or an error occurs). Returning for other reasons is discouraged (in my eye that’s a sign of a poor implementation), but still permissible (mainly because programs need to be compatible with the poor implementations that already exist). Handlers are required to enter a low-power state if there isn’t any work to do (e.g. call Portable_Idle in a loop)
• Change Portable_Idle to use WFE where possible (and fix it to issue a DSB prior to the WFE/WFI, as recommended by ARM). This would require it to enable IRQs+FIQs internally. But callers of Portable_Idle can’t assume WFE is in use, so will still need to implement the recommended sequence to avoid redundant sleeps. Considering that only ARMv6+ has WFE I don’t think there’ll be any compatibility issues with changing Portable_Idle to enable IRQs+FIQs internally.
• Add a default UpCall handler to the kernel which calls Portable_idle
• Fix RTSupport’s handler so that it calls Portable_Idle if it fails to find a thread to switch to
• RTSupport’s RT_Yield implementation will remain as-is – doing nothing if it fails to find a thread.

Although, I am worried that we’ll eventually run into deadlocks caused by the fact that there are potentially multiple thread managers active (e.g. RTSupport & TaskWindow on a typical system). Not sure offhand what the “best” solution would be for that.

Unfortunately I don’t think it will be possible to change Portable_Idle to use WFE. Returning from an exception (i.e. SWI) causes the event register to be set, so if you call Portable_Idle in a loop it’ll never sleep because the return from one SWI call will always mean the event register is set when you make the next call. So we’re basically limited to using WFE in situations where we can loop without calling any SWIs, e.g. in an UpCall 6 handler.

Also after doing a bit of testing it looks like Cortex-A8 treats WFE as NOP, so at some point we might want to teach SyncLib (and any other places) about some alternatives like WFI/Portable_Idle.