portYIELD_WITHIN_API usage logic bug

Kernel
1

deckard026354 wrote on Wednesday, July 04, 2012:

Greetings.

Looking at the logic for yield_FROM_WITHIN_API and the ARM7 code that goes with it in two places

xTaskResumeAll();

There is a case where we call portYIELD_WITHIN_API() line 1236.

On the SAM7 port this will call the naked function vPortYieldProcessor() which will setup the next return instruction to do a context-swtich

void vPortYieldProcessor( void )
{
        /* Within an IRQ ISR the link register has an offset from the true return
        address, but an SWI ISR does not.  Add the offset manually so the same
        ISR return code can be used in both cases. */
        asm volatile ( “ADD             LR, LR, #4” );

        /* Perform the context switch.  First save the context of the current task. */
        portSAVE_CONTEXT();

        /* Find the highest priority task that is ready to run. */
        vTaskSwitchContext();

        /* Restore the context of the new task. */
        portRESTORE_CONTEXT();
}

We call into taskEXIT_CRITICAL() - portEXIT_CRITICAL() - which shouldn’t corrupt any of the contents of the registers populated by portRESTORE_CONTEXT();

However the return xAlreadyYielded will overwrite a register ?

On x86 return xAlreadyYielded would result in %eax being overwritten - on the specific example called out for ARM7 I believe that chunk of code “return xAlreadyYielded” after the call to the naked function will result in %r0 from the portRESTORE_CONTEXT() being overwritten.

This will be the case for any CPU architecture that defines particular registers to hold procedure return values.

In simple terms the problem is this can happen:

int yield_test(void)
{
portYIELD_WITHIN_API();
return 0x12345678;
}

2

deckard026354 wrote on Wednesday, July 04, 2012:

Just to illustrate

signed portBASE_TYPE xTaskResumeAll( void )

{

register tskTCB *pxTCB;

signed portBASE_TYPE xAlreadyYielded = pdFALSE;

/* If uxSchedulerSuspended is zero then this function does not match a

previous call to vTaskSuspendAll(). */

configASSERT( uxSchedulerSuspended );

/* It is possible that an ISR caused a task to be removed from an event

list while the scheduler was suspended.  If this was the case then the

removed task will have been added to the xPendingReadyList.  Once the

scheduler has been resumed it is safe to move all the pending ready

tasks from this list into their appropriate ready list. */

taskENTER_CRITICAL();

{

-uxSchedulerSuspended;

if( uxSchedulerSuspended == ( unsigned portBASE_TYPE ) pdFALSE )

{

if( uxCurrentNumberOfTasks > ( unsigned portBASE_TYPE ) 0 )

{

portBASE_TYPE xYieldRequired = pdFALSE;

/* Move any readied tasks from the pending list into the

appropriate ready list. */

while( listLIST_IS_EMPTY( ( xList * ) &xPendingReadyList ) == pdFALSE )

{

pxTCB = ( tskTCB * ) listGET_OWNER_OF_HEAD_ENTRY(  ( ( xList * ) &xPendingReadyList ) );

vListRemove( &( pxTCB->xEventListItem ) );

vListRemove( &( pxTCB->xGenericListItem ) );

prvAddTaskToReadyQueue( pxTCB );

/* If we have moved a task that has a priority higher than

the current task then we should yield. */

if( pxTCB->uxPriority >= pxCurrentTCB->uxPriority )

{

xYieldRequired = pdTRUE;

}

}

/* If any ticks occurred while the scheduler was suspended then

they should be processed now.  This ensures the tick count does not

slip, and that any delayed tasks are resumed at the correct time. */

if( uxMissedTicks > ( unsigned portBASE_TYPE ) 0 )

{

while( uxMissedTicks > ( unsigned portBASE_TYPE ) 0 )

{

vTaskIncrementTick();

-uxMissedTicks;

}

/* As we have processed some ticks it is appropriate to yield

to ensure the highest priority task that is ready to run is

the task actually running. */

#if configUSE_PREEMPTION == 1

{

xYieldRequired = pdTRUE;

}

#endif

}

if( ( xYieldRequired == pdTRUE ) || ( xMissedYield == pdTRUE ) )

{

xAlreadyYielded = pdTRUE;

xMissedYield = pdFALSE;

portYIELD_WITHIN_API();

}

}

}

}

taskEXIT_CRITICAL();

return xAlreadyYielded;

}

3

rtel wrote on Thursday, July 05, 2012:

There is a case where we call portYIELD_WITHIN_API() line 1236.

If you are going to quote line numbers, you need to also quote the file name and the file version.

On the SAM7 port this will call the naked function vPortYieldProcessor() which will setup the next return instruction to do a context-swtich

On the SAM7, portYIELD_WITHIN_API() just calls portYield().  You haven’t said which compiler you are using, but I think in all the ports portYIELD() does not call a naked function, and does not set up a next return instruction.  portYIELD() does call the SWI instruction, which causes the ARM7 to switch modes, and stacks, and in effect start executing a software interrupt (or exception).  The handler for that exception is called vPortYieldProcessor().

We call into taskEXIT_CRITICAL() - portEXIT_CRITICAL() - which shouldn’t corrupt any of the contents of the registers populated by portRESTORE_CONTEXT();

I’m not sure what you mean here.  The exception handler does not call these macros.

However the return xAlreadyYielded will overwrite a register ?

On x86 return xAlreadyYielded would result in %eax being overwritten - on the specific example called out for ARM7 I believe that chunk of code “return xAlreadyYielded” after the call to the naked function will result in %r0 from the portRESTORE_CONTEXT() being overwritten.

This will be the case for any CPU architecture that defines particular registers to hold procedure return values.

In simple terms the problem is this can happen:

int yield_test(void)
{
portYIELD_WITHIN_API();
return 0x12345678;
}

As much as I want to answer your question, I’m really not following this at all, and your illustration does not help.

In any case, are you aware that the SWI handler has its own R0 register, so it not using the same R0 as the task?  Are you familiar with the ARM7 architecture?

Regards.

4

deckard026354 wrote on Thursday, July 05, 2012:

Hi Richard.

I’m using the ARM example to illustrate the point.

In xTaskResumeAll @ portYIELD_WITHIN_API() - we’re inside a critical section. So a software interrupt will jump to the interrupt handler - but hold on… we’re inside a critical section - interrupt should be off at this point.

The SWI interrupt handler will do a portSAVE_CONTEXT() vTaskSwitchContext() portRESTORE_CONTEXT()

The restore will setup the stack frame for a context switch on the return from interrupt.

Trouble is we were previously inside of a critical section…

Is this really the logic you are after ?

You’d jump into another thread with interrupts switched off surely ?

5

rtel wrote on Thursday, July 05, 2012:

In xTaskResumeAll @ portYIELD_WITHIN_API() - we’re inside a critical section. So a software interrupt will jump to the interrupt handler - but hold on… we’re inside a critical section - interrupt should be off at this point.

SWI is synchronous and is not masked, so will execute inside a critical section.  Each task maintains its own critical section nesting status, and the interrupt status is returned to the correct value when the task is restored.  It does not mean interrupt remain disabled until the task runs again.  This is ok, the scheduler does this in a controlled way, but it is definitely not something that application writers should do.

Is this really the logic you are after ?

Quite definitely yes.  It ensures correct and consistent behaviour across context switches, especially when the task being switched out is next switched in.

You’d jump into another thread with interrupts switched off surely ?

6

rtel wrote on Thursday, July 05, 2012:

In xTaskResumeAll @ portYIELD_WITHIN_API() - we’re inside a critical section. So a software interrupt will jump to the interrupt handler - but hold on… we’re inside a critical section - interrupt should be off at this point.

SWI is synchronous and is not masked, so will execute inside a critical section.  Each task maintains its own critical section nesting status, and the interrupt status is returned to the correct value when the task is restored.  It does not mean interrupt remain disabled until the task runs again.  This is ok, the scheduler does this in a controlled way, but it is definitely not something that application writers should do.

Is this really the logic you are after ?

Quite definitely yes.  It ensures correct and consistent behaviour across context switches, especially when the task being switched out is next switched in.

You’d jump into another thread with interrupts switched off surely ?

Not so.  If that were the case nothing would run.  Look at the implementation of the restore context code and you will see that it checks the nesting value for the critical section which is saved per task not globally, to ensure interrupts return to their correct state.

Regards.

7

deckard026354 wrote on Thursday, July 05, 2012:

Hi Richard.

I had an “ah ha” moment EnterCritical != SwitchOffInterrupt

This is why it’s OK for the vPortYieldProcessor to switch on interrupts on exit - it requires the interrupt state to be saved to TCB and restored so that when we context switch back - (inside of the critical section) interrupts are off (as expected).

Thanks !