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

Clean-up the TCB at the right time. #359

Closed
wants to merge 1 commit into from

Conversation

danicampora
Copy link
Contributor

In the ESP32 port the current statement about clean-up TCB does not apply:

/* This call is required specifically for the TriCore port.  It must be
above the vPortFree() calls.  The call is also used by ports/demos that
want to allocate and clean RAM statically. */
portCLEAN_UP_TCB( pxTCB );

Because it's not a tri-core port. What is actually useful is the ability to "release" an statically allocated TCB when a task is deleted. In order to do so, portCLEAN_UP_TCB can be used, but it needs to be called after all the accesses to the TCB are performed. In the current location, releasing the TCB causes a crash because the TCB is accessed by FreeRTOS several times afterwards. The proposed patch moves the call to the end and inside the check for a statically allocated TCB (where it makes sense).

In the ESP32 port the current statement about clean-up TCB does not apply:

```c
/* This call is required specifically for the TriCore port.  It must be
above the vPortFree() calls.  The call is also used by ports/demos that
want to allocate and clean RAM statically. */
portCLEAN_UP_TCB( pxTCB );
```

Because it's not a tri-core port. What is actually useful is the ability to "release" an statically allocated TCB when a task is deleted. In order to do so, ```portCLEAN_UP_TCB``` can be used, but it needs to be called after all the accesses to the TCB are performed. In the current location, releasing the TCB causes a crash because the TCB is accessed by FreeRTOS several times afterwards. The proposed patch moves the call to the end and inside the check for a statically allocated TCB (where it makes sense).
@danicampora
Copy link
Contributor Author

@igrr this one is also needed for Threading to work properly on MicroPython. Related to this other one as well: #358

@projectgus
Copy link
Contributor

To be sure I understand the motivation here: the cleanup work in this case will be a callback into the Micropython core, so Micropython's thread accounting knows this task no longer exists?

@dpgeorge
Copy link

the cleanup work in this case will be a callback into the Micropython core, so Micropython's thread accounting knows this task no longer exists?

Yes, it needs to free the memory for the stack and TCB itself for the thread that is being deleted. This memory is allocated on the uPy heap. You can't free the TCB memory until the very last moment because it's used by FreeRTOS to do the deletion of the thread.

@projectgus
Copy link
Contributor

Do you mind sending this one as a new PR (or updating this PR) when you send the Kconfig item for enabling it as a feature?

(If that makes things more difficult for you in terms of maintaining forks then we can merge this earlier, it just seems like they are closely related changes.)

@danicampora
Copy link
Contributor Author

@projectgus OK, I'll create a new PR with this one and #358 plus the Kconfig changes included. Thanks.

@danicampora
Copy link
Contributor Author

Sorry for the long delay. Superseded by: #444

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

Successfully merging this pull request may close these issues.

None yet

3 participants