Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 merged PR #16194: URL: https://github.com/apache/nuttx/pull/16194 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2857383471 OK, thanks for the detailed explanation, ti's clear to me that we need at least one holder for binary semaphore. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2857114818 As this seems to be difficult subject, I'll try to explain how the priority boost and restore works here, and why the thread/TCB needs to keep a list of priority boosts. It is not really *that* complicated. A thread (T1) may hold several mutexes at (Mx) the time (it can first wait on M1 then on M2, then on M3). While it is holding those, other higher priority threads (T2, T3) may boost the priority of T1 many times by blocking on mutexes M2, M3), . When the T1 posts the M3, it needs to find the correct priority to restore to. This information is *not* necessarily in M3, the correct restore priority may need to be determined by M1 or M2. This logic is (correctly) implemented in https://github.com/apache/nuttx/blob/358469e5bbb5e950c8c0b563c761213ececd/sched/semaphore/sem_holder.c#L413 So whenever a priority boost happens, the semaphore must be added to the current mutex holder's list in order to do proper priority restoration. Here is one step by step example to make it even more simple: Assume we have 3 threads: T1, base priority 1 T2, base priority 2 T3, base priority 3 and 2 mutexes: M1 and M2. I am marking "wait on M1" as W(M1) and "post M1" as P(M1). When a thread 1 is running at priority x, I am marking it as T1(x), similarly for T2 and T3 1) T1(1) is running and does W(M1) , W(M2) 2) T2(2) gets to run, and does W(M1). It gets blocked and boosts T1 to prio 2 3) T1(2) continues running 4) T3(3) gets to run and does W(M2). It gets blocked and boosts T1 to prio 3 5) T1(3) continues running and does P(M2). Priority of T1 is restored to *2* and not its base priority *since T1 is still holding M1, and T2 is blocked on that* 6) T3(3) continues running until it is done and blocks on something else 7) T1(2) continues running and posts M1. Priority of T1 is restored to 1 8) T2(2) continues running Now, If you don't add the semaphore to the task's tcb->holdsem list, you go wrong on 5) and restore the T1 to it's base priority, causing priority inversion (blocking T2). Note that at 5), there is no information on M2 about what is the correct priority to restore to. The previous boosts *must* be tracked in the tcb, and this is done by adding the semaphore holder information to the tcb->holdsem list. I hope this clarifies the subject? Again, I am not re-writing any priority inheritance code (which is IMHO working correctly) in this PR. And I explained already earlier why you *always* need to add the holder information to the TCB:s list when the priority boost occurs, but how the semaphores own holder list is redundant for mutexes. Please don't mix up these two things. I am *not* tracking here if the mutex is being held by many threads, this is obviously not happening. But a thread may hold several mutexes! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2856829299 > > But POSIX never define the priority inheritance and priority ceiling for counting semaphore. > > As I said, the logic is there to support necessary priority inheritance in a realtime system. It is irrelevant that POSIX does not specify the internal behavior; that is purely up to the implementer of the OS. > The priority inheritance support still here for binary semaphore(mutex), but the implementation is optimized more fast. > If there are bugs, that is a difference issue: Priority inheritance must be retained and error must be fixed. Removing the necessary solution is not the correct way to go. The discussion is removing the priority inheritance support from the counting semaphore, since there is no well defined behavior for counting semaphore. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2076652731 ## sched/semaphore/sem_post.c: ## @@ -116,7 +153,10 @@ int nxsem_post_slow(FAR sem_t *sem) * initialized if the semaphore is to used for signaling purposes. */ - nxsem_release_holder(sem); + if (!mutex || blocking) Review Comment: > Can you please explain what you mean. You need to add the holder to tcb->holdsem list for the priority inheritance / restoration. > No, we don't need add a new field like `holdsem`, we can restore holder tcb from `sem->u.mholder`. > I am definitely not going to remove those. A thread can hold several mutexes and priority can be boosted many times. To restore the priority to correct level you need to have the semaphores (holder structures) added to the tcb:s list (holdsem list). Yes, but all priority adjust trigger happen when someone operate on semaphore. At this case, we can restore the holder tcb from `sem->u.mholder`. with the new implementation: 1. Binary semaphore(mutex) already record the only holder in `u.mholder` field, we don't need add it into `holder/hhead` again 2. Adjust nxsem_boost_priority/nxsem_restore_baseprio to consider the special holder in `u.mholder` for mutex 3. Change the default value of SEM_PREALLOCHOLDERS to zero These suggestion could satisfy: 1. Binary semaphore work without SEM_PREALLOCHOLDERS 2. Support the multiple holders for counting semaphore -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2076136934 ## sched/semaphore/sem_post.c: ## @@ -116,7 +153,10 @@ int nxsem_post_slow(FAR sem_t *sem) * initialized if the semaphore is to used for signaling purposes. */ - nxsem_release_holder(sem); + if (!mutex || blocking) Review Comment: Can you please explain what you mean. You need to add the holder to tcb->holdsem list for the priority inheritance / restoration. I am definitely not going to remove those. A thread can hold several mutexes and priority can be boosted many times. To restore the priority to correct level you need to have the semaphores (holder structures) added to the tcb:s list (holdsem list). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2076136934 ## sched/semaphore/sem_post.c: ## @@ -116,7 +153,10 @@ int nxsem_post_slow(FAR sem_t *sem) * initialized if the semaphore is to used for signaling purposes. */ - nxsem_release_holder(sem); + if (!mutex || blocking) Review Comment: Can you please explain what you mean. You need to add the holder to tcb->holdsem list for the priority inheritance / restoration. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2076132252 ## sched/semaphore/sem_wait.c: ## @@ -126,6 +171,13 @@ int nxsem_wait_slow(FAR sem_t *sem) rtcb->waitobj = sem; + /* In case of a mutex, store the previous holder in the task's list */ + + if (mutex) Review Comment: what??? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2076134668 ## sched/semaphore/sem_post.c: ## @@ -116,7 +153,10 @@ int nxsem_post_slow(FAR sem_t *sem) * initialized if the semaphore is to used for signaling purposes. */ - nxsem_release_holder(sem); + if (!mutex || blocking) Review Comment: That's wrong -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2076132252 ## sched/semaphore/sem_wait.c: ## @@ -126,6 +171,13 @@ int nxsem_wait_slow(FAR sem_t *sem) rtcb->waitobj = sem; + /* In case of a mutex, store the previous holder in the task's list */ + + if (mutex) Review Comment: what? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2076022628 ## sched/semaphore/sem_post.c: ## @@ -116,7 +153,10 @@ int nxsem_post_slow(FAR sem_t *sem) * initialized if the semaphore is to used for signaling purposes. */ - nxsem_release_holder(sem); + if (!mutex || blocking) Review Comment: ```suggestion if (!mutex) ``` ## sched/semaphore/sem_wait.c: ## @@ -126,6 +171,13 @@ int nxsem_wait_slow(FAR sem_t *sem) rtcb->waitobj = sem; + /* In case of a mutex, store the previous holder in the task's list */ + + if (mutex) Review Comment: let's remove ALL holder related function call in the mutex case -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
patacongo commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2854789114 > But POSIX never define the priority inheritance and priority ceiling for counting semaphore. As I said, the logic is there to support necessary priority inheritance in a realtime system. It is irrelevant that POSIX does not specify the internal behavior; that is purely up to the implementer of the OS. If there are bugs, that is a difference issue: Priority inheritance must be retained and error must be fixed. Removing the necessary solution is not the correct way to go. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2854788694 > > 2. Remove the code which track multiple holder for counting semaphore since it never work as expect and impact the real time > > "never work as expect" _might_ not be correct. I do agree that in most common use case for a counting semaphore is signalling, and it is hard to find real world use cases where you would want the priority inheritance on counting semaphores. > > Perhaps it could also be used as a resource counter where same thread waits and posts on a counting semaphore. Or a case where you want to allow exactly n number of threads execute the code. > semaphore use as the resource counter also can't work with the holder concept too, since the resource acquire and release still happen in the different threads in most case. > Or, for some reason you want to use a counting semaphore as a mutex... > In this case, the semaphore should be a binary semaphore. Even you need the recessive mutex, you still need binary semaphore, not counting semaphore. > I'd like to see proof that it is really useless before doing anything as drastic as removing it completely, even if it would simplify many things. > > Perhaps a better discussion point is, whether the priority inheritance should be enabled by default on counting semaphores or not? To make the holder tracking work as expect, the key requirement is that sem_wait and sem_post is called in the same thread. using semaphore as event notification or resource counting can't match this requirement. Also, I never hear any spec which define the priority inheritance for counting semaphore. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
pussuw commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2854759698 > Counting semaphore is normally waited(event) in one thread, but posted(event) in another thread(or even interrupt context) yes in this case using priority inheritance is totally wrong. For signaling semaphores it will just not work. But nothing prevents multiple threads taking a count on a counting semaphore that is *not* used for signaling, but is used e.g. as a resource counter. In this case if a higher priority thread cannot get the resource, you need to boost. If another even higher priority thread comes along, you need to boost again. The use case for this is not obvious to me, so this problem could be superficial. I don't know of examples of this use, maybe there is some driver in the NuttX kernel that uses this ? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2854746686 > 2. Remove the code which track multiple holder for counting semaphore since it never work as expect and impact the real time "never work as expect" *might* not be correct. I do agree that in most common use case for a counting semaphore is signalling, and it is hard to find real world use cases where you would want the priority inheritance on counting semaphores. Perhaps it could also be used as a resource counter where same thread waits and posts on a counting semaphore. Or a case where you want to allow exactly n number of threads execute the code. Or, for some reason you want to use a counting semaphore as a mutex... I'd like to see proof that it is really useless before doing anything as drastic as removing it completely, even if it would simplify many things. Perhaps a better discussion point is, whether the priority inheritance should be enabled by default on counting semaphores or not? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2075559102
##
sched/semaphore/sem_post.c:
##
@@ -85,19 +87,54 @@ int nxsem_post_slow(FAR sem_t *sem)
flags = enter_critical_section();
- /* Check the maximum allowable value */
+ if (mutex)
+{
+ /* Mutex post from interrupt context is not allowed */
+
+ DEBUGASSERT(!up_interrupt_context());
+
+ /* Lock the mutex for us by setting the blocking bit */
+
+ mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKING_BIT);
+
+ /* Mutex post from another thread is not allowed, unless
+ * called from nxsem_reset
+ */
+
+ DEBUGASSERT(mholder == (NXSEM_MBLOCKING_BIT | NXSEM_MRESET) ||
+ (mholder & (~NXSEM_MBLOCKING_BIT)) == nxsched_gettid());
+
+ blocking = NXSEM_MBLOCKING(mholder);
- sem_count = atomic_read(NXSEM_COUNT(sem));
- do
+ if (!blocking)
Review Comment:
No, it is correct. It sets regardless of being reset or not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2854659321 > > @patacongo could you give some comment about this problem? > > If this feature is removed, you will re-introduce priority inheritance issues and damage realtime performance. That is why that code is there in the first place. > Only the multiple holders is removed, the single holder still track and adjust the priority as needed. Here is the reason why the multiple holders track is wrong and should be removed: Counting semaphore is normally waited(event) in one thread, but posted(event) in another thread(or even interrupt context), the holder doesn't make any sense and totally wrong in this case. The holder concept only make sense when the sem_wait and sem_post is called in the same thread(binary semaphore case). > POSIX does not specify the implementation of a realtime system and, hence, does not address these implementation issues. > > POSIX permits implementations and features not addressed by POSIX. POSIX only addresses minimum funtions and only for application interfaces. It is completely permissible to support additional functionality. POSIX places no requirements on the internal implementation of an OS; only on the external, application interface provided by the OS. > No, POSIX does define the priority inheritance and priority ceiling in the spec: https://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_mutexattr_setprotocol.html both are implemented in NuttX now for pthread_mutex/mutex(binary semaphore). POSIX never define the priority inheritance and priority ceiling for counting semaphore. > Do not remove this functionality it is critical for the correct operation of the OS and would cause the loss of realtime performance. Let's summary that the suggestion: 1. Keep the binary semaphore priority inheritance and priority ceiling feature by tracking one holder 2. Remove the code which track multiple holder for counting semaphore since it never work as expect and impact the real time -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
patacongo commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2854544906 > @patacongo could you give some comment about this problem? If this feature is removed, you will re-introduce priority inheritance issues and damage realtime performance. That is why that code is there in the first place. POSIX does not specify the implementation of a realtime system and, hence, does not address these implementation issues. POSIX permits implementations and features not addressed by POSIX. POSIX only addresses minimum funtions and only for application interfaces. It is completely permissible to support additional functionality. POSIX places no requirements on the internal implementation of an OS; only on the external, application interface provided by the OS. Do not remove this functionality it is critical for the correct operation of the OS and would cause the loss of realtime performance. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2075344025
##
sched/semaphore/sem_waitirq.c:
##
@@ -72,31 +72,54 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
{
FAR struct tcb_s *rtcb = this_task();
FAR sem_t *sem = wtcb->waitobj;
+ bool mutex = NXSEM_IS_MUTEX(sem);
/* It is possible that an interrupt/context switch beat us to the punch
* and already changed the task's state.
*/
- DEBUGASSERT(sem != NULL && atomic_read(NXSEM_COUNT(sem)) < 0);
+ DEBUGASSERT(sem != NULL);
+ DEBUGASSERT((mutex || atomic_read(NXSEM_COUNT(sem)) < 0) &&
+ (!mutex || NXSEM_MBLOCKING(atomic_read(NXSEM_MHOLDER(sem);
Review Comment:
let's spit to two assert like other similar places, which also make the
assertion trigger condition more clear.
##
sched/semaphore/sem_post.c:
##
@@ -85,19 +87,54 @@ int nxsem_post_slow(FAR sem_t *sem)
flags = enter_critical_section();
- /* Check the maximum allowable value */
+ if (mutex)
+{
+ /* Mutex post from interrupt context is not allowed */
+
+ DEBUGASSERT(!up_interrupt_context());
+
+ /* Lock the mutex for us by setting the blocking bit */
+
+ mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKING_BIT);
+
+ /* Mutex post from another thread is not allowed, unless
+ * called from nxsem_reset
+ */
+
+ DEBUGASSERT(mholder == (NXSEM_MBLOCKING_BIT | NXSEM_MRESET) ||
+ (mholder & (~NXSEM_MBLOCKING_BIT)) == nxsched_gettid());
+
+ blocking = NXSEM_MBLOCKING(mholder);
- sem_count = atomic_read(NXSEM_COUNT(sem));
- do
+ if (!blocking)
Review Comment:
```
if (!blocking && mholder != NXSEM_MRESET)
{
atomic_set(NXSEM_MHOLDER(sem), NXSEM_NO_MHOLDER);
}
```
##
include/nuttx/mutex.h:
##
@@ -354,407 +384,499 @@ void nxmutex_reset(FAR mutex_t *mutex);
*
/
-int nxmutex_breaklock(FAR mutex_t *mutex, FAR unsigned int *locked);
+int nxrmutex_breaklock(FAR rmutex_t *rmutex, FAR unsigned int *count);
/
- * Name: nxmutex_restorelock
+ * Name: nxrmutex_restorelock
*
* Description:
- * This function attempts to restore the mutex.
+ * This function attempts to restore the recursive mutex.
*
* Parameters:
- * mutex - mutex descriptor.
- * locked - true: it's mean that the mutex is broke success
+ * rmutex - Recursive mutex descriptor.
*
* Return Value:
* This is an internal OS interface and should not be used by applications.
* It follows the NuttX internal error return policy: Zero (OK) is
- * returned on success. A negated errno value is returned on failure
+ * returned on success. A negated errno value is returned on failure.
+ * Possible returned errors:
*
/
-int nxmutex_restorelock(FAR mutex_t *mutex, unsigned int locked);
+int nxrmutex_restorelock(FAR rmutex_t *rmutex, unsigned int count);
+
+#define nxrmutex_set_protocol(rmutex, protocol) \
+nxmutex_set_protocol(&(rmutex)->mutex, protocol)
+#define nxrmutex_getprioceiling(rmutex, prioceiling) \
+nxmutex_getprioceiling(&(rmutex)->mutex, prioceiling)
+#define nxrmutex_setprioceiling(rmutex, prioceiling, old_ceiling) \
+nxmutex_setprioceiling(&(rmutex)->mutex, prioceiling, old_ceiling)
+
+/
+ * Inline functions
+ /
/
- * Name: nxmutex_set_protocol
+ * Name: nxmutex_get_holder
*
* Description:
- * This function attempts to set the priority protocol of a mutex.
+ * This function get the holder of the mutex referenced by 'mutex'.
+ * Note that this is inherently racy unless the calling thread is
+ * holding the mutex.
*
* Parameters:
- * mutex- mutex descriptor.
- * protocol - mutex protocol value to set.
+ * mutex - mutex descriptor.
*
* Return Value:
- * This is an internal OS interface and should not be used by applications.
- * It follows the NuttX internal error return policy: Zero (OK) is
- * returned on success. A negated errno value is returned on failure
*
/
-int nxmutex_set_protocol(FAR mutex_t *mutex, int protocol);
+static inline_function pid_t nxmutex_get_holder(FAR mutex_t *mutex)
+{
+ uint32_t mholder = mutex->sem.val.mholder & ~NXSEM_MBLOCKING_BIT;
+ return NXSEM_MACQUIRED(mholder) ? (pid_t)mholder : -1;
+}
/
- * Name: nxmutex_g
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2854058081 > > It doesn't make sense to boost the thread which pass nxsem_wait on a counting semaphore, since the priority boost is meaningful only for a mutex(binary semaphore). That's why I suggest that we can drop the holder of counting semaphores directly. > > I don't believe this is 100% true; currently the priority inheritance is supported also on counting semaphores. I also believe that if you really need to protect from priority inversion you'll need to have priority inheritance also on counting semaphores... Counting semaphore is normally waited(event) in one thread, but posted(event) in another thread(or even interrupt context), the holder doesn't make any sense and totally wrong in this case. The holder concept only make sense when the sem_wait and sem_post is called in the same thread(binary semaphore case). @patacongo could you give some comment about this problem? > Removing that would need quite strong agreement from the community that it is not really needed! > Yes, let's vote on the dev list. > Since the support is there, I'd hesitate removing that, and leaving it to the users to decide whether they want to disable priority inheritance on a counting semaphore or not. Yes, it should be belong another PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2075158766
##
sched/semaphore/sem_waitirq.c:
##
@@ -72,31 +72,55 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
{
FAR struct tcb_s *rtcb = this_task();
FAR sem_t *sem = wtcb->waitobj;
+ bool mutex = NXSEM_IS_MUTEX(sem);
/* It is possible that an interrupt/context switch beat us to the punch
* and already changed the task's state.
*/
- DEBUGASSERT(sem != NULL && atomic_read(NXSEM_COUNT(sem)) < 0);
+ DEBUGASSERT(sem != NULL);
+ DEBUGASSERT(
Review Comment:
sorry, my previous comment is wrong.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2074862627
##
libs/libc/semaphore/sem_getvalue.c:
##
@@ -65,7 +69,23 @@ int nxsem_get_value(FAR sem_t *sem, FAR int *sval)
{
if (sem != NULL && sval != NULL)
{
- *sval = atomic_read(NXSEM_COUNT(sem));
+ if (NXSEM_IS_MUTEX(sem))
+{
+ uint32_t mholder = atomic_read(NXSEM_MHOLDER(sem));
+ if (NXSEM_MACQUIRED(mholder))
+{
+ *sval = NXSEM_MBLOCKS(mholder) ? -1 : 0;
Review Comment:
I see, this went to wrong commit, fixed
##
sched/semaphore/sem_destroy.c:
##
@@ -61,6 +61,9 @@
int nxsem_destroy(FAR sem_t *sem)
{
int32_t old;
+ bool mutex = NXSEM_IS_MUTEX(sem);
+ FAR atomic_t *const val = mutex ? NXSEM_MHOLDER(sem) : NXSEM_COUNT(sem);
Review Comment:
removed const
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2074866117
##
libs/libc/semaphore/sem_getvalue.c:
##
@@ -67,25 +63,9 @@
int nxsem_get_value(FAR sem_t *sem, FAR int *sval)
{
- if (sem != NULL && sval != NULL)
+ if (sem != NULL && sval != NULL && !NXSEM_IS_MUTEX(sem))
{
- if (NXSEM_IS_MUTEX(sem))
Review Comment:
hm I guess this comment was about if defined -> ifdef for sem_trywait ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2074856652
##
include/nuttx/mutex.h:
##
@@ -354,407 +384,497 @@ void nxmutex_reset(FAR mutex_t *mutex);
*
/
-int nxmutex_breaklock(FAR mutex_t *mutex, FAR unsigned int *locked);
+int nxrmutex_breaklock(FAR rmutex_t *rmutex, FAR unsigned int *count);
/
- * Name: nxmutex_restorelock
+ * Name: nxrmutex_restorelock
*
* Description:
- * This function attempts to restore the mutex.
+ * This function attempts to restore the recursive mutex.
*
* Parameters:
- * mutex - mutex descriptor.
- * locked - true: it's mean that the mutex is broke success
+ * rmutex - Recursive mutex descriptor.
*
* Return Value:
* This is an internal OS interface and should not be used by applications.
* It follows the NuttX internal error return policy: Zero (OK) is
- * returned on success. A negated errno value is returned on failure
+ * returned on success. A negated errno value is returned on failure.
+ * Possible returned errors:
*
/
-int nxmutex_restorelock(FAR mutex_t *mutex, unsigned int locked);
+int nxrmutex_restorelock(FAR rmutex_t *rmutex, unsigned int count);
+
+#define nxrmutex_set_protocol(rmutex, protocol) \
+nxmutex_set_protocol(&(rmutex)->mutex, protocol)
+#define nxrmutex_getprioceiling(rmutex, prioceiling) \
+nxmutex_getprioceiling(&(rmutex)->mutex, prioceiling)
+#define nxrmutex_setprioceiling(rmutex, prioceiling, old_ceiling) \
+nxmutex_setprioceiling(&(rmutex)->mutex, prioceiling, old_ceiling)
+
+/
+ * Inline functions
+ /
/
- * Name: nxmutex_set_protocol
+ * Name: nxmutex_get_holder
*
* Description:
- * This function attempts to set the priority protocol of a mutex.
+ * This function get the holder of the mutex referenced by 'mutex'.
+ * Note that this is inherently racy unless the calling thread is
+ * holding the mutex.
*
* Parameters:
- * mutex- mutex descriptor.
- * protocol - mutex protocol value to set.
+ * mutex - mutex descriptor.
*
* Return Value:
- * This is an internal OS interface and should not be used by applications.
- * It follows the NuttX internal error return policy: Zero (OK) is
- * returned on success. A negated errno value is returned on failure
*
/
-int nxmutex_set_protocol(FAR mutex_t *mutex, int protocol);
+static inline pid_t nxmutex_get_holder(FAR mutex_t *mutex)
+{
+ uint32_t mholder = mutex->sem.val.mholder & ~NXSEM_MBLOCKING_BIT;
Review Comment:
This is not necessary, and is not even possible. Including nuttx/atomic.h in
mutex.h will lead to horrible include dependency problems for some platforms.
I'd rather remove this function, together with nxmutex_is_locked in the
future; there is no reason to have this kind of interfaces for mutex...
##
include/nuttx/mutex.h:
##
@@ -354,407 +384,497 @@ void nxmutex_reset(FAR mutex_t *mutex);
*
/
-int nxmutex_breaklock(FAR mutex_t *mutex, FAR unsigned int *locked);
+int nxrmutex_breaklock(FAR rmutex_t *rmutex, FAR unsigned int *count);
/
- * Name: nxmutex_restorelock
+ * Name: nxrmutex_restorelock
*
* Description:
- * This function attempts to restore the mutex.
+ * This function attempts to restore the recursive mutex.
*
* Parameters:
- * mutex - mutex descriptor.
- * locked - true: it's mean that the mutex is broke success
+ * rmutex - Recursive mutex descriptor.
*
* Return Value:
* This is an internal OS interface and should not be used by applications.
* It follows the NuttX internal error return policy: Zero (OK) is
- * returned on success. A negated errno value is returned on failure
+ * returned on success. A negated errno value is returned on failure.
+ * Possible returned errors:
*
/
-int nxmutex_restorelock(FAR mutex_t *mutex, unsigned int locked);
+int nxrmutex_restorelock(FAR rmutex_t *rmutex, unsigned int count);
+
+#define nxrmutex_set_protocol(rmutex, protocol) \
+nxmutex_set_protocol(&(rmutex)->mutex, protocol)
+#define nxrmutex_getprioceiling(rmutex, prioceiling) \
+nxmutex_getprioceiling(&(rmutex)->mutex, prioceiling)
+#defi
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2074827578
##
sched/semaphore/sem_waitirq.c:
##
@@ -72,31 +72,55 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
{
FAR struct tcb_s *rtcb = this_task();
FAR sem_t *sem = wtcb->waitobj;
+ bool mutex = NXSEM_IS_MUTEX(sem);
/* It is possible that an interrupt/context switch beat us to the punch
* and already changed the task's state.
*/
- DEBUGASSERT(sem != NULL && atomic_read(NXSEM_COUNT(sem)) < 0);
+ DEBUGASSERT(sem != NULL);
+ DEBUGASSERT(
Review Comment:
Hm, you previously wanted this the other way around (using the positive
logic...). But ok, reverting the logic to original.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2074821911
##
sched/semaphore/sem_trywait.c:
##
@@ -63,8 +63,11 @@
int nxsem_trywait_slow(FAR sem_t *sem)
{
irqstate_t flags;
- int32_t semcount;
- int ret;
+ int ret = -EAGAIN;
+ bool mutex = NXSEM_IS_MUTEX(sem);
+ FAR atomic_t *const val = mutex ? NXSEM_MHOLDER(sem) : NXSEM_COUNT(sem);
Review Comment:
same comment on const as above. I'll just remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2074810210
##
sched/semaphore/sem_destroy.c:
##
@@ -61,6 +61,9 @@
int nxsem_destroy(FAR sem_t *sem)
{
int32_t old;
+ bool mutex = NXSEM_IS_MUTEX(sem);
+ FAR atomic_t *const val = mutex ? NXSEM_MHOLDER(sem) : NXSEM_COUNT(sem);
Review Comment:
not right, this reads "val is a constant pointer to FAR atomic_t". It is not
"FAR pointer to constant val". Const is there just to note that the pointer is
not changing in this scope. If you dislike "const" as a word, I can surely
remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2853411481 > It doesn't make sense to boost the thread which pass nxsem_wait on a counting semaphore, since the priority boost is meaningful only for a mutex(binary semaphore). That's why I suggest that we can drop the holder of counting semaphores directly. > I don't believe this is 100% true; currently the priority inheritance is supported also on counting semaphores. I also believe that if you really need to protect from priority inversion you'll need to have priority inheritance also on counting semaphores... Removing that would need quite strong agreement from the community that it is not really needed! Since the support is there, I'd hesitate removing that, and leaving it to the users to decide whether they want to disable priority inheritance on a counting semaphore or not. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2853378798 > > @jlaitine after this patchset get merged, we can simplify the holder logic for priority inheritance since: > > > > 1. Don't track the multiple holders since mutex always can have one and only one holder > > 2. But need track multiple thread which is blocking/waiting on the mutex with the local variable > > > > Do you think so? > > Yes, cleanup is definitely welcome, but I didn't quite understand yet the idea of the local variable, perhaps you can elaborate a bit more on that? > > My thinking is as follows: > > 1. When a thread comes a holder, it needs to set it to the semaphore; in case of mutex it is just the PID to the val.mholder (the single holder) or adding the holder structure to the semaphore's list in case of counting semaphores. > It doesn't make sense to boost the thread which pass nxsem_wait on a counting semaphore, since the priority boost is meaningful only for a mutex(binary semaphore). That's why I suggest that we can drop the holder of counting semaphores directly. > 2. When a thread blocks on the mutex or counting semaphore, i.e. when the priority boost happens, it needs to add the semaphore to the tcb list of semaphores/mutexes (the "tcb->holdsem" list). All data struct is already defined: - `tcb->waitobj` point to the semaphore - `sem->waitlist` point to all blocking threads - sem->u.mholder point to the holding thread we can implement the priority boost with the above info. > A thread can be a holder of multiple counting semaphores and mutexes, and any of those may have caused priority boost; so this list is always needed for proper priority restoration. > As I said before, we don't need boost the holder of counting semaphore, so all stuff related semholder_s could be removed, recording mholder is enough. > > These two a bit different things (list of sem holders and tracking priority boosts) are now mixed togheter in nxsem_add_holder_tcb/nxsem_allocholder, which just allocates the holder and puts it to both lists, even though for mutexes adding the structure to the semaphores own holder list is redundant, and they don't need to be added at the same time. > > Because 2. is always needed, I found in easier to just use all the existing code and semholder structure for adding the holder and using the priority inheritance code as is, even though it adds the same structure also to the semaphore's list. It doesn't really matter if it is added also to the semaphore's list; you don't save anything by removing it (the holder list pointer is there allocated in sem_s anyway as long as the structure is the same for a mutex and a counting semaphore). > We can drop the support of item 2, which could simplify the code a lot. > But, for the further cleanup / optimization possibilities, these two could be separated. For example making own functions for "nxsem_add_holder_tcb" to add it to tcb->holdsem and "nxsem_add_counting_sem_holder" to add it to semaphores own list. > > Does this make sense? > > Another thing for optimizations, there is one very small and beneficial optimization that we could add; for counting semaphores the "fast path" in libc can be also enabled when the priority inheritance is disabled on a counting semaphore (basically in that case the libc fast functions can just increase/decrease the counter as long as it doesn't block or wake up another threads). This would be beneficial for performance especially in queue implementations (e.g. multiple-writer - single reader, where writers post a signalling semaphore and reader sleeps on the semaphore waiting for the writers). I have code for this ready, if you are interested after this PR. Yes, it's the next step to optimize the counting semaphore usage. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2853322370 > @jlaitine after this patchset get merged, we can simplify the holder logic for priority inheritance since: > > 1. Don't track the multiple holders since mutex always can have one and only one holder > 2. But need track multiple thread which is blocking/waiting on the mutex with the local variable > > Do you think so? Yes, cleanup is definitely welcome, but I didn't quite understand yet the idea of the local variable, perhaps you can elaborate a bit more on that? My thinking is as follows: 1. When a thread comes a holder, it needs to set it to the semaphore; in case of mutex it is just the PID to the val.mholder (the single holder) or adding the holder structure to the semaphore's list in case of counting semaphores. 2. When a thread blocks on the mutex or counting semaphore, i.e. when the priority boost happens, it needs to add the semaphore to the tcb list of semaphores/mutexes (the "tcb->holdsem" list). A thread can be a holder of multiple counting semaphores and mutexes, and any of those may have caused priority boost; so this list is always needed for proper priority restoration. These two a bit different things (list of sem holders and tracking priority boosts) are now mixed togheter in nxsem_add_holder_tcb/nxsem_allocholder, which just allocates the holder and puts it to both lists, even though for mutexes adding the structure to the semaphores own holder list is redundant, and they don't need to be added at the same time. Because 2. is always needed, I found in easier to just use all the existing code and semholder structure for adding the holder and using the priority inheritance code as is, even though it adds the same structure also to the semaphore's list. It doesn't really matter if it is added also to the semaphore's list; you don't save anything by removing it (the holder list pointer is there allocated in sem_s anyway as long as the structure is the same for a mutex and a counting semaphore). But, for the further cleanup / optimization possibilities, these two could be separated. For example making own functions for "nxsem_add_holder_tcb" to add it to tcb->holdsem and "nxsem_add_counting_sem_holder" to add it to semaphores own list. Does this make sense? Another thing for optimizations, there is one very small and beneficial optimization that we could add; for counting semaphores the "fast path" in libc can be also enabled when the priority inheritance is disabled on the semaphore (basically in that case the libc fast functions can just increase/decrease the counter as long as it doesn't block or wake up another threads). This would be beneficial for performance especially in queue implementations (e.g. multiple-writer - single reader, where writers post a signalling semaphore and reader sleeps on the semaphore waiting for the writers). I have code for this ready, if you are interested after this PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2851402290 @jlaitine after this patchset get merged, we can simplify the holder logic for priority inheritance since: 1. Don't track the multiple holders since mutex always can have one and only one holder 2. But need track multiple thread which is blocking/waiting on the mutex with the local variable Do you think so? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2073187368
##
libs/libc/semaphore/sem_getvalue.c:
##
@@ -65,7 +69,23 @@ int nxsem_get_value(FAR sem_t *sem, FAR int *sval)
{
if (sem != NULL && sval != NULL)
{
- *sval = atomic_read(NXSEM_COUNT(sem));
+ if (NXSEM_IS_MUTEX(sem))
+{
+ uint32_t mholder = atomic_read(NXSEM_MHOLDER(sem));
+ if (NXSEM_MACQUIRED(mholder))
+{
+ *sval = NXSEM_MBLOCKS(mholder) ? -1 : 0;
Review Comment:
NXSEM_MBLOCKS->NXSEM_MBLOCKING
##
sched/semaphore/sem_post.c:
##
@@ -156,7 +195,17 @@ int nxsem_post_slow(FAR sem_t *sem)
* it is awakened.
*/
- nxsem_add_holder_tcb(stcb, sem);
+ if (mutex)
+{
+ uint32_t blocking = dq_empty(SEM_WAITLIST(sem)) ?
Review Comment:
```
uint32_t blocking_bit = dq_empty(SEM_WAITLIST(sem)) ?
0 : NXSEM_MBLOCKING_BIT;
```
##
sched/semaphore/sem_recover.c:
##
@@ -104,7 +105,23 @@ void nxsem_recover(FAR struct tcb_s *tcb)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (NXSEM_IS_MUTEX(sem))
+{
+ /* Clear the blocking bit, if not blocked any more */
+
+ if (dq_empty(SEM_WAITLIST(sem)))
+{
+ uint32_t mholder =
+atomic_fetch_and(NXSEM_MHOLDER(sem), ~NXSEM_MBLOCKING_BIT);
+ DEBUGASSERT(NXSEM_MBLOCKING(mholder));
+}
+}
+ else
+{
+ DEBUGASSERT(atomic_read(NXSEM_COUNT(sem)) < 0);
+
Review Comment:
remove the blank line
##
sched/semaphore/sem_waitirq.c:
##
@@ -72,31 +72,55 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
{
FAR struct tcb_s *rtcb = this_task();
FAR sem_t *sem = wtcb->waitobj;
+ bool mutex = NXSEM_IS_MUTEX(sem);
/* It is possible that an interrupt/context switch beat us to the punch
* and already changed the task's state.
*/
- DEBUGASSERT(sem != NULL && atomic_read(NXSEM_COUNT(sem)) < 0);
+ DEBUGASSERT(sem != NULL);
+ DEBUGASSERT(
Review Comment:
```
DEBUGASSERT(NXSEM_IS_MUTEX(sem) || atomic_read(NXSEM_COUNT(sem)) <= 0);
DEBUGASSERT(!NXSEM_IS_MUTEX(sem) ||
NXSEM_MBLOCKING(atomic_read(NXSEM_MHOLDER(sem;
```
##
libs/libc/semaphore/sem_wait.c:
##
@@ -151,9 +151,9 @@ int nxsem_wait(FAR sem_t *sem)
#ifndef CONFIG_LIBC_ARCH_ATOMIC
- if ((sem->flags & SEM_TYPE_MUTEX)
-# if defined(CONFIG_PRIORITY_PROTECT) || defined(CONFIG_PRIORITY_INHERITANCE)
- && (sem->flags & SEM_PRIO_MASK) == SEM_PRIO_NONE
+ if (NXSEM_IS_MUTEX(sem)
+# if defined(CONFIG_PRIORITY_PROTECT)
Review Comment:
ditto
##
include/nuttx/mutex.h:
##
@@ -354,407 +384,497 @@ void nxmutex_reset(FAR mutex_t *mutex);
*
/
-int nxmutex_breaklock(FAR mutex_t *mutex, FAR unsigned int *locked);
+int nxrmutex_breaklock(FAR rmutex_t *rmutex, FAR unsigned int *count);
/
- * Name: nxmutex_restorelock
+ * Name: nxrmutex_restorelock
*
* Description:
- * This function attempts to restore the mutex.
+ * This function attempts to restore the recursive mutex.
*
* Parameters:
- * mutex - mutex descriptor.
- * locked - true: it's mean that the mutex is broke success
+ * rmutex - Recursive mutex descriptor.
*
* Return Value:
* This is an internal OS interface and should not be used by applications.
* It follows the NuttX internal error return policy: Zero (OK) is
- * returned on success. A negated errno value is returned on failure
+ * returned on success. A negated errno value is returned on failure.
+ * Possible returned errors:
*
/
-int nxmutex_restorelock(FAR mutex_t *mutex, unsigned int locked);
+int nxrmutex_restorelock(FAR rmutex_t *rmutex, unsigned int count);
+
+#define nxrmutex_set_protocol(rmutex, protocol) \
+nxmutex_set_protocol(&(rmutex)->mutex, protocol)
+#define nxrmutex_getprioceiling(rmutex, prioceiling) \
+nxmutex_getprioceiling(&(rmutex)->mutex, prioceiling)
+#define nxrmutex_setprioceiling(rmutex, prioceiling, old_ceiling) \
+nxmutex_setprioceiling(&(rmutex)->mutex, prioceiling, old_ceiling)
+
+/
+ * Inline functions
+ /
/
- * Name: nxmutex_set_protocol
+ * Name: nxmutex_get_holder
*
* Description:
- * This function attempts to set the priority protocol of a mutex.
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2847119201 > > > > since the current code base never add const to the local integer variables even they doesn't change after initialization. > > > > > > > > > Why not? I'll change this if I get a second opinion on other reviewers stating that it is wrong to use const. > > > > > > My suggestion just make the code base consistent with each other. From my point of view, the consistence is more important than personal preference. > > This is insanely stupid, but I disagree and commit to change this. When I find motivation again to work on style changes which have no reason - other than make changes just for changes. Apologies if I offended anyone with the above comment; I am sometimes quick-tempered and I have strong opinions. Didn't mean anything ill. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2846715665 Fixed one style review comment which I missed before ( change of dq_peek() != NULL -> !dq_empty() ) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2071315661
##
sched/semaphore/sem_wait.c:
##
@@ -86,19 +89,62 @@ int nxsem_wait_slow(FAR sem_t *sem)
/* Check if the lock is available */
- if (atomic_fetch_sub(NXSEM_COUNT(sem), 1) > 0)
+ if (mutex)
+{
+ uint32_t mholder;
Review Comment:
I like to keep the "m" as for mutex, like in other parts of the pr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2071315094
##
sched/semaphore/sem_reset.c:
##
@@ -76,38 +81,65 @@ int nxsem_reset(FAR sem_t *sem, int16_t count)
flags = enter_critical_section();
- /* A negative count indicates that the negated number of threads are
- * waiting to take a count from the semaphore. Loop here, handing
- * out counts to any waiting threads.
- */
-
- while (atomic_read(NXSEM_COUNT(sem)) < 0 && count > 0)
+ if (NXSEM_IS_MUTEX(sem))
{
- /* Give out one counting, waking up one of the waiting threads
- * and, perhaps, kicking off a lot of priority inheritance
- * logic (REVISIT).
+ /* Support only resetting mutex by removing one waiter */
+
+ DEBUGASSERT(count == 1);
+
+ /* Post the mutex once with holder value set to RESET | BLOCKS
+ * so we know that it is ok in this case to call the post from
+ * another thread.
*/
- DEBUGVERIFY(nxsem_post(sem));
- count--;
+ atomic_set(NXSEM_MHOLDER(sem),
+ NXSEM_MRESET | NXSEM_MBLOCKS_BIT);
+
+ if (dq_peek(SEM_WAITLIST(sem)) != NULL)
Review Comment:
I missed this one, sorry, will update
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2071312619
##
sched/semaphore/sem_wait.c:
##
@@ -86,19 +89,62 @@ int nxsem_wait_slow(FAR sem_t *sem)
/* Check if the lock is available */
- if (atomic_fetch_sub(NXSEM_COUNT(sem), 1) > 0)
+ if (mutex)
+{
+ uint32_t mholder;
+
+ /* We lock the mutex for us by setting the blocks bit,
+ * this is all that is needed if we block
+ */
+
+ mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKS_BIT);
+ if (NXSEM_MACQUIRED(mholder))
+{
+ /* htcb gets NULL if
+ * - the only holder did exit (without posting first)
+ * - the mutex was reset before
+ * In both cases we simply acquire the mutex, thus recovering
+ * from these situations.
+ */
+
+ htcb = nxsched_get_tcb(mholder & (~NXSEM_MBLOCKS_BIT));
+}
+
+ unlocked = htcb == NULL;
Review Comment:
No this is correct, the unlocked is true if the smaphore is not acquired, or
nxsched_get_tcb returns NULL (which happens in all possible error cases
mentioned in the comment above). The htcb is NULL initially.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2071312619
##
sched/semaphore/sem_wait.c:
##
@@ -86,19 +89,62 @@ int nxsem_wait_slow(FAR sem_t *sem)
/* Check if the lock is available */
- if (atomic_fetch_sub(NXSEM_COUNT(sem), 1) > 0)
+ if (mutex)
+{
+ uint32_t mholder;
+
+ /* We lock the mutex for us by setting the blocks bit,
+ * this is all that is needed if we block
+ */
+
+ mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKS_BIT);
+ if (NXSEM_MACQUIRED(mholder))
+{
+ /* htcb gets NULL if
+ * - the only holder did exit (without posting first)
+ * - the mutex was reset before
+ * In both cases we simply acquire the mutex, thus recovering
+ * from these situations.
+ */
+
+ htcb = nxsched_get_tcb(mholder & (~NXSEM_MBLOCKS_BIT));
+}
+
+ unlocked = htcb == NULL;
Review Comment:
No this is correct, the unlocked is true if the smaphore is not acquired, or
nxsched_get_tcb returns NULL (which happens in all possible error cases
mentioned in the comment above).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2071307167
##
sched/semaphore/sem_trywait.c:
##
@@ -74,29 +77,57 @@ int nxsem_trywait_slow(FAR sem_t *sem)
/* If the semaphore is available, give it to the requesting task */
- semcount = atomic_read(NXSEM_COUNT(sem));
+ if (mutex)
+{
+ new = nxsched_gettid();
+}
+
+ old = atomic_read(NXSEM_COUNT(sem));
do
{
- if (semcount <= 0)
+ if (mutex)
{
- leave_critical_section(flags);
- return -EAGAIN;
+ if (NXSEM_MACQUIRED(old))
+{
+ goto out;
+}
+}
+ else
+{
+ if (old <= 0)
+{
+ goto out;
+}
+
+ new = old - 1;
}
}
- while (!atomic_try_cmpxchg_acquire(NXSEM_COUNT(sem),
- &semcount, semcount - 1));
+ while (!atomic_try_cmpxchg_acquire(plock, &old, new));
/* It is, let the task take the semaphore */
ret = nxsem_protect_wait(sem);
if (ret < 0)
{
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (mutex)
+{
+ atomic_set(NXSEM_MHOLDER(sem), NXSEM_NO_MHOLDER);
+}
+ else
+{
+ atomic_fetch_add(NXSEM_COUNT(sem), 1);
+}
+
leave_critical_section(flags);
return ret;
}
- nxsem_add_holder(sem);
+ if (!mutex)
Review Comment:
Sem with priority inheritance can surely be a mutex.
I am not sure I understood the question; but you don't need the holder
structure for mutex if the trywait succeeds; and actually can't add it here.
The holder structure is only needed if you get blocked, in which case it is
added for the current holder. This is the same as in sem_wait.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2071307942
##
sched/semaphore/sem_wait.c:
##
@@ -71,9 +71,12 @@
int nxsem_wait_slow(FAR sem_t *sem)
{
- FAR struct tcb_s *rtcb;
+ FAR struct tcb_s *rtcb = this_task();
irqstate_t flags;
int ret;
+ bool unlocked;
Review Comment:
This is not needed, unlocked is set in the following if...else in both
branches.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2071291015
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
Review Comment:
This is done I believe
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2067898824
##
sched/semaphore/sem_post.c:
##
@@ -73,10 +73,12 @@ int nxsem_post_slow(FAR sem_t *sem)
{
FAR struct tcb_s *stcb = NULL;
irqstate_t flags;
- int32_t sem_count;
#if defined(CONFIG_PRIORITY_INHERITANCE) || defined(CONFIG_PRIORITY_PROTECT)
uint8_t proto;
#endif
+ bool sem_blocks = false;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
+ uint32_t mholder = NXSEM_NO_MHOLDER;
Review Comment:
ok
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2067898000
##
sched/semaphore/sem_destroy.c:
##
@@ -61,6 +61,9 @@
int nxsem_destroy(FAR sem_t *sem)
{
int32_t old;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
+ FAR atomic_t *const plock = mutex ? NXSEM_MHOLDER(sem) : NXSEM_COUNT(sem);
Review Comment:
nuttx code base seldomly add p to pointer type, that's why I suggest to drop
p prefix. Again, I don't have any preference here, all my suggestion is
following the original code style even these doesn't mention in coding style
guide documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2067740893 ## sched/semaphore/sem_post.c: ## @@ -156,7 +195,17 @@ int nxsem_post_slow(FAR sem_t *sem) * it is awakened. */ - nxsem_add_holder_tcb(stcb, sem); + if (mutex) Review Comment: OK. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065980010 ## libs/libc/misc/lib_mutex.c: ## @@ -195,9 +140,11 @@ bool nxmutex_is_hold(FAR mutex_t *mutex) * / -int nxmutex_get_holder(FAR mutex_t *mutex) +pid_t nxmutex_get_holder(FAR mutex_t *mutex) Review Comment: Inlined all the functions which didn't increase image size now. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065979507 ## libs/libc/misc/lib_mutex.c: ## @@ -177,7 +122,7 @@ int nxmutex_destroy(FAR mutex_t *mutex) bool nxmutex_is_hold(FAR mutex_t *mutex) Review Comment: _SCHED_GETTID requires including nuttx/sched.h, and including that in mutex.h causes some havoc. It's fine to have it in C file for now I suppose. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065975368 ## libs/libc/misc/lib_mutex.c: ## @@ -120,7 +98,6 @@ int nxmutex_init(FAR mutex_t *mutex) return ret; } - mutex->holder = NXMUTEX_NO_HOLDER; Review Comment: This adds image size -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065671928
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER ((uint32_t)0x7ffe)
+#define NXSEM_MRESET ((uint32_t)0x7fff)
+
+/* Macro to retrieve mutex's atomic holder's ptr */
+
+#define NXSEM_MHOLDER(s) ((FAR atomic_t *)&(s)->val.mholder)
+
+/* Check if holder value (TID) is not NO_HOLDER or RESET */
+
+#define NXSEM_MACQUIRED(h) (!(((h) & NXSEM_NO_MHOLDER) == NXSEM_NO_MHOLDER))
+
+/* Check if mutex is acquired and blocks some other task */
+
+#define NXSEM_MBLOCKS(h) (((h) & NXSEM_MBLOCKS_BIT) != 0)
Review Comment:
Ok, I am fine with `M`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2837573228 A bunch of style changes just to please some reviewers, no functional change. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r206613
##
sched/semaphore/sem_destroy.c:
##
@@ -61,6 +61,9 @@
int nxsem_destroy(FAR sem_t *sem)
{
int32_t old;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
+ FAR atomic_t *const plock = mutex ? NXSEM_MHOLDER(sem) : NXSEM_COUNT(sem);
Review Comment:
Changed plock (pointer to lock) into val, according to your suggestion. The
suggested re-placement of const is of course wrong. The pointer is const in
this context, the value is not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065541572 ## sched/semaphore/sem_post.c: ## @@ -156,7 +195,17 @@ int nxsem_post_slow(FAR sem_t *sem) * it is awakened. */ - nxsem_add_holder_tcb(stcb, sem); + if (mutex) Review Comment: No. The else path is for counting semaphores which may be posted before anyone has wated on them. This is not true for mutexes. Whoever waited on the mutex has aready added the holder in sem_wait. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065542615
##
sched/semaphore/sem_reset.c:
##
@@ -76,38 +81,65 @@ int nxsem_reset(FAR sem_t *sem, int16_t count)
flags = enter_critical_section();
- /* A negative count indicates that the negated number of threads are
- * waiting to take a count from the semaphore. Loop here, handing
- * out counts to any waiting threads.
- */
-
- while (atomic_read(NXSEM_COUNT(sem)) < 0 && count > 0)
+ if (NXSEM_IS_MUTEX(sem))
{
- /* Give out one counting, waking up one of the waiting threads
- * and, perhaps, kicking off a lot of priority inheritance
- * logic (REVISIT).
+ /* Support only resetting mutex by removing one waiter */
+
+ DEBUGASSERT(count == 1);
+
+ /* Post the mutex once with holder value set to RESET | BLOCKS
+ * so we know that it is ok in this case to call the post from
+ * another thread.
*/
- DEBUGVERIFY(nxsem_post(sem));
- count--;
+ atomic_set(NXSEM_MHOLDER(sem),
+ NXSEM_MRESET | NXSEM_MBLOCKS_BIT);
+
+ if (dq_peek(SEM_WAITLIST(sem)) != NULL)
+{
+ DEBUGVERIFY(nxsem_post(sem));
+}
+ else
+{
+ atomic_set(NXSEM_MHOLDER(sem), NXSEM_MRESET);
+}
}
+ else
+{
+ /* A negative count indicates that the negated number of threads are
+ * waiting to take a count from the semaphore. Loop here, handing
+ * out counts to any waiting threads.
+ */
- /* We exit the above loop with either (1) no threads waiting for the
- * (i.e., with sem->semcount >= 0). In this case, 'count' holds the
- * the new value of the semaphore count. OR (2) with threads still
- * waiting but all of the semaphore counts exhausted: The current
- * value of sem->semcount is already correct in this case.
- */
+ while (atomic_read(NXSEM_COUNT(sem)) < 0 && count > 0)
+{
+ /* Give out one counting, waking up one of the waiting threads
+ * and, perhaps, kicking off a lot of priority inheritance
+ * logic (REVISIT).
+ */
- semcount = atomic_read(NXSEM_COUNT(sem));
- do
-{
- if (semcount < 0)
+ DEBUGVERIFY(nxsem_post(sem));
+ count--;
+}
+
+ /* We exit the above loop with either (1) no threads waiting for the
+ * (i.e., with sem->semcount >= 0). In this case, 'count' holds the
+ * the new value of the semaphore count. OR (2) with threads still
+ * waiting but all of the semaphore counts exhausted: The current
+ * value of sem->semcount is already correct in this case.
+ */
+
+ semcount = atomic_read(NXSEM_COUNT(sem));
Review Comment:
Original code, I won't change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065540627
##
sched/semaphore/sem_post.c:
##
@@ -85,19 +87,54 @@ int nxsem_post_slow(FAR sem_t *sem)
flags = enter_critical_section();
- /* Check the maximum allowable value */
+ if (mutex)
+{
+ /* Mutex post from interrupt context is not allowed */
+
+ DEBUGASSERT(!up_interrupt_context());
+
+ /* Lock the mutex for us by setting the blocking bit */
+
+ mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKS_BIT);
+
+ /* Mutex post from another thread is not allowed, unless
+ * called from nxsem_reset
+ */
+
+ DEBUGASSERT(mholder == (NXSEM_MBLOCKS_BIT | NXSEM_MRESET) ||
+ (mholder & (~NXSEM_MBLOCKS_BIT)) == nxsched_gettid());
+
+ sem_blocks = NXSEM_MBLOCKS(mholder);
- sem_count = atomic_read(NXSEM_COUNT(sem));
- do
+ if (!sem_blocks)
+{
+ if (mholder != NXSEM_MRESET)
+{
+ mholder = NXSEM_NO_MHOLDER;
+}
+
+ atomic_set(NXSEM_MHOLDER(sem), mholder);
+}
+}
+ else
{
- if (sem_count >= SEM_VALUE_MAX)
+ int32_t sem_count;
Review Comment:
That is original code, why change it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065540110
##
sched/semaphore/sem_post.c:
##
@@ -73,10 +73,12 @@ int nxsem_post_slow(FAR sem_t *sem)
{
FAR struct tcb_s *stcb = NULL;
irqstate_t flags;
- int32_t sem_count;
#if defined(CONFIG_PRIORITY_INHERITANCE) || defined(CONFIG_PRIORITY_PROTECT)
uint8_t proto;
#endif
+ bool sem_blocks = false;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
+ uint32_t mholder = NXSEM_NO_MHOLDER;
Review Comment:
I like the m:s, it is a good letter, and points this variable being related
to "mutex"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065539704
##
sched/semaphore/sem_post.c:
##
@@ -85,19 +87,54 @@ int nxsem_post_slow(FAR sem_t *sem)
flags = enter_critical_section();
- /* Check the maximum allowable value */
+ if (mutex)
+{
+ /* Mutex post from interrupt context is not allowed */
+
+ DEBUGASSERT(!up_interrupt_context());
+
+ /* Lock the mutex for us by setting the blocking bit */
+
+ mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKS_BIT);
+
+ /* Mutex post from another thread is not allowed, unless
+ * called from nxsem_reset
+ */
+
+ DEBUGASSERT(mholder == (NXSEM_MBLOCKS_BIT | NXSEM_MRESET) ||
+ (mholder & (~NXSEM_MBLOCKS_BIT)) == nxsched_gettid());
Review Comment:
No, I like the parenthesis. I know the C-precedence, but I like adding
parenthesis because it makes things look clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065538672
##
sched/semaphore/sem_holder.c:
##
@@ -876,7 +876,9 @@ void nxsem_canceled(FAR struct tcb_s *stcb, FAR sem_t *sem)
{
/* Check our assumptions */
- DEBUGASSERT(atomic_read(NXSEM_COUNT(sem)) <= 0);
+ DEBUGASSERT((NXSEM_IS_MUTEX(sem) || atomic_read(NXSEM_COUNT(sem)) <= 0));
+ DEBUGASSERT((!NXSEM_IS_MUTEX(sem) ||
Review Comment:
And this would fail alwys when the semaphore is not a mutex
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2065537698
##
sched/semaphore/sem_holder.c:
##
@@ -876,7 +876,9 @@ void nxsem_canceled(FAR struct tcb_s *stcb, FAR sem_t *sem)
{
/* Check our assumptions */
- DEBUGASSERT(atomic_read(NXSEM_COUNT(sem)) <= 0);
+ DEBUGASSERT((NXSEM_IS_MUTEX(sem) || atomic_read(NXSEM_COUNT(sem)) <= 0));
Review Comment:
This would fail always when the sem is mutex
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2063564754
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER ((uint32_t)0x7ffe)
+#define NXSEM_MRESET ((uint32_t)0x7fff)
+
+/* Macro to retrieve mutex's atomic holder's ptr */
+
+#define NXSEM_MHOLDER(s) ((FAR atomic_t *)&(s)->val.mholder)
+
+/* Check if holder value (TID) is not NO_HOLDER or RESET */
+
+#define NXSEM_MACQUIRED(h) (!(((h) & NXSEM_NO_MHOLDER) == NXSEM_NO_MHOLDER))
+
+/* Check if mutex is acquired and blocks some other task */
+
+#define NXSEM_MBLOCKS(h) (((h) & NXSEM_MBLOCKS_BIT) != 0)
Review Comment:
I guess I would just use NXSEM_MBLOCKING, NXSEM_MBLOCKING_BIT, NXSEM_MRESET
etc. to be consistent and keep the word "Mutex" somehow in the game...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2063569782
##
sched/semaphore/sem_waitirq.c:
##
@@ -72,30 +72,54 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
{
FAR struct tcb_s *rtcb = this_task();
FAR sem_t *sem = wtcb->waitobj;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
/* It is possible that an interrupt/context switch beat us to the punch
* and already changed the task's state.
*/
- DEBUGASSERT(sem != NULL && atomic_read(NXSEM_COUNT(sem)) < 0);
+ DEBUGASSERT(sem != NULL &&
+ (mutex || atomic_read(NXSEM_COUNT(sem)) < 0) &&
+ (!mutex ||
Review Comment:
well, they don't. But I can follow your positive logic and add the missing
parenthesis in your suggestion. Then they equal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2063564754
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER ((uint32_t)0x7ffe)
+#define NXSEM_MRESET ((uint32_t)0x7fff)
+
+/* Macro to retrieve mutex's atomic holder's ptr */
+
+#define NXSEM_MHOLDER(s) ((FAR atomic_t *)&(s)->val.mholder)
+
+/* Check if holder value (TID) is not NO_HOLDER or RESET */
+
+#define NXSEM_MACQUIRED(h) (!(((h) & NXSEM_NO_MHOLDER) == NXSEM_NO_MHOLDER))
+
+/* Check if mutex is acquired and blocks some other task */
+
+#define NXSEM_MBLOCKS(h) (((h) & NXSEM_MBLOCKS_BIT) != 0)
Review Comment:
Of course that would leave the "mutex" out of it, but I guess that's fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2834159189 > > > since the current code base never add const to the local integer variables even they doesn't change after initialization. > > > > > > Why not? I'll change this if I get a second opinion on other reviewers stating that it is wrong to use const. > > My suggestion just make the code base consistent with each other. From my point of view, the consistence is more important than personal preference. This is insanely stupid, but I disagree and commit to change this. When I find motivation again to work on style changes which have no reason - other than make changes just for changes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2834063080 > > since the current code base never add const to the local integer variables even they doesn't change after initialization. > > Why not? I'll change this if I get a second opinion on other reviewers stating that it is wrong to use const. My suggestion just make the code base consistent with each other. From my point of view, the consistence is more important than preference. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2062926746
##
sched/semaphore/sem_waitirq.c:
##
@@ -72,30 +72,54 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
{
FAR struct tcb_s *rtcb = this_task();
FAR sem_t *sem = wtcb->waitobj;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
/* It is possible that an interrupt/context switch beat us to the punch
* and already changed the task's state.
*/
- DEBUGASSERT(sem != NULL && atomic_read(NXSEM_COUNT(sem)) < 0);
+ DEBUGASSERT(sem != NULL &&
+ (mutex || atomic_read(NXSEM_COUNT(sem)) < 0) &&
+ (!mutex ||
Review Comment:
the follow two assert equals:
```
DEBUGASSERT(sem != NULL &&
(mutex || atomic_read(NXSEM_COUNT(sem)) < 0) &&
(!mutex ||
(atomic_read(NXSEM_MHOLDER(sem)) & NXSEM_MBLOCKS_BIT)));
```
```
DEBUGASSERT(sem != NULL &&
(!mutex && atomic_read(NXSEM_COUNT(sem)) < 0) ||
(mutex && (atomic_read(NXSEM_MHOLDER(sem)) &
NXSEM_MBLOCKS_BIT)));
```
but, the second one use the positive logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2062917757
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER ((uint32_t)0x7ffe)
+#define NXSEM_MRESET ((uint32_t)0x7fff)
+
+/* Macro to retrieve mutex's atomic holder's ptr */
+
+#define NXSEM_MHOLDER(s) ((FAR atomic_t *)&(s)->val.mholder)
+
+/* Check if holder value (TID) is not NO_HOLDER or RESET */
+
+#define NXSEM_MACQUIRED(h) (!(((h) & NXSEM_NO_MHOLDER) == NXSEM_NO_MHOLDER))
+
+/* Check if mutex is acquired and blocks some other task */
+
+#define NXSEM_MBLOCKS(h) (((h) & NXSEM_MBLOCKS_BIT) != 0)
Review Comment:
NXSEM_BLOCKING?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2834026328 > since the current code base never add const to the local integer variables even they doesn't change after initialization. Why not? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2062913438 ## libs/libc/misc/lib_mutex.c: ## @@ -35,30 +35,14 @@ * Pre-processor Definitions / -#define NXMUTEX_RESET ((pid_t)-2) - / * Private Functions Review Comment: remove what? NXMUTEX_RESET is already removed I believe. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2062913034
##
sched/semaphore/sem_waitirq.c:
##
@@ -72,30 +72,54 @@ void nxsem_wait_irq(FAR struct tcb_s *wtcb, int errcode)
{
FAR struct tcb_s *rtcb = this_task();
FAR sem_t *sem = wtcb->waitobj;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
/* It is possible that an interrupt/context switch beat us to the punch
* and already changed the task's state.
*/
- DEBUGASSERT(sem != NULL && atomic_read(NXSEM_COUNT(sem)) < 0);
+ DEBUGASSERT(sem != NULL &&
+ (mutex || atomic_read(NXSEM_COUNT(sem)) < 0) &&
+ (!mutex ||
Review Comment:
No, would be wrong. This checks " (if it is !mutex, the count must be < 0)
AND (if it is mutex, it must be blocking a thread).
Your suggestion would lead it to always fail for counting semaphores.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2062910152
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER ((uint32_t)0x7ffe)
+#define NXSEM_MRESET ((uint32_t)0x7fff)
+
+/* Macro to retrieve mutex's atomic holder's ptr */
+
+#define NXSEM_MHOLDER(s) ((FAR atomic_t *)&(s)->val.mholder)
+
+/* Check if holder value (TID) is not NO_HOLDER or RESET */
+
+#define NXSEM_MACQUIRED(h) (!(((h) & NXSEM_NO_MHOLDER) == NXSEM_NO_MHOLDER))
+
+/* Check if mutex is acquired and blocks some other task */
+
+#define NXSEM_MBLOCKS(h) (((h) & NXSEM_MBLOCKS_BIT) != 0)
Review Comment:
Yes, exacty my point. Perhaps NXSEM_MBLOCKING? Which tries to be an
abbreviation of "NXSEM_MUTEX_IS_BLOCKING", which just looks a bit too long to
my eye
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2062910152
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER ((uint32_t)0x7ffe)
+#define NXSEM_MRESET ((uint32_t)0x7fff)
+
+/* Macro to retrieve mutex's atomic holder's ptr */
+
+#define NXSEM_MHOLDER(s) ((FAR atomic_t *)&(s)->val.mholder)
+
+/* Check if holder value (TID) is not NO_HOLDER or RESET */
+
+#define NXSEM_MACQUIRED(h) (!(((h) & NXSEM_NO_MHOLDER) == NXSEM_NO_MHOLDER))
+
+/* Check if mutex is acquired and blocks some other task */
+
+#define NXSEM_MBLOCKS(h) (((h) & NXSEM_MBLOCKS_BIT) != 0)
Review Comment:
Yes, exacty my point. Perhaps NXSEM_MBLOCKING?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2062909627
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
Review Comment:
Perhaps NXSEM_MBLOCKING_BIT ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061222786
##
sched/semaphore/sem_recover.c:
##
@@ -104,7 +105,23 @@ void nxsem_recover(FAR struct tcb_s *tcb)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (NXSEM_IS_MUTEX(sem))
+{
+ /* Clear the blocking bit, if not blocked any more */
+
+ uint32_t blocks_mask =
Review Comment:
```
if (dq_empty(SEM_WAITLIST(sem)))
{
uint32_t holder = atomic_fetch_and(NXSEM_MHOLDER(sem),
~NXSEM_MBLOCKS_BIT);
DEBUGASSERT(NXSEM_MBLOCKS(holder));
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061217016
##
sched/semaphore/sem_holder.c:
##
@@ -876,7 +876,9 @@ void nxsem_canceled(FAR struct tcb_s *stcb, FAR sem_t *sem)
{
/* Check our assumptions */
- DEBUGASSERT(atomic_read(NXSEM_COUNT(sem)) <= 0);
+ DEBUGASSERT((NXSEM_IS_MUTEX(sem) || atomic_read(NXSEM_COUNT(sem)) <= 0));
+ DEBUGASSERT((!NXSEM_IS_MUTEX(sem) ||
Review Comment:
```
DEBUGASSERT(NXSEM_IS_MUTEX(sem) &&
NXSEM_MACQUIRED(atomic_read(NXSEM_MHOLDER(sem;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061353160
##
libs/libc/semaphore/sem_trywait.c:
##
@@ -128,8 +129,9 @@ int nxsem_trywait(FAR sem_t *sem)
#endif
)
{
- int32_t old = 1;
- return atomic_try_cmpxchg_acquire(NXSEM_COUNT(sem), &old, 0) ?
+ const int32_t tid = _SCHED_GETTID();
Review Comment:
yes, I know, but the current code base never add const to local integer
variables.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061338927 ## libs/libc/misc/lib_mutex.c: ## @@ -195,9 +140,11 @@ bool nxmutex_is_hold(FAR mutex_t *mutex) * / -int nxmutex_get_holder(FAR mutex_t *mutex) +pid_t nxmutex_get_holder(FAR mutex_t *mutex) Review Comment: but some functions is very simple, but not inline. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061340132
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER ((uint32_t)0x7ffe)
+#define NXSEM_MRESET ((uint32_t)0x7fff)
+
+/* Macro to retrieve mutex's atomic holder's ptr */
+
+#define NXSEM_MHOLDER(s) ((FAR atomic_t *)&(s)->val.mholder)
+
+/* Check if holder value (TID) is not NO_HOLDER or RESET */
+
+#define NXSEM_MACQUIRED(h) (!(((h) & NXSEM_NO_MHOLDER) == NXSEM_NO_MHOLDER))
+
+/* Check if mutex is acquired and blocks some other task */
+
+#define NXSEM_MBLOCKS(h) (((h) & NXSEM_MBLOCKS_BIT) != 0)
Review Comment:
from my understanding, this flag represents other threads is blocked(or
waiting) mutex.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2832245430 > @xiaoxiang781216 I disagree on your request for removing the const in many places. what is the reason why you want that? it just tells both the compiler and reader that the value is not going to change in the scope of the variable. since the current code base never add const to the local integer variables even they doesn't change after initialization. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061257154
##
sched/semaphore/sem_post.c:
##
@@ -73,10 +73,12 @@ int nxsem_post_slow(FAR sem_t *sem)
{
FAR struct tcb_s *stcb = NULL;
irqstate_t flags;
- int32_t sem_count;
#if defined(CONFIG_PRIORITY_INHERITANCE) || defined(CONFIG_PRIORITY_PROTECT)
uint8_t proto;
#endif
+ bool sem_blocks = false;
Review Comment:
no, sem is not blocked. it is blocking a thred, so it blocks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061255959
##
libs/libc/semaphore/sem_trywait.c:
##
@@ -128,8 +129,9 @@ int nxsem_trywait(FAR sem_t *sem)
#endif
)
{
- int32_t old = 1;
- return atomic_try_cmpxchg_acquire(NXSEM_COUNT(sem), &old, 0) ?
+ const int32_t tid = _SCHED_GETTID();
Review Comment:
why? tid is not going to change during this function. it is const in this
scope.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061256889
##
libs/libc/semaphore/sem_wait.c:
##
@@ -156,8 +157,9 @@ int nxsem_wait(FAR sem_t *sem)
# endif
)
{
- int32_t old = 1;
- if (atomic_try_cmpxchg_acquire(NXSEM_COUNT(sem), &old, 0))
+ const int32_t tid = _SCHED_GETTID();
Review Comment:
nah.. it is const in this scope
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061256667
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
Review Comment:
sure if you like it better. the mutex is not blocked, it blocks a thread,
that's why it is "blocks". but obviously you can think that holder refers to
the thread, so blocked is just as good
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2832026530 @xiaoxiang781216 I disagree on your request for removing the const in many places. what is the reason why you want that? it just tells both the compiler and reader that the value is not going to change in the scope of the variable. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061257731 ## libs/libc/misc/lib_mutex.c: ## @@ -195,9 +140,11 @@ bool nxmutex_is_hold(FAR mutex_t *mutex) * / -int nxmutex_get_holder(FAR mutex_t *mutex) +pid_t nxmutex_get_holder(FAR mutex_t *mutex) Review Comment: could do.. but if inlined function generates more code than a branch to the function, it increases code size, especially if it is frequently used. I only inlined functions for which I was sure that the code size doesn't increase -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061257020
##
sched/semaphore/sem_destroy.c:
##
@@ -61,6 +61,9 @@
int nxsem_destroy(FAR sem_t *sem)
{
int32_t old;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
Review Comment:
Why?? It is const in this scope
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061256810
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER ((uint32_t)0x7ffe)
+#define NXSEM_MRESET ((uint32_t)0x7fff)
+
+/* Macro to retrieve mutex's atomic holder's ptr */
+
+#define NXSEM_MHOLDER(s) ((FAR atomic_t *)&(s)->val.mholder)
+
+/* Check if holder value (TID) is not NO_HOLDER or RESET */
+
+#define NXSEM_MACQUIRED(h) (!(((h) & NXSEM_NO_MHOLDER) == NXSEM_NO_MHOLDER))
+
+/* Check if mutex is acquired and blocks some other task */
+
+#define NXSEM_MBLOCKS(h) (((h) & NXSEM_MBLOCKS_BIT) != 0)
Review Comment:
I don't like... semaphore is not blocked, it blocks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061256356
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER ((uint32_t)0x7ffe)
Review Comment:
no, this is correct. it has no holder if the value is NO_HOLDER or RESET.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
xiaoxiang781216 commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2061204544
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
Review Comment:
should we change ALL MBLOCKS to BLOCKED?(e.g. NXSEM_BLOCKED_BIT)?
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
Review Comment:
remove one space to align with other macros
##
sched/semaphore/sem_destroy.c:
##
@@ -61,6 +61,9 @@
int nxsem_destroy(FAR sem_t *sem)
{
int32_t old;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
Review Comment:
remove too
##
include/nuttx/semaphore.h:
##
@@ -45,23 +45,42 @@
/* semcount, flags, waitlist, hhead */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, NULL}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, NULL}
# else
/* semcount, flags, waitlist, holder[2] */
#define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER, SEMHOLDER_INITIALIZER}
# endif
#else /* CONFIG_PRIORITY_INHERITANCE */
/* semcount, flags, waitlist */
# define NXSEM_INITIALIZER(c, f) \
- {(c), (f), SEM_WAITLIST_INITIALIZER}
+ {{(c)}, (f), SEM_WAITLIST_INITIALIZER}
#endif /* CONFIG_PRIORITY_INHERITANCE */
-/* Macro to retrieve sem count */
+/* Macros to retrieve sem count and to check if nxsem is mutex */
-#define NXSEM_COUNT(s) ((FAR atomic_t *)&(s)->semcount)
+#define NXSEM_COUNT(s)((FAR atomic_t *)&(s)->val.semcount)
+#define NXSEM_IS_MUTEX(s) (((s)->flags & SEM_TYPE_MUTEX) != 0)
+
+/* Mutex related helper macros */
+
+#define NXSEM_MBLOCKS_BIT(((uint32_t)1) << 31)
+#define NXSEM_NO_MHOLDER ((uint32_t)0x7ffe)
Review Comment:
do you need swap the value of NXSEM_NO_MHOLDER and NXSEM_MRESET? to match
the bit and operation at line 79
##
sched/semaphore/sem_post.c:
##
@@ -85,19 +87,54 @@ int nxsem_post_slow(FAR sem_t *sem)
flags = enter_critical_section();
- /* Check the maximum allowable value */
+ if (mutex)
+{
+ /* Mutex post from interrupt context is not allowed */
+
+ DEBUGASSERT(!up_interrupt_context());
+
+ /* Lock the mutex for us by setting the blocking bit */
+
+ mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKS_BIT);
+
+ /* Mutex post from another thread is not allowed, unless
+ * called from nxsem_reset
+ */
+
+ DEBUGASSERT(mholder == (NXSEM_MBLOCKS_BIT | NXSEM_MRESET) ||
+ (mholder & (~NXSEM_MBLOCKS_BIT)) == nxsched_gettid());
+
+ sem_blocks = NXSEM_MBLOCKS(mholder);
- sem_count = atomic_read(NXSEM_COUNT(sem));
- do
+ if (!sem_blocks)
+{
+ if (mholder != NXSEM_MRESET)
+{
+ mholder = NXSEM_NO_MHOLDER;
+}
+
+ atomic_set(NXSEM_MHOLDER(sem), mholder);
+}
+}
+
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2060240413
##
sched/semaphore/sem_recover.c:
##
@@ -100,7 +101,39 @@ void nxsem_recover(FAR struct tcb_s *tcb)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (NXSEM_IS_MUTEX(sem))
+{
+ FAR dq_entry_t *wtcb;
+ uint32_t mholder;
+ uint32_t blocks_mask = ~NXSEM_MBLOCKS_BIT;
+
+ /* The TID of the mutex holder is correct but we need to
+ * update the blocking bit. Count the remaining tcbs in the
+ * blocking list (ignoring the one that is being cancelled).
+ */
+
+ for (wtcb = dq_peek(SEM_WAITLIST(sem)); wtcb; wtcb = dq_next(wtcb))
Review Comment:
Decided to fix it right away. Tested again MPFS SMP / CONFIG_BUILD_FLAT and
rv-virt:nsh, also ostest on both. Should be fine...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
pussuw commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2060189145
##
sched/semaphore/sem_recover.c:
##
@@ -100,7 +101,39 @@ void nxsem_recover(FAR struct tcb_s *tcb)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (NXSEM_IS_MUTEX(sem))
+{
+ FAR dq_entry_t *wtcb;
+ uint32_t mholder;
+ uint32_t blocks_mask = ~NXSEM_MBLOCKS_BIT;
+
+ /* The TID of the mutex holder is correct but we need to
+ * update the blocking bit. Count the remaining tcbs in the
+ * blocking list (ignoring the one that is being cancelled).
+ */
+
+ for (wtcb = dq_peek(SEM_WAITLIST(sem)); wtcb; wtcb = dq_next(wtcb))
Review Comment:
Check that nxsem_wait_irq is not called by the task exit logic, I forgot to
verify that
Edit: There is no call to nxsem_wait_irq
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
pussuw commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2060189145
##
sched/semaphore/sem_recover.c:
##
@@ -100,7 +101,39 @@ void nxsem_recover(FAR struct tcb_s *tcb)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (NXSEM_IS_MUTEX(sem))
+{
+ FAR dq_entry_t *wtcb;
+ uint32_t mholder;
+ uint32_t blocks_mask = ~NXSEM_MBLOCKS_BIT;
+
+ /* The TID of the mutex holder is correct but we need to
+ * update the blocking bit. Count the remaining tcbs in the
+ * blocking list (ignoring the one that is being cancelled).
+ */
+
+ for (wtcb = dq_peek(SEM_WAITLIST(sem)); wtcb; wtcb = dq_next(wtcb))
Review Comment:
Check that nxsem_wait_irq is not called by the task exit logic, I forgot to
verify that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2060173828
##
sched/semaphore/sem_recover.c:
##
@@ -100,7 +101,39 @@ void nxsem_recover(FAR struct tcb_s *tcb)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (NXSEM_IS_MUTEX(sem))
+{
+ FAR dq_entry_t *wtcb;
+ uint32_t mholder;
+ uint32_t blocks_mask = ~NXSEM_MBLOCKS_BIT;
+
+ /* The TID of the mutex holder is correct but we need to
+ * update the blocking bit. Count the remaining tcbs in the
+ * blocking list (ignoring the one that is being cancelled).
+ */
+
+ for (wtcb = dq_peek(SEM_WAITLIST(sem)); wtcb; wtcb = dq_next(wtcb))
Review Comment:
Yes, I was reading your comment with eyes crossed. There indeed seems to be
an already existing bug in here. The counting semaphores should have been
removed from the waiting list just the same. I was copying the existing
functionality and didn't just notice that.
I'll make a separate patch fixing this, and add to this same PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
pussuw commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2060137404
##
sched/semaphore/sem_recover.c:
##
@@ -100,7 +101,39 @@ void nxsem_recover(FAR struct tcb_s *tcb)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (NXSEM_IS_MUTEX(sem))
+{
+ FAR dq_entry_t *wtcb;
+ uint32_t mholder;
+ uint32_t blocks_mask = ~NXSEM_MBLOCKS_BIT;
+
+ /* The TID of the mutex holder is correct but we need to
+ * update the blocking bit. Count the remaining tcbs in the
+ * blocking list (ignoring the one that is being cancelled).
+ */
+
+ for (wtcb = dq_peek(SEM_WAITLIST(sem)); wtcb; wtcb = dq_next(wtcb))
Review Comment:
I think you are mixing up holder and waiter lists now. For a mutex only a
single entry can EVER be in the holder list, and in this case since tcb is
waiting for the mutex it CANNOT be the holder. For counting semaphores this is
different.
However, for waiter list there can be multiple entries.
What I said above is correct, the tcb must be removed from the semaphores
waiter list here. Otherwise it stays there forever.
It also looks like for semaphores there is a bug, the tcb being removed
stays in the wait list forever...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
pussuw commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2060138277
##
sched/semaphore/sem_recover.c:
##
@@ -100,7 +101,39 @@ void nxsem_recover(FAR struct tcb_s *tcb)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (NXSEM_IS_MUTEX(sem))
+{
+ FAR dq_entry_t *wtcb;
+ uint32_t mholder;
+ uint32_t blocks_mask = ~NXSEM_MBLOCKS_BIT;
+
+ /* The TID of the mutex holder is correct but we need to
+ * update the blocking bit. Count the remaining tcbs in the
+ * blocking list (ignoring the one that is being cancelled).
+ */
+
+ for (wtcb = dq_peek(SEM_WAITLIST(sem)); wtcb; wtcb = dq_next(wtcb))
Review Comment:
I can find two places where nodes are removed from SEM_WAITLIST:
https://github.com/apache/nuttx/blob/8832136b69865d54124c9a48d3a87c29ecca152c/sched/semaphore/sem_waitirq.c#L98)
https://github.com/apache/nuttx/blob/8832136b69865d54124c9a48d3a87c29ecca152c/sched/semaphore/sem_post.c#L149
So yes, it does look like we leave the dying tcb in the semaphore's waiter
list for eternity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
pussuw commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2060137404
##
sched/semaphore/sem_recover.c:
##
@@ -100,7 +101,39 @@ void nxsem_recover(FAR struct tcb_s *tcb)
* place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (NXSEM_IS_MUTEX(sem))
+{
+ FAR dq_entry_t *wtcb;
+ uint32_t mholder;
+ uint32_t blocks_mask = ~NXSEM_MBLOCKS_BIT;
+
+ /* The TID of the mutex holder is correct but we need to
+ * update the blocking bit. Count the remaining tcbs in the
+ * blocking list (ignoring the one that is being cancelled).
+ */
+
+ for (wtcb = dq_peek(SEM_WAITLIST(sem)); wtcb; wtcb = dq_next(wtcb))
Review Comment:
I think you are mixing up holder and waiter lists now. For a mutex only a
single entry can EVER be in the holder list, for counting semaphores this is
different.
However, for waiter list there can be multiple entries.
What I said above is correct, the tcb must be removed from the semaphores
waiter list here. Otherwise it stays there forever.
It also looks like for semaphores there is a bug, the tcb being removed
stays in the wait list forever...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
pussuw commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2060112180
##
sched/semaphore/sem_post.c:
##
@@ -85,19 +87,54 @@ int nxsem_post_slow(FAR sem_t *sem)
flags = enter_critical_section();
- /* Check the maximum allowable value */
+ if (mutex)
+{
+ /* Mutex post from interrupt context is not allowed */
+
+ DEBUGASSERT(!up_interrupt_context());
+
+ /* Lock the mutex for us by setting the blocking bit */
+
+ mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKS_BIT);
+
+ /* Mutex post from another thread is not allowed, unless
Review Comment:
Let's not try to remove it here, instead make a coherent implementation for
nxmutex_reset.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
pussuw commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2060111412
##
sched/semaphore/sem_recover.c:
##
@@ -86,7 +86,8 @@ void nxsem_recover(FAR struct tcb_s *tcb)
if (tcb->task_state == TSTATE_WAIT_SEM)
{
FAR sem_t *sem = tcb->waitobj;
- DEBUGASSERT(sem != NULL && atomic_read(NXSEM_COUNT(sem)) < 0);
+
+ DEBUGASSERT(sem != NULL);
/* Restore the correct priority of all threads that hold references
Review Comment:
Yes, many can wait, but only 1 can hold
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on PR #16194: URL: https://github.com/apache/nuttx/pull/16194#issuecomment-2829681915 @pussuw I now modified the nxsem_reset for mutex somewhat acc. to your feedback, and re-tested that it works for me. Please re-check that part! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2059578589 ## sched/semaphore/sem_wait.c: ## @@ -218,6 +271,16 @@ int nxsem_wait_slow(FAR sem_t *sem) #endif } + /* If this now holds the mutex, set the holder TID and the lock bit */ + + if (mutex && ret == OK) Review Comment: Yes, thanks for asking, the discussion clarifies things for others as well. I believe the only "sane" place to set the mutex holder TID is really when it gets out of sem_wait and really holding the mutex. This is after all the priority adjustments and what not. Setting it in sem_post is more dubious, but it must be set to something else than the TID which just posted. One could arque that the most proper value to set in sem post would be "NXSEM_NO_MHOLDER | NXSEM_MBLOCKS_BIT", since at that point no other threads who have acquired the mutex are running. However, this would require special handling again in sem_wait; if while everyone, who acquired the mutex before, are blocked for some reason and some other thread blocks on the mutex again, one would need special handling for that value. So basically, setting *any* TID from waiting list is fine - you don't *really* need to know which one of those will wake up. Picking the one from the top of the waitlist is basically the same thing as what is done with counting semaphores. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194:
URL: https://github.com/apache/nuttx/pull/16194#discussion_r2059591043
##
sched/semaphore/sem_post.c:
##
@@ -85,19 +87,54 @@ int nxsem_post_slow(FAR sem_t *sem)
flags = enter_critical_section();
- /* Check the maximum allowable value */
+ if (mutex)
+{
+ /* Mutex post from interrupt context is not allowed */
+
+ DEBUGASSERT(!up_interrupt_context());
+
+ /* Lock the mutex for us by setting the blocking bit */
+
+ mholder = atomic_fetch_or(NXSEM_MHOLDER(sem), NXSEM_MBLOCKS_BIT);
+
+ /* Mutex post from another thread is not allowed, unless
Review Comment:
Yes, sticking to that for now... all my attempts to remove it caused havoc,
especially with USB CDCACM (in the drone mavlink over USB). Replicating the
functionality of nxmutex_reset just does the trick for now. Eventually I got
fed up with debugging USB and serial drivers. There has to be some design flaw
in the serial if one needs to "reset" mutexes, I just couldn't wrap my head
around that yet, I am currently working on a bit too many things at the same
time
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2059578589 ## sched/semaphore/sem_wait.c: ## @@ -218,6 +271,16 @@ int nxsem_wait_slow(FAR sem_t *sem) #endif } + /* If this now holds the mutex, set the holder TID and the lock bit */ + + if (mutex && ret == OK) Review Comment: Yes, thanks for asking, the discussion clarifies things for others as well. I believe the only "sane" place to set the mutex holder TID is really when it gets out of sem_wait and really holding the mutex. This is after all the priority adjustments and what not. Setting it in sem_post is more dubious, but it must be set to something else than the TID which just posted. One could arque that the most proper value to set in sem post would be "NXSEM_NO_MHOLDER | NXSEM_MBLOCKS_BIT", since at that point no other threads who have acquired the mutex are running. However, this would require special handling again in sem_wait; if, while everyone acquired the mutex before, are blocked for some reason and some thread blocks on the mutex again, one would need special handling for that value. So basically, setting *any* TID from waiting list is fine - you don't *really* need to know which one of those will wake up. Picking the one from the top of the waitlist is basically the same thing as what is done with counting semaphores. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Enable mutex functionality in nxsem [nuttx]
jlaitine commented on code in PR #16194: URL: https://github.com/apache/nuttx/pull/16194#discussion_r2059569009 ## libs/libc/semaphore/sem_init.c: ## @@ -62,15 +62,15 @@ * / -int nxsem_init(FAR sem_t *sem, int pshared, unsigned int value) +int nxsem_init(FAR sem_t *sem, int pshared, int32_t value) Review Comment: Yes, and I had already forgotten this; I believe changing it is fine since it is anyhow nuttx internal interface, and initializing a 32-bit value (both counter and holder are 32 bits nowdays). But, it probably should be uint32_t; it would make no sense to initialize a counting semaphore with a negative value... -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
