Is safe to call functions that each request a context switch in a row?

Hi,

I’m writing a module for conditional variables/monitor and I noticed that two calls and an explicit yield all request a context switch:

void CWait( SemaphoreHandle_t& mutex )
   {
      taskENTER_CRITICAL();
      {
         TaskHandle_t self = xTaskGetCurrentTaskHandle();
         vTaskSuspend( self );     //// here ...
         xSemaphoreGive( mutex );  //// here ...

         configASSERT( this->len < this->max );
         this->queue[ this->tail++ ] = self;
         if( this->tail == this->max ) this->tail = 0;
         ++this->len;

      } taskEXIT_CRITICAL();
      
      taskYIELD();                 //// and here

      xSemaphoreTake( mutex, portMAX_DELAY );
   }

Although interrupts are disabled because of the critical section, I wonder if the way that vTaskSuspend() context switch request interferes or nullifies the context switch of xSemaphoreGive() when exiting the CS. Even more, in such case, does the call to taskYield make any sense since a context switch request was already made?

Thank you :smiley: !!!

Doing a vTaskSuspend(self) inside a critical section sounds like a bad idea. If you suspend yourself, you will automatically stop running this task, and on some ports, that can’t happen inside the critical section, on others it WILL happen and you will get a ‘break’ in the critical section.

Hi,

I’ll need to reorder the instructions so that the call to vTaskSuspend() doesn’t happen inside the CS. Thanks for the tip!

Say I already did it, is it safe to call (out of CS):

vTaskSuspend( self );     
xSemaphoreGive( mutex );  
taskYIELD();

in that theoretical order?

How do you manage that the code after self-suspending the task is executed ?
taskYIELD would only make sense if there is no blocking call/wait for something afterwards AND there are other tasks of equal priority ready to run (with configUSE_PREEMPTION enabled).
IMO task suspend/resume is limited to special use cases, but is sometimes misused for task synchronization. That’s not a good idea.
Which behavior do want to implement with CWait ?

BTW Instead of vTaskSuspend( self ); you can simply use vTaskSuspend( NULL );

1 Like

vTaskSuspend(self) is a timing nightmare, and almost always is a mistake. First, as Hartmut said, this task won’t continue pass then so won’t give the mutex until someone else does a resume.

The problem with someone else doing the resume is that if they do it before you do the suspend, it will get lost, so you almost always have a race condition.

Hi,

I want to implement a monitor/conditional variables, as explained here, here, and here (the two preview pages).

I have the idea on how to implement them, however it is hard to do it using the FreeRTOS API. I found this open source RTOS written in C that actually implements such functions. For example, the CWait() operation is code as:

void cond_wait(cond_t *cond, mutex_t *mutex)
{
    unsigned irqstate = irq_disable();
    thread_t *me = thread_get_active();

    mutex_unlock(mutex);                       
    sched_set_status(me, STATUS_COND_BLOCKED);
    thread_add_to_list(&cond->queue, me);      
    irq_restore(irqstate);
    thread_yield_higher();

    /*
     * Once we reach this point, the condition variable was signalled,
     * and we are free to continue.
     */
    mutex_lock(mutex);
}

(condt_t is nothing but a circular buffer of threads. Each cond_var holds such buffer.)

Right now I’m discussing with the RIOT admin guys whether that code performs as expected (here).

The CWait() op needs to suspend the caller (self); however, outside the RTOS, e.g. using the API, the only way I found right now is through suspension. I think I’ll need to use the indexed notifications (note 1) so that they block on an event that will be post by the CNotify() op. The problem with this solution is that ALL tasks in the system will include the notifications array, wasting memory.

(1) Indexed notifs DO NOT WORK in the ATmega323 port, but happily I’m doing this development with a Cortex-M3 cpu.

I need to get the task’s handler (self) in order to enqueue it as well.

I’m sure there should exist a reason for suspend()/resume() ops are included in the kernel.

There’s kind of control through mutexes and CSs, but you’ve scared me. As I said, I’ll explore other implementations that do not use suspend/resume ops.

Well Xavier, honestly, I’ve been through all of that when I was in grad school at IU (almost 30 years ago, I’m afraid), but even back then, I wasn’t able to find a use case for monitors or CCRs that gain you more or more reliable inter-thread synchronization than mutual exclusion and signals.

Do you happen to have a real-life problem that truly benefits from being solved using one of those compound sync operations instead of what we got (ie mutex, signals and RW locks)? Don’t get me wrong, I love to be convinced, but I’m still waiting for something to convince me… :wink:

Also, like Hartmut, I still wonder what the purpose of your call to taskYield() in your earlier code is. It doesn’t really do anything but possibly prematurely terminate the calling task’s time slice.

Could you also present the other side (the cond_signal()) of your implementation?

Edit: Believe it or not (I almost forgot), but I once wrote up an article on MSDN entitled “Compound Win32 Synchronization objects” in which I presented an implementation of (among other things) conditional critical sections built on Win32 sync primitives. It seems to have gotten lost, I’ll see if I can crank up a version somewhere from my old MSDN CDs.

Hi,

In raw programming nothing stops a programmer to release a mutex before it’s aquired, or a task that isn’t part of the subsystem to take a mutex. In those scenarios a monitor that encapsulates the mutex and the resource, and exposes a limited op’s will be safer than a raw mutex.

No, I don’t want to convince you. I just asked whether a set of instructions might cause problems. And it seems it does :cry:

I’m just following the CWait() op template (or pseudo-code). Some of the actual instructions (like releasing the mutex or suspending a task) might perform the context switch, so a yielding isn’t needed at all.

I’m stuck with CWait(), but the companion op, CNotify(), is simpler, something like this:

Dequeue a thread
Resume the thread

I would like to read it! Hope you find it. There are already conditional variable implementations in Linux, QT, C and C++, but such thread APIs are far different from that of FreeRTOS. That’s way I searched over simpler ones.

(While doing my research I read that the multithread system on WinNT (or so) was a gem. Perhaps it’s the work you were part of :smiley: . )

As a conclusion, I’ll need to code a solution and then look for flaws when running in real life.

Thank you to everyone that took the time to point me out in the right direction.

The underlying project from my original question came up (conditional variables) is now up and running. It means the system never hangs and never misses data, although any comment or testbed will be highly appreciated.

The complete source code can be cloned here. It misses some options, but you can get the idea. It includes an example of the application of conditional variables.

In order for this thread to be complete I’m including the core of my original question, the CWait() function:

template<size_t Len>
class ConditionV
{
private:
   TaskHandle_t queue[ Len ]; 
   size_t head{ 0 };
   size_t tail{ 0 };
   size_t max { Len };
   size_t len { 0 };

public:
   void CWait( SemaphoreHandle_t& mutex )
   {
      TaskHandle_t self;

      taskENTER_CRITICAL();
      {
         self = xTaskGetCurrentTaskHandle();

         configASSERT( this->len < this->max );
         this->queue[ this->tail++ ] = self;
         if( this->tail == this->max ) this->tail = 0;
         ++this->len;

      } taskEXIT_CRITICAL();

      xSemaphoreGive( mutex );
      vTaskSuspend( self );
      
      xSemaphoreTake( mutex, portMAX_DELAY );
   }

   void CNotify()
   {
      TaskHandle_t task;

      taskENTER_CRITICAL();
      {
         if( this->len > 0 ){
            task = this->queue[ this->head++ ];
            if( this->head == this->max ) this->head = 0;
            --this->len;
         }
      } taskEXIT_CRITICAL();

      vTaskResume( task );
   }
};

Hi Xavier,

thanks for sharing your work!

I can’t seem to be able to look at the repository you posted (in my browser, it’s but an empty page). Can you outline the test code (at least the places where the CWait() and CNotify() members are called)?

Also, since you use taskEnterCritical(), it is implied that it’s only FreeRTOS tasks (not ISRs) that use your sync mechanism, in which case using a mutex instead of the system wide critical section is preferrable.

Probably worth pointing out that you have a data-race between the taskEXIT_CRITICAL() in CWait and the vTaskSuspend call. If CNotify() gets called by another thread in that window, it might try to resume the thread before the thread suspends itself, and the resume will get lost.

That is a fundamental issue with the use of suspend/resume.

Also, it looks like CNotify will only wake up one task, and it does the oldest, not the highest priority, so this might not meet real-time requirements. I thought Condition Variables would wake up ALL tasks waiting on the change, in which case an event-group might make more sense.

If you want it to wake up the just the highest priority task waiting, then you could use a semaphore.

Hi,

Please take a look int the repo again, it seems ok. Anyway here is the code you requested:

ConditionV<4> data_avail;
ConditionV<4> space_avail;

char buf[ 8 ];
size_t head{ 0 };
size_t tail{ 0 };
size_t max { 8 };
size_t len { 0 };

SemaphoreHandle_t mutex;


void put( char c )
{
   xSemaphoreTake( mutex, portMAX_DELAY );

   while( len >= max ){
      DEBUG_PRINT0( "*\n" );
      space_avail.CWait( mutex );
   }

   buf[ tail++ ] = c;
   if( tail >= max ) tail = 0;
   ++len;

   data_avail.CNotify();
   DEBUG_PRINT0( "/\n" );

   xSemaphoreGive( mutex );
}

char get()
{
   xSemaphoreTake( mutex, portMAX_DELAY );

   while( len == 0 ){
      DEBUG_PRINT0( "+\n" );
      data_avail.CWait( mutex );
   }

   char c = buf[ head++ ];
   if( head >= max ) head = 0;
   --len;

   space_avail.CNotify();
   DEBUG_PRINT0( "&\n" );

   xSemaphoreGive( mutex );

   return c;
}

void producer( void* pvParameters )
{
   uint32_t offset = (uint32_t)pvParameters;
   char cont = 0;

   char* task_name = pcTaskGetName( NULL );

   char letter = offset;

   while( 1 )
   {
      DEBUGOUT( "%s->%c\n", task_name, letter );
      put( letter++ );

      ++cont;
      if( cont == 10 ){
         cont = 0;
         letter = offset;
      }

      vTaskDelay( pdMS_TO_TICKS( ( rand() % 50 ) + 20 ) );
   }
}

Yes, that’s another option. Actually books talk about a double mutex, one for the shared resource and one for the conditional variable. I picked a CS up instead just for proof of concept.

Mutexes shouldn’t be used inside ISRs unless they return immediatly. Better off to use deferred tasks.

Hi,

Thank you for your comments.

Both functions xSemaphoreGive() and vTaskSuspend() request a context switch, so I’m not sure whether reordering, or including them into the CS, makes a difference (race condition). Sadly I don’t know any other way of suspending a task out of the API that don’t request the context switch, even the indexed notifications, I can say. But it worths to give a try: blocking a task on some controlled event.

Yes, it was done that way by design (proof of concept). One can choose any mechanism that fits the best his/her system. Some books say “wake up a random task from the queue”. I choose the oldest. Any way, this implementation might include the task’s priority and always put the one with the highest at the queue’s head. The hardwork has been done.

Yes, there is a function NotifyAll() that I hadn’t written at the moment I upload the code, but I’ve done (however, they aren’t in the repo so far because I’m doing some tests). For example, such function is:

  /**
    * @brief Wakes up all tasks.
    */
   void CNotifyAll()
   {
      taskENTER_CRITICAL();
      {
         while( this->len ) CNotify();
      } taskEXIT_CRITICAL();
   }

Also I added CWaitFor() and CWaitUntil() functions:

   // new version: includes the time and returns whether the mutex was aquired
   bool CWait( SemaphoreHandle_t& mutex, TickType_t ticks = portMAX_DELAY )
   {
      TaskHandle_t self;

      taskENTER_CRITICAL();
      {
         self = xTaskGetCurrentTaskHandle();

         configASSERT( this->len < this->max );
         this->queue[ this->tail++ ] = self;
         if( this->tail == this->max ) this->tail = 0;
         ++this->len;

      } taskEXIT_CRITICAL();

      xSemaphoreGive( mutex );
      vTaskSuspend( self );
      
      return xSemaphoreTake( mutex, ticks );
   }

   bool CWaitFor( SemaphoreHandle_t& mutex, TickType_t ticks )
   {
      return CWait( mutex, ticks );
   }

   bool CWaitUntil( SemaphoreHandle_t mutex, TickType_t& last, TickType_t ticks ) 
   {
      const TickType_t now = xTaskGetTickCount();

      TickType_t until = last + ticks - now;
      // overflows are handled automatically

      last = until;

      return CWait( mutex, until );
   }

Those changes aren’t in the repo yet. Please come check it out from time to time.

Any other comment is appreciated. Greetings!

The race condition exist even without the xSemaphoreGive() as once you leave the context switch you might get a task switch due to an interrupt, and that might call the cNotify and cause the missed resume to happen.

Doing an action that might cause a context switch within a critical section has special rules, which are dependent on the port being used, and I am not sure is defined for doing the suspend of self.

As I mentioned, you could wait on a semaphore to release the highest priority waiting task, or you could use a direct to task notification instead of the suspend/resume to get the same effect but without the race, as if the notify happens before the wait it is remembered, while the resume is not.

No, they CAN NOT be used inside ISRs (for one thing because they track ownership). But neither can taskEnterCritical() which you use is your sample code which implies that you can not call your sync functions in an ISR, and thus muteces are sufficient.

EDIT: Since your synchronization mechanism keeps a local list of calling tasks as its central data structure obtained via xTaskGetCurrentTaskHandle(), your CWait() can not possibly be called from an ISR. CNotify(), not adding to the list but only reading from it, could theoretically be called in an ISR context but then would need to call xTaskResumeFromISR() instead of vTaskResume(). In your code there is no provision for that, so we are talking task contexts only.

Thus, you should use an object local mutex to protect the array of task handles. The crit section is way too much of an overkill since it blocks all FreeRTOS conformant ISRs from executing (including, somewhat of a side effect, the sys tick handler and along with it the task scheduler). Thus you crank your entire system to a grinding halt to accomplish a simple mutual exclusion. Don’t do that.

I’ll comment on the rest later on.

Hi Xavier,

I agree with Richard’s observations.

I’d also like to point out that your test suite is sloppy in several respects, one of them being that although your CWait() member function has a return value, neither of its callers check on the return value, implying that the tasks calling get() and/or set() may erratically try to release a mutex they don’t own. This can lead to extremly subtle and hard to track runtime behaviors, in particular when a timeout is specified as a parameter to CWait().

Edit: BTW, I was able to locate a copy of my earlier article in case you are interested: Compound Synchronization Objects | Microsoft Docs