Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Critial region issues in tasks.c #374

Closed
gerhil opened this issue Feb 23, 2017 · 8 comments
Closed

Critial region issues in tasks.c #374

gerhil opened this issue Feb 23, 2017 · 8 comments

Comments

@gerhil
Copy link

gerhil commented Feb 23, 2017

Currently, vTaskResume(..) and vTaskSuspend(..)are useless in critical regions. Also, the critical region implementation is incomplete!

A Resume issue!

Line 1882 of method vTaskResume(..) in freertos/tasks.c is unexpected! I think this is in conflict with critical regions. Same for line 1886.

Line 1882:

taskYIELD_IF_USING_PREEMPTION_MUX(&xTaskQueueMutex);

This performs a yield to (possibly) the resumed thread. A resume should only resume and not yield!!!!

For example if you would use

taskENTER_CRITICAL(&myMutex);
vTaskResume(newThreadHandle);
taskEXIT_CRITICAL(&myMutex);

I would expect that there is no context switching. However, vTaskResume(..) causes a context switch (yield) to the newThreadHandle thread of control.

Please, read taskENTER_CRITICAL() description.

"Preemptive context switches only occur inside an interrupt, so will not occur when interrupts are disabled. Therefore, the task that called taskENTER_CRITICAL() is guaranteed to remain in the Running state until the critical section is exited, unless the task explicitly attempts to block or yield (which it should not do from inside a critical section)."

Suggestion: leaving line 1882 out of freertos/tasks.c is better! What do you think?

A Suspend issue!

Consider the following example:

taskENTER_CRITICAL(&myMutex);
vTaskResume(newThreadHandle);
vTaskSuspend(thisThreadHandle);
taskEXIT_CRITICAL(&myMutex);

I would expect that vTaskSuspend(thisThreadHandle) should not suspend immediately, it should suspend on taskEXIT_CRITICAL(&myMutex).

vTaskSuspend() should only perform portYIELD_WITHIN_API() when it is not in a critical region, otherwise the outer taskEXIT_CRITICAL(&xTaskQueueMutex) should perform context switching (it should take the next ready thread of control).

I think this has been fixed for the ESP8266.

Please, let me know what you think.

@Spritetm
Copy link
Member

Hmm, you may have a point here. If I recall correctly, the voluntary-yield-when-resuming-a-task behaviour comes from the original FreeRTOS implementation; if so, there's something that can be said for keeping that behaviour. Your line of thinking definitely has merit as well, however. I need to think this over for a while as well as check the original FreeRTOS sources.

@gerhil
Copy link
Author

gerhil commented Feb 24, 2017

Thanks! Well, tasks,c suppose to be the same for all platforms. I have checked FreeRTOS and there were some discussions about yield in a critical regions. They suggest not to perform yield in a critical region unless explicitly you use yield() (=voluntarily yield). However, this is a bad idea, but one is free to do so.

http://www.freertos.org/FreeRTOS_Support_Forum_Archive/May_2006/freertos_Yield_in_critical_section_1494178.html

Another discussion (which I cannot find right now) was about pending the yield until the end of the critical region. This behavior is implemented by the port. So we should look at the port of the critical region. I found this, read last message.

http://www.openrtos.net/FreeRTOS_Support_Forum_Archive/April_2009/freertos_calling_vTaskDelete_when_in_critical_section_3165469.html

The ESP8266 and PC implementations are pending the yield until the end of the critical region. This is good! My program works with FreeRTOS (9.0.0 and 8.2.0) for the ESP8266 and the PC, but not for the ESP32 (esp-idf).

@gerhil
Copy link
Author

gerhil commented Feb 24, 2017

Hi Jeroen, I saw that you studied electrical engineering at the University of Twente until 2006. I was a PhD student at electrical engineering (UT, control lab, van Amerongen) until 2005.

If you wonder why I use suspend, resume and critical region is because I am developing a new real-time programmingmodel for embedded systems. This is called Waza. It uses FreeRTOS for context switching and FreeRTOS allows me to port Waza easily to many platforms. Waza provides true concurrent programming. Waza is based on formal semantics for building parallel software for single core and multi-core microprocessors. It can be used for programming and it can also be use with formal methods in order to proof the correctness and completeness of the software by evidence.

Waza is in many ways superior than FreeRTOS with respect to programming and reliability. FreeRTOS is low-level, difficult-to-use and error-prone. FreeRTOS is not based on formal semantics and it does not provide a formal foundation to multi-core OSes. Waza is best for the software developer, and FreeRTOS is best for the processor. FreeRTOS provide multi-threading, it certainly does NOT provide concurrency. The applications I build with Waza behave guaranteed the same on all platforms. This includes non-determinism.

Waza makes the ESP32 a powerful parallel computing unit for iot-applications.

Cheers,
Gerald Hilderink
www.wazalogic.com

@projectgus projectgus added the Status: In Progress Work is in progress label Mar 16, 2017
@projectgus
Copy link
Contributor

Hi @Wazalogic ,

Waza sounds very interesting, I look forward to seeing it when it becomes available.

We recently ran into some other issues related to immediately context switching from API calls in this way[*]. A change is pending to defer the immediate context switch (for yields and also other API operations which unblock a different task) until all critical sections have exited. This is similar to the behaviour on many other FreeRTOS ports (not the Xtensa reference port, for some reason), and seems like the more correct behaviour.

Angus

[*] For example, if a lower priority task gives a semaphore and this wakes a higher priority task which then deletes that semaphore. This seems like correct semantics, but with the current implementation or the Xtensa reference implementation this is invalid because the higher task was yielded to inside the xQueueGenericSend() function, and that function still accesses some of the data held inside the semaphore's data structure.

@gerhil
Copy link
Author

gerhil commented Mar 16, 2017

I think this is a serious problem that needs to be fixed. I am glad that this problem is looked into and hopefully it will be fixed soon.

Thanks,
Gerald.

@gerhil
Copy link
Author

gerhil commented Apr 3, 2017

Any news on this issue?

@projectgus
Copy link
Contributor

Hi @gerhil ,

The changes are still in our review queue, they got delayed due to some more urgent work. But I expect they will be merged this week. Thanks for your patience with this issue.

Angus

@gerhil
Copy link
Author

gerhil commented Apr 4, 2017

Hi Angus,

Thanks. I am glad to help.

Gerald.

@igrr igrr closed this as completed in 45581db Apr 6, 2017
igrr pushed a commit that referenced this issue Apr 6, 2017
freertos: Delay context switch from queue/task APIs until exiting critical section

Closes #374: #374

See merge request !610
@igrr igrr removed the Status: In Progress Work is in progress label Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants