Bug report for uxTaskPriorityGet

Hi All,
I find that task’s priority can be unexpectedly changed in certain cases. For an example:

        /* protect resources that require exclusive access */
        xSemaphoreTakeRecursive(my_mutex, portMAX_DELAY);

        /* 
          * For some reason, I need to temporarily increase the priority of current 
          * task. So, I save the old priority first.
          */
        int old_prio = uxTaskPriorityGet(NULL);

        /* then, I increase the priority of current task */
        vTaskPrioritySet(NULL, configMAX_PRIORITIES - 1);
        
        /* do something... */

        /* now restore the old priority */
        vTaskPrioritySet(NULL, old_prio);

The above code works well in most cases. But, If priority inheritance occurs before uxTaskPriorityGet is called, current task’s uxBasePriority will be changed to the priority inherited from another task. This means that current task’s priority is permanently increased.

I think the root case is that uxTaskPriorityGet and vTaskPrioritySet work in an asymmetrical way.
In uxTaskPriorityGet:

pxTCB = prvGetTCBFromHandle( xTask );
uxReturn = pxTCB->uxPriority;

however, in vTaskPrioritySet:

#if ( configUSE_MUTEXES == 1 )
      {
              /* omit some code */

              /* The base priority gets set whatever. */
              pxTCB->uxBasePriority = uxNewPriority;
       }
#else /* if ( configUSE_MUTEXES == 1 ) */
       {
              pxTCB->uxPriority = uxNewPriority;
       }
#endif /* if ( configUSE_MUTEXES == 1 ) */

I am not sure whether this is a bug, so I want to get some advice from the official. If this is not a bug, why is uxTaskPriorityGet designed to always get uxPriority, which is a temporary priority when configUSE_MUTEXES is configured.

Don’t have the code in front of me now but I think what you report is the intended behavior, if I understand you correctly. Namely that a task will report the priority it is currently running at, which may be an inherently priority. If you then store that priority and later set the task’s priority using the stored value then you are permanently changing the task’s base priority.

If this were not the case, and setting the priority of a task while it was running with an inherited priority only changed the inherited priority and not the base priority, then an intentional attempt to change the base priority could unexpectedly fail when the change is reverted as the task disinherited the priority again.

Neither way round is ideal in all cases though - interested in thoughts.

I’d suggest to abandon the Get/Set priority API in favor of disjoint Get/SetBasePri and Get/SetCurrentPri APIs to disambiguate. There are imaginable use cases for either.

1 Like

Thank you for your attention to this issue. Can you share some knowledge about In what case do we need a temporary priority(uxPriority)? To make my code work, I changed uxTaskPriorityGet, making this function return uxBasePriority when configuring configUSE_MUTEXES. I want to assess the potential impact of doing so.

Hi lenny,

I wouldn’t use the term “temporary” but rather “current.” I’m sure you understand the motivation behind priority inheritance, so while a task is subject to an inherited priority, that priority IS the task’s (current) priority.

To me, intuitively an API that returns a task’s priority would return the current dynamic priority, not the configured “default” static one, but I can see your motivation to use the configured priority. Thus my suggestion to explicify which one to use via disjoint APIs.

That said, I’d also like to point out that temporarily changing the priority of a task that is already subject to dynamic boosting due to priority inheritance does bear potential for problems in the first place. Is your temporary priority change a further boost or a lowering (the latter would sort of contradict the idea of pri inheritance)? What is your motivation to further change the pri of a task that already runs under an adjusted dynamic priority?

I will also point out that needing to change a tasks priority at a ‘random’ place is a sign of code smell, something that is likely incorrect, and may indicate a design problem.

The one case where I might do something like this is with a ‘task pool’ but then the priority would get set with the task in a specific state where it won’t have a mutex held, so no priority inheritance problem.

Hi, RAc

My motivation is that increasing current task’s priority to prevent the task from being preempted. For some reason, I can’t use API vTaskSuspendAll, so I choose to increase current task’s priority. It works well when priority inheritance does not occur.

Now, I find that my modification to uxTaskPriorityGet isn’t correct. Because, it can only prevent uxBasePriority from being modified. But in vTaskPrioritySet:

                       /* Only change the priority being used if the task is not
                         * currently using an inherited priority. */
                        if( pxTCB->uxBasePriority == pxTCB->uxPriority )
                        {
                            pxTCB->uxPriority = uxNewPriority;
                        }
                        else
                        {
                            mtCOVERAGE_TEST_MARKER();
                        }

                        /* The base priority gets set whatever. */
                        pxTCB->uxBasePriority = uxNewPriority;

When priority inheritance happens, uxPriority adopted by the scheduler will not be set. Maybe what I want is an API like vTaskPrioritySetCurrent.

I think your suggestion is excellent. Meanwhile, it is risky to separate the setget operation of uxPriority and uxBasePriority, because of priority inheritance and disinheritance. These operations must be used very carefully.

The biggest problem I face now is that the official does not provide an API like vTaskPrioritySetCurrent. And, it is very difficult to modify the design of the code. In addition to implementing vTaskPrioritySetCurrent myself, Is there any better way?

I think your posts are proving why changing the current behaviour would not be a good idea. What if you set the current priority high enough for your mutual exclusion use case, then unknown to you, the priority was disinherited, breaking the mutual exclusion without the code knowing.

From your last post, this sounds like the actual problem to be fixed: " For some reason, I can’t use API `vTaskSuspendAll" - and changing the task priority is trying to work around an unexplained scenario. If that is genuinely not working then you have more serious problems to contend with and fixing those problems will remove the need to change task priorities.

Can you elaborate on “I can’t use API vTaskSuspendAll”?

If you really need code that is so time critical that you can’t afford any other thread of execution to steal CPU cycles, you might consider using the critical section. Again, I’m not an advocate of application’s use of the CS, but if that’s your use case, it’s probably your safest bet. You wouldn’t even need the mutex anymore because once you’ve got the CS, you have the CPU completly for yourself (aside from non conforming interrupts, of course).

That doesn’t make any sense at all. Please explain what works well and what doesn’t. Priority inheritance will only RAISE the priority of a task obtaining a mutex, never lower it, so whatever happens when a task has a mutex will by definition only enhance its throughput, not deteriorate it. You may wish to outline the intended control flow a little bit more in detail.

Like Richard, I’d also be interested in what the issue with vTaskSuspendAll() is.

If I can guarantee that the current priority increased previously will be restored before disinheritance, then there will be no unknown problems. I mean that increase and restore happen between lock and unlock.

I don’t mean to change the behavior of vTaskPrioritySet, I know that this function has its context of use. I want to know if freeRTOS can provide an API like vTaskPrioritySetCurrent, maybe this function has its context of use too.

I can’t use CS, because CS can only take one core. My code run on processor with two cores. I need another core stop(busy wait) during my access to critical resource.

The reason I can’t use vTaskSuspendAll is:

        /* protect resources that require exclusive access */
        xSemaphoreTakeRecursive(my_mutex, portMAX_DELAY);

        /* 
          * I need to temporarily increase the priority of current 
          * task. So, I save the old priority first.
          */
        int old_prio = uxTaskPriorityGet(NULL);

        /* then, increase the priority of current task to the highest */
        vTaskPrioritySet(NULL, configMAX_PRIORITIES - 1);
        
        /* 
          * do something:
          * Communicate with another core, make another core “busy wait” until the end of access 
          * to critical resources. During this process, some functions for synchronization like 
          * `xSemaphoreTake`、`xSemaphoreGive` will be used, so vTaskSuspendAll can not be 
          * used.
          *
          * Although the current task may be scheduled out, a highest priority allows the task to 
          * be run immediately when the condition is met. If not increase current task's priority,
          * current task may be preempted by another task, which can be called task1.
          * If task1 "busy wait" some signal from another core, which is already “busy waiting”, 
          * then deadlock happens. That's why I need to increase current task's priority.
          *
          * As a supplement, task1 may be customer's code, out of my control.
          */
         
        /* now, vTaskSuspendAll can be called */
        vTaskSuspendAll();

        /* then restore the old priority */
        vTaskPrioritySet(NULL, old_prio);

Is this an SMP application (with one instance of FreeRTOS scheduling tasks on multiple cores), or AMP (one instance of FreeRTOS per core)? Which chip are you using?

Well, in either case, you (lenny) always have the option to use binary semaphores instead of muteces as binary semaphores are by definition not subject to priority inheritance.

This is an SMP application, chip is esp32.

In which case I think critical sections already work across cores.

Yeah, critical section can work across cores. In most instances, critical section can fully protect critical resources(like UARTi2c controller) in a multi-core system. But I want to access flash, a special peripheral of esp32.

esp32’s flash not only supports storing data but also supports XIP. If task0 on core0 need to access flash, task0 must ensure that tasks running on core1 cannot access flash, and cannot call functions located in flash. Besides, core1 cannot execute interrupt handlers located in flash.

So, In this case, critical section is not enough.

Thanks for your advice. Binary semaphore is indeed effective. But there is still a potential risk. We will encapsulate the operations we did before accessing the flash into a function, let’s call it do_something_before_access_flash:

void do_something_before_access_flash(void)
{
        /* binary semaphore */
       xSemaphoreTake(my_semaphore, portMAX_DELAY);

        /* 
          * I need to temporarily increase the priority of current 
          * task. So, I save the old priority first.
          */
        int old_prio = uxTaskPriorityGet(NULL);

        /* then, increase the priority of current task to the highest */
        vTaskPrioritySet(NULL, configMAX_PRIORITIES - 1);
        
       ......
}

This function will be provided to customers. I cannot guarantee that customers’ task does not have priority inheritance before calling this function.

The most suitable method we have found so far is to add an API : vTaskPrioritySetCurrent. I know this function is risky and must be used carefully.

I want to ask the official for some advice. Is it appropriate to add this API?

I don’t think adding that function is desirable and could even be dangerous for your situation. Changing the task priority through the api at the same time it is being manipulated by the priority inheritance mechanism is going to lead to race conditions and non deterministic behavior.

Thank you for your advice. And I have a question about vTaskPrioritySet, in this function:

#if ( configUSE_MUTEXES == 1 )
				{
					/* Only change the priority being used if the task is not
					currently using an inherited priority. */
					if( pxTCB->uxBasePriority == pxTCB->uxPriority )
					{
						pxTCB->uxPriority = uxNewPriority;
					}
					else
					{
						mtCOVERAGE_TEST_MARKER();
					}

					/* The base priority gets set whatever. */
					pxTCB->uxBasePriority = uxNewPriority;
				}
#else
......

I noticed that once priority inheritance occurs, uxPriority will not be set. My question is:

If the priority set by vTaskPrioritySet is higher than the inherited priority, why not use the higher one?

like this:

if( pxTCB->uxBasePriority == pxTCB->uxPriority || uxNewPriority > pxTCB->uxPriority )
{
	pxTCB->uxPriority = uxNewPriority;
}

Otherwise, the priority set by vTaskPrioritySet will not take effect until the disinheritance occurs.

I am looking forward to your reply, thank you.