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)0x7ffffffe)
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);
+ }
+ }
+ else
{
- if (sem_count >= SEM_VALUE_MAX)
+ int32_t sem_count;
Review Comment:
sem_count to count?
##########
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));
}
```
##########
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 blocks = dq_peek(SEM_WAITLIST(sem)) != NULL ?
Review Comment:
blocks to blocked and replace dq_peek with dq_empty
##########
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:
bool unlocked = false;
##########
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);
Review Comment:
remove const
##########
libs/libc/misc/lib_mutex.c:
##########
@@ -35,30 +35,14 @@
* Pre-processor Definitions
****************************************************************************/
-#define NXMUTEX_RESET ((pid_t)-2)
-
/****************************************************************************
* Private Functions
Review Comment:
remove
##########
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)0x7ffffffe)
+#define NXSEM_MRESET ((uint32_t)0x7fffffff)
+
+/* 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))
Review Comment:
```
#define NXSEM_MACQUIRED(h) (((h) & NXSEM_NO_MHOLDER) != NXSEM_NO_MHOLDER)
```
##########
include/semaphore.h:
##########
@@ -104,8 +104,16 @@ struct semholder_s
struct sem_s
{
- volatile int32_t semcount; /* >0 -> Num counts available */
- /* <0 -> Num tasks waiting for semaphore */
+ union
+ {
+ volatile int32_t semcount; /* >0 -> Num counts available */
+ /* <0 -> Num tasks waiting for semaphore
*/
+ volatile uint32_t mholder; /* == NXSEM_NO_MHOLDER -> mutex has no
holder */
+ /* == NXSEM_RESET -> mutex has been reset
*/
+ /* Otherwise: */
+ /* bits[30:0]: TID of the current
holder */
+ /* bit [31]: Mutex is blocking some
task */
+ } val;
Review Comment:
need add more spaces to align
##########
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)0x7ffffffe)
+#define NXSEM_MRESET ((uint32_t)0x7fffffff)
+
+/* 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_IS_MBLOCKED?
##########
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))));
```
##########
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:
remove const?
##########
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:
can we remove `m` from ALL symbols
##########
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:
sem_blocks->blocked
##########
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:
```
DEBUGASSERT(!NXSEM_IS_MUTEX(sem) && atomic_read(NXSEM_COUNT(sem)) <= 0);
```
##########
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:
```
FAR const atomic_t *val = mutex ? NXSEM_MHOLDER(sem) : NXSEM_COUNT(sem);
```
##########
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:
remove too
##########
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:
`(mholder & ~NXSEM_MBLOCKS_BIT) == nxsched_gettid());`
##########
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);
+ const int32_t new = mutex ? NXSEM_NO_MHOLDER : 1;
Review Comment:
remove const
##########
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);
Review Comment:
remove const
##########
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:
dq_empty
##########
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;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
Review Comment:
remove const
##########
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:
but mutex with priority inheritance should to else path.
##########
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;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
+ FAR atomic_t *const plock = mutex ? NXSEM_MHOLDER(sem) : NXSEM_COUNT(sem);
Review Comment:
```
FAR const atomic_t *val = mutex ? NXSEM_MHOLDER(sem) : NXSEM_COUNT(sem);
```
##########
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't be a mutex?
##########
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 ||
+ (atomic_read(NXSEM_MHOLDER(sem)) & NXSEM_MBLOCKS_BIT)));
+
+ /* Mutex is never interrupted by a signal or canceled */
+
+ if (mutex && (errcode == EINTR || errcode == ECANCELED))
+ {
+ return;
+ }
/* Restore the correct priority of all threads that hold references
* to this semaphore.
*/
nxsem_canceled(wtcb, sem);
- /* And increment the count on the semaphore. This releases the count
- * that was taken by sem_post(). This count decremented the semaphore
- * count to negative and caused the thread to be blocked in the first
- * place.
+ /* Remove task from waiting list */
+
+ dq_rem((FAR dq_entry_t *)wtcb, SEM_WAITLIST(sem));
+
+ /* This restores the value to what it was before the previous sem_wait.
+ * This caused the thread to be blocked in the first place.
*/
- atomic_fetch_add(NXSEM_COUNT(sem), 1);
+ if (mutex)
+ {
+ /* The TID of the mutex holder is correct but we need to
+ * update the blocking bit. The mutex is still blocking if there are
+ * any items left in the wait queue.
+ */
- /* Remove task from waiting list */
+ uint32_t blocks_mask =
+ dq_peek(SEM_WAITLIST(sem)) != 0 ? ~0 : ~NXSEM_MBLOCKS_BIT;
Review Comment:
ditto
##########
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));
Review Comment:
remove () around NXSEM_MBLOCKS_BIT and merge line 78 here
##########
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:
holder
##########
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;
+ FAR struct tcb_s *htcb = NULL;
+ const bool mutex = NXSEM_IS_MUTEX(sem);
Review Comment:
remove const
##########
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:
semcount->count?
##########
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:
`unlocked = !htcb;` and move after line 110
##########
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:
why not inline all functions in this file?
##########
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:
why not inline nxmutex_is_hold
##########
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:
`mutex && NXSEM_MBLOCKS(atomic_read(NXSEM_MHOLDER(sem))`
##########
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) &&
Review Comment:
`!mutex && atomic_read(NXSEM_COUNT(sem)) < 0`
##########
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:
why not inline nxmutex_init too
##########
sched/semaphore/sem_wait.c:
##########
@@ -218,6 +270,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)
+ {
+ uint32_t blocks = dq_peek(SEM_WAITLIST(sem)) == NULL ? 0 :
Review Comment:
use dq_empty instead
--
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
