External Memory for Pthread (IDFGH-9237)

[f-hoepfinger-hr-agrartechnik]

#8662

[CLAassistant]

All committers have signed the CLA.

[igrr]

Please note that the stack and the TCB of a task allocated using xTaskCreateStaticPinnedToCore are not deleted automatically. You also need to modify the pthread deletion code to free the memory, otherwise it would be leaked.

I've also left a few other comments below.

[igrr]

Also please combine the changes into a single commit (using git rebase -i) to keep the commit history clean.

[f-hoepfinger-hr-agrartechnik]

Also please combine the changes into a single commit (using git rebase -i) to keep the commit history clean.

like this ?

[f-hoepfinger-hr-agrartechnik]

This code fails with a assertion at thread.join();

[f-hoepfinger-hr-agrartechnik]

no, still
assert failed: prvDeleteTCB tasks.c:4758 (pxTCB->ucStaticallyAllocated == ( ( uint8_t ) 2 ))

[f-hoepfinger-hr-agrartechnik]

no, still assert failed: prvDeleteTCB tasks.c:4758 (pxTCB->ucStaticallyAllocated == ( ( uint8_t ) 2 ))

ah, but this time in the Idle Task.
need more Help.

[f-hoepfinger-hr-agrartechnik]

https://github.com/hr-agrartechnik/esp-idf-examples-cxx-pthread

Crash on join()

It is reproducible.
So need to debug on this minimal example.

[0xjakob]

@f-hoepfinger-hr-agrartechnik Thanks for addressing the pthread external memory issue and sorry for the slow turnaround!

I looked at the code and most of it looks good. I have, however two points that I'm not certain about, please see the comments in the code.

We'll talk about the issue with the esp_pthread_set_cfg() API (see my comments) and let you know about any results.

[0xjakob]

I'm afraid this might cause trouble for users who initialized each member of esp_pthread_cfg_t manually like this:

    esp_pthread_cfg_t cfg;
    cfg.thread_name = name;
    cfg.pin_to_core = core_id;
    cfg.stack_size = stack;
    cfg.prio = prio;
    cfg.inherit_cfg = false;

If they would upgrade IDF including this change, esp_pthread_cfg_t.stack_alloc_caps is uninitialized. Once the configuration is used, this would produce a crash at runtime at best or a way more subtle memory issue in the worst case.

I don't have a definitive solution for this yet. The esp_pthread_set_cfg makes it hard to introduce non-breaking configuration changes.

[0xjakob]

Sorry, I missed that our documentation is explicitly advising users to zero-initialize configuration structures. You can consider this comment resolved, then.

[0xjakob]

What was the reason for moving vTaskDelete(NULL) here? It seems that the semantics are changed significantly: the following two statements won't be executed, the log call and giving the semaphore below won't be executed, either.

[0xjakob]

Please take a look at pthread unit tests. They produce a deadlock, likely due to moving the vTaskDelete() call here which prevents the mutex from being unlocked below (xSemaphoreGive(s_threads_mux);).

[donghengqaz]

no, still assert failed: prvDeleteTCB tasks.c:4758 (pxTCB->ucStaticallyAllocated == ( ( uint8_t ) 2 ))

It looks like the freeRTOS forbidden to delete a full static(static task stack and TCB) task, so code has assert process. @f-hoepfinger-hr-agrartechnik @0xjakob using xTaskCreateRestrictedStatic may be solution here.

[0xjakob]

https://github.com/hr-agrartechnik/esp-idf-examples-cxx-pthread

Crash on join()

It is reproducible. So need to debug on this minimal example.

@f-hoepfinger-hr-agrartechnik could you elaborate "Crash on join()"? Given your change in IDF, I can compile the linked example application, modified so that it actually uses SPI RAM (external memory), and run it without error. The only prerequisites are enabling SPI RAM as well as allowing SPI RAM for FreeRTOS task stacks in menuconfig. I used an ESP32-WROVER Development board.

[0xjakob]

@f-hoepfinger-hr-agrartechnik I think saving the stack memory in the struct esp_pthread_t does not work since the deletion must be deferred if the thread is deleting itself, this might be the cause of some of the issues you observed. The best way to defer stack deletion seems to be via vPortCleanUpTCB(), which needs to be enabled and provided.

I'll take a look at this and provide suggestions this week.

[diplfranzhoepfinger]

I'll take a look at this and provide suggestions this week.

Thanks so much. Very Helpful !

[f-hoepfinger-hr-agrartechnik]

I'll take a look at this and provide suggestions this week.

any news ?

[0xjakob]

@f-hoepfinger-hr-agrartechnik We needed to go a long way because we needed to create new private FreeRTOS functions for this use case. We have merged the FreeRTOS part already. Now we're in the process of merging the final pthread MR. It will be quite different compared to your code, but we'll leave a thanks in the commit message for sure!

[diplfranzhoepfinger]

@0xjakob any idea when we can test this ?

[0xjakob]

@diplfranzhoepfinger It's been merged to master now, PTAL. There's a unit test which shows how to use the API.

[diplfranzhoepfinger]

Very cool. Want to test it.