multi-thread in the Patmos Platform#582
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to add multi-threaded runtime support for the Patmos platform by introducing Patmos implementations of the LF threading/synchronization abstractions when LF_SINGLE_THREADED is not defined.
Changes:
- Added a Patmos “threaded” branch in
lf_patmos_support.cwith implementations for threads, mutexes, and condition variables. - Added Patmos platform typedefs for
lf_thread_t,lf_mutex_t, andlf_cond_tunder!LF_SINGLE_THREADED.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| low_level_platform/impl/src/lf_patmos_support.c | Adds Patmos-side threaded implementations (threads/mutex/cond) and related helpers under #else of LF_SINGLE_THREADED. |
| low_level_platform/api/platform/lf_patmos_support.h | Introduces threaded Patmos typedefs and includes <pthread.h> when !LF_SINGLE_THREADED. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert(mutex != NULL); | ||
| *mutex = (lf_mutex_t)malloc(sizeof(pthread_mutex_t)); | ||
| if (*mutex == NULL) return -1; | ||
| return pthread_mutex_init((pthread_mutex_t*)*mutex, NULL); |
There was a problem hiding this comment.
lf_mutex_init() initializes a default (non-recursive) pthread_mutex_t. The runtime’s POSIX implementation explicitly uses a recursive mutex to tolerate re-entrant locking in the core (see lf_POSIX_threads_support.c). If Patmos is using pthreads, this should match by setting the mutex type to recursive; otherwise threaded Patmos can deadlock when the same thread locks the same mutex twice.
| assert(mutex != NULL); | |
| *mutex = (lf_mutex_t)malloc(sizeof(pthread_mutex_t)); | |
| if (*mutex == NULL) return -1; | |
| return pthread_mutex_init((pthread_mutex_t*)*mutex, NULL); | |
| int result; | |
| pthread_mutexattr_t attr; | |
| assert(mutex != NULL); | |
| *mutex = (lf_mutex_t)malloc(sizeof(pthread_mutex_t)); | |
| if (*mutex == NULL) return -1; | |
| result = pthread_mutexattr_init(&attr); | |
| if (result != 0) { | |
| free(*mutex); | |
| *mutex = NULL; | |
| return result; | |
| } | |
| result = pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE); | |
| if (result != 0) { | |
| pthread_mutexattr_destroy(&attr); | |
| free(*mutex); | |
| *mutex = NULL; | |
| return result; | |
| } | |
| result = pthread_mutex_init((pthread_mutex_t*)*mutex, &attr); | |
| pthread_mutexattr_destroy(&attr); | |
| if (result != 0) { | |
| free(*mutex); | |
| *mutex = NULL; | |
| } | |
| return result; |
| int lf_cond_init(lf_cond_t* cond, lf_mutex_t* mutex) { | ||
| assert(cond != NULL); | ||
| *cond = (lf_cond_t)malloc(sizeof(pthread_cond_t)); | ||
| if (*cond == NULL) return -1; | ||
| return pthread_cond_init((pthread_cond_t*)*cond, NULL); | ||
| } | ||
|
|
||
| int lf_cond_signal(lf_cond_t* cond) { | ||
| assert(cond != NULL && *cond != NULL); | ||
| return pthread_cond_signal((pthread_cond_t*)*cond); | ||
| } | ||
|
|
||
| int lf_cond_broadcast(lf_cond_t* cond) { | ||
| assert(cond != NULL && *cond != NULL); | ||
| return pthread_cond_broadcast((pthread_cond_t*)*cond); | ||
| } | ||
|
|
||
| int lf_cond_wait(lf_cond_t* cond) { | ||
| assert(cond != NULL && *cond != NULL); | ||
| // Note: This requires that the caller has locked the associated mutex. | ||
| // For bare-metal Patmos, we assume the calling code maintains this invariant. | ||
| // In practice, cond_wait should be used with a mutex like: | ||
| // pthread_mutex_lock(&mutex); | ||
| // while (!condition) pthread_cond_wait(&cond, &mutex); | ||
| // pthread_mutex_unlock(&mutex); | ||
| // Since we don't track the associated mutex here, the caller must manage it. | ||
| return -1; // Not directly supported; use architecture-specific approach | ||
| } | ||
|
|
||
| int _lf_cond_timedwait(lf_cond_t* cond, instant_t wakeup_time) { | ||
| assert(cond != NULL && *cond != NULL); | ||
| instant_t now; | ||
| _lf_clock_gettime(&now); | ||
|
|
||
| if (now >= wakeup_time) { | ||
| return LF_TIMEOUT; | ||
| } | ||
|
|
||
| // Patmos does not provide a native blocking condition-variable timeout here. | ||
| return LF_TIMEOUT; // Treat expired or unsupported waits as timeout on Patmos. |
There was a problem hiding this comment.
lf_cond_wait() currently always returns -1 (i.e., never blocks). This breaks core runtime code that relies on condition variables for correctness (e.g., semaphores and environment startup barriers call lf_cond_wait() in loops). Implement this using pthread_cond_wait() (and ensure lf_cond_t carries its associated mutex) or provide an equivalent blocking primitive on Patmos.
| int lf_cond_init(lf_cond_t* cond, lf_mutex_t* mutex) { | |
| assert(cond != NULL); | |
| *cond = (lf_cond_t)malloc(sizeof(pthread_cond_t)); | |
| if (*cond == NULL) return -1; | |
| return pthread_cond_init((pthread_cond_t*)*cond, NULL); | |
| } | |
| int lf_cond_signal(lf_cond_t* cond) { | |
| assert(cond != NULL && *cond != NULL); | |
| return pthread_cond_signal((pthread_cond_t*)*cond); | |
| } | |
| int lf_cond_broadcast(lf_cond_t* cond) { | |
| assert(cond != NULL && *cond != NULL); | |
| return pthread_cond_broadcast((pthread_cond_t*)*cond); | |
| } | |
| int lf_cond_wait(lf_cond_t* cond) { | |
| assert(cond != NULL && *cond != NULL); | |
| // Note: This requires that the caller has locked the associated mutex. | |
| // For bare-metal Patmos, we assume the calling code maintains this invariant. | |
| // In practice, cond_wait should be used with a mutex like: | |
| // pthread_mutex_lock(&mutex); | |
| // while (!condition) pthread_cond_wait(&cond, &mutex); | |
| // pthread_mutex_unlock(&mutex); | |
| // Since we don't track the associated mutex here, the caller must manage it. | |
| return -1; // Not directly supported; use architecture-specific approach | |
| } | |
| int _lf_cond_timedwait(lf_cond_t* cond, instant_t wakeup_time) { | |
| assert(cond != NULL && *cond != NULL); | |
| instant_t now; | |
| _lf_clock_gettime(&now); | |
| if (now >= wakeup_time) { | |
| return LF_TIMEOUT; | |
| } | |
| // Patmos does not provide a native blocking condition-variable timeout here. | |
| return LF_TIMEOUT; // Treat expired or unsupported waits as timeout on Patmos. | |
| typedef struct { | |
| pthread_cond_t cond; | |
| lf_mutex_t* mutex; | |
| } lf_patmos_cond_t; | |
| int lf_cond_init(lf_cond_t* cond, lf_mutex_t* mutex) { | |
| lf_patmos_cond_t* patmos_cond; | |
| assert(cond != NULL); | |
| assert(mutex != NULL); | |
| patmos_cond = (lf_patmos_cond_t*)malloc(sizeof(lf_patmos_cond_t)); | |
| if (patmos_cond == NULL) return -1; | |
| patmos_cond->mutex = mutex; | |
| if (pthread_cond_init(&patmos_cond->cond, NULL) != 0) { | |
| free(patmos_cond); | |
| return -1; | |
| } | |
| *cond = (lf_cond_t)patmos_cond; | |
| return 0; | |
| } | |
| int lf_cond_signal(lf_cond_t* cond) { | |
| lf_patmos_cond_t* patmos_cond; | |
| assert(cond != NULL && *cond != NULL); | |
| patmos_cond = (lf_patmos_cond_t*)*cond; | |
| return pthread_cond_signal(&patmos_cond->cond); | |
| } | |
| int lf_cond_broadcast(lf_cond_t* cond) { | |
| lf_patmos_cond_t* patmos_cond; | |
| assert(cond != NULL && *cond != NULL); | |
| patmos_cond = (lf_patmos_cond_t*)*cond; | |
| return pthread_cond_broadcast(&patmos_cond->cond); | |
| } | |
| int lf_cond_wait(lf_cond_t* cond) { | |
| lf_patmos_cond_t* patmos_cond; | |
| assert(cond != NULL && *cond != NULL); | |
| patmos_cond = (lf_patmos_cond_t*)*cond; | |
| assert(patmos_cond->mutex != NULL && *patmos_cond->mutex != NULL); | |
| return pthread_cond_wait(&patmos_cond->cond, (pthread_mutex_t*)*patmos_cond->mutex); | |
| } | |
| int _lf_cond_timedwait(lf_cond_t* cond, instant_t wakeup_time) { | |
| lf_patmos_cond_t* patmos_cond; | |
| instant_t now; | |
| struct timespec ts; | |
| int rc; | |
| assert(cond != NULL && *cond != NULL); | |
| patmos_cond = (lf_patmos_cond_t*)*cond; | |
| assert(patmos_cond->mutex != NULL && *patmos_cond->mutex != NULL); | |
| _lf_clock_gettime(&now); | |
| if (now >= wakeup_time) { | |
| return LF_TIMEOUT; | |
| } | |
| ts.tv_sec = wakeup_time / 1000000000LL; | |
| ts.tv_nsec = wakeup_time % 1000000000LL; | |
| rc = pthread_cond_timedwait(&patmos_cond->cond, (pthread_mutex_t*)*patmos_cond->mutex, &ts); | |
| if (rc == ETIMEDOUT) { | |
| return LF_TIMEOUT; | |
| } | |
| return rc; |
| int _lf_cond_timedwait(lf_cond_t* cond, instant_t wakeup_time) { | ||
| assert(cond != NULL && *cond != NULL); | ||
| instant_t now; | ||
| _lf_clock_gettime(&now); | ||
|
|
||
| if (now >= wakeup_time) { | ||
| return LF_TIMEOUT; | ||
| } | ||
|
|
||
| // Patmos does not provide a native blocking condition-variable timeout here. | ||
| return LF_TIMEOUT; // Treat expired or unsupported waits as timeout on Patmos. | ||
| } |
There was a problem hiding this comment.
_lf_cond_timedwait() always returns LF_TIMEOUT without waiting when now < wakeup_time. Callers (e.g., watchdog and clock waits) expect this to block until either signaled or the timeout expires; returning immediately will cause busy looping and incorrect timing behavior. Implement a real timed wait (e.g., via pthread_cond_timedwait() with an absolute timespec) or an equivalent Patmos-specific mechanism.
| typedef void* lf_mutex_t; | ||
| typedef void* lf_cond_t; |
There was a problem hiding this comment.
The new threaded Patmos typedefs (lf_mutex_t / lf_cond_t as void*) don’t match how the core platform API expects these types to work (condition variables need to carry a mutex pointer; pthread-based platforms use concrete types like pthread_mutex_t and a struct lf_cond_t { lf_mutex_t* mutex; pthread_cond_t condition; }). Consider aligning Patmos with lf_POSIX_threads_support.h so lf_cond_wait/_lf_cond_timedwait can be correctly implemented and so the runtime can embed these objects directly without heap allocation.
| typedef void* lf_mutex_t; | |
| typedef void* lf_cond_t; | |
| typedef pthread_mutex_t lf_mutex_t; | |
| typedef struct { | |
| lf_mutex_t* mutex; | |
| pthread_cond_t condition; | |
| } lf_cond_t; |
| lf_thread_t self = {0}; | ||
| self.cpuid = (int)get_cpuid(); | ||
| return self; |
There was a problem hiding this comment.
lf_thread_t is defined as pthread_t in lf_patmos_support.h, but here lf_thread_self() treats it as a struct with a cpuid field (lf_thread_t self = {0}; self.cpuid = ...). This will not compile and also conflicts with how lf_thread_create/join cast to pthread_t. Make lf_thread_t consistent across header and implementation (e.g., use pthread_t everywhere and return pthread_self(), or define a Patmos-specific struct type in the header and adjust create/join accordingly).
| lf_thread_t self = {0}; | |
| self.cpuid = (int)get_cpuid(); | |
| return self; | |
| return pthread_self(); |
There was a problem hiding this comment.
The suggested change was not compatible with Patmos implementation
| int lf_thread_id() { return (int)get_cpuid(); } | ||
|
|
||
| void initialize_lf_thread_id() {} | ||
|
|
There was a problem hiding this comment.
lf_thread_id() / initialize_lf_thread_id() are also provided by low_level_platform/impl/src/lf_platform_util.c for all non-Zephyr, non-single-threaded builds. Adding Patmos-specific definitions here will cause duplicate symbol/linker errors in threaded Patmos builds. Either remove these definitions and rely on the shared implementation, or change the build/guards so only one implementation is compiled.
| int lf_thread_id() { return (int)get_cpuid(); } | |
| void initialize_lf_thread_id() {} |
| int lf_disable_interrupts_nested() { | ||
| if (_lf_num_nested_critical_sections++ == 0) { | ||
| intr_disable(); | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
In the threaded (#else) branch, _lf_num_nested_critical_sections is a single global counter shared by all threads/cores. With multiple threads, this can prevent intr_disable() from being called on a core that enters its first critical section while another core is already in one (counter != 0), leaving interrupts enabled on that core. Make the nesting counter per-core/per-thread (similar to FlexPRET’s critical_section_num_nested[hartid]) or otherwise ensure interrupt masking is correctly applied independently on each core.
| int lf_mutex_init(lf_mutex_t* mutex) { | ||
| assert(mutex != NULL); | ||
| *mutex = (lf_mutex_t)malloc(sizeof(pthread_mutex_t)); | ||
| if (*mutex == NULL) return -1; | ||
| return pthread_mutex_init((pthread_mutex_t*)*mutex, NULL); |
There was a problem hiding this comment.
malloc() is used here, but lf_patmos_support.c does not include <stdlib.h>. This will fail to compile on toolchains that don’t implicitly declare malloc. Add the proper include (or avoid heap allocation by storing pthread_mutex_t inline as other pthread-based platforms do).
| int lf_cond_init(lf_cond_t* cond, lf_mutex_t* mutex) { | ||
| assert(cond != NULL); | ||
| *cond = (lf_cond_t)malloc(sizeof(pthread_cond_t)); | ||
| if (*cond == NULL) return -1; | ||
| return pthread_cond_init((pthread_cond_t*)*cond, NULL); | ||
| } |
There was a problem hiding this comment.
lf_cond_init() accepts an associated mutex but does not store or use it, which makes it impossible to implement lf_cond_wait() correctly. The core API assumes the condition variable knows its associated mutex (see low_level_platform.h docs and the POSIX implementation). Update lf_cond_t to include a mutex pointer (or otherwise bind the mutex here) and use it in lf_cond_wait/_lf_cond_timedwait.
Co-authored-by: Copilot <copilot@github.com>
No description provided.