You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/05 11:35:09 UTC
[GitHub] [incubator-nuttx] zhaoxiu-zeng opened a new pull request #5171: semphore: release all semphores' holder that the task held when exit
zhaoxiu-zeng opened a new pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171
Add a list in TCB to track all semphores the task held, so we
can release all holders when exit, so nxsched_verify_tcb
is unnecessary.
Signed-off-by: Zeng Zhaoxiu <wa...@transtekcorp.com>
## Summary
## Impact
## Testing
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779457573
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,17 +1011,52 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+ {
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+ {
+ pholder = &sem->holder[i];
+#endif
+
+ if (pholder->counts <= 0)
+ continue;
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
Yes, the sum of holder's counts must <= SEM_VALUE_MAX, but there may be multi holders. If the caller of sem_post is not a holder, how we choose the holder to decremnt it's counts?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779482914
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,17 +1011,52 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+ {
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+ {
+ pholder = &sem->holder[i];
+#endif
+
+ if (pholder->counts <= 0)
+ continue;
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
> Yes, the sum of holder's counts must <= SEM_VALUE_MAX, but there may be multi holders. If the caller of sem_post is not a holder, how do we choose the holder to decrement it's counts?
No, the sum of holder's counts is not limited by `SEM_VALUE_MAX`. For example the task1 always call `sem_post` and task2 and task3 try to take semaphore with concurrency to each other. Then sem counts will be incremented and decremented why task2 and task3 holders counts will increment only (currently with rollover).
As it was discussed in the mailing list signaling use case for semaphores is not a valid case, so we do not have any expectations to priority inheritance in this case. So I really think that might think of only limiting holders count by `SEM_VALUE_MAX` and do not decrement at all.
As an alternative to your proposed changes it is better to fing a holder with the highest counts value and use it as a candidate.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782855494
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +945,58 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
Review comment:
```suggestion
if (pholder->counts <= 0)
{
continue;
}
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782747575
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -263,23 +276,16 @@ static int nxsem_foreachholder(FAR sem_t *sem, holderhandler_t handler,
next = pholder->flink;
Review comment:
No, because the holder may be freed in the handler callback, so we must preserve pointer to the next 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r787400505
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -954,11 +867,8 @@ void nxsem_add_holder_tcb(FAR struct tcb_s *htcb, FAR sem_t *sem)
pholder = nxsem_findorallocateholder(sem, htcb);
if (pholder != NULL)
{
- /* Then set the holder and increment the number of counts held by
- * this holder
- */
+ /* Increment the number of counts held by this holder */
- pholder->htcb = htcb;
pholder->counts++;
Review comment:
I think we should add a constraint that only the holder task can call nxsem_release_holder just like other OS mutexes, then there is no such problems.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779272625
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1200,4 +1211,31 @@ int nxsem_nfreeholders(void)
}
#endif
+/****************************************************************************
+ * Name: nxsem_release_all
+ *
+ * Description:
+ * Release all semphore holders for the task.
+ *
+ * Input Parameters:
+ * stcb - TCB of the task
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void nxsem_release_all(FAR struct tcb_s *htcb)
+{
+ while (htcb->holdsem != NULL)
+ {
+ FAR struct semholder_s *pholder = htcb->holdsem;
+ FAR sem_t *sem = pholder->sem;
+
+ nxsem_freeholder(sem, pholder);
+ }
+}
Review comment:
Applied, thanks!
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -86,30 +87,36 @@ static inline FAR struct semholder_s *nxsem_allocholder(sem_t *sem)
g_freeholders = pholder->flink;
pholder->flink = sem->hhead;
sem->hhead = pholder;
-
- /* Make sure the initial count is zero */
-
- pholder->counts = 0;
}
#else
if (sem->holder[0].htcb == NULL)
{
pholder = &sem->holder[0];
- pholder->counts = 0;
}
else if (sem->holder[1].htcb == NULL)
{
pholder = &sem->holder[1];
- pholder->counts = 0;
}
#endif
else
{
serr("ERROR: Insufficient pre-allocated holders\n");
pholder = NULL;
+ DEBUGASSERT(pholder != NULL);
Review comment:
Applied, thanks!
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779457547
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,17 +1011,52 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+ {
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+ {
+ pholder = &sem->holder[i];
+#endif
+
+ if (pholder->counts <= 0)
+ continue;
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
I see. You are trying to handle the case when task that posts semaphore never becomes a holder. As an alternative we might limit `pholders->count` by `SEM_VALUE_MAX` before we increment.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782851520
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -66,7 +66,8 @@ static FAR struct semholder_s *g_freeholders;
* Name: nxsem_allocholder
****************************************************************************/
-static inline FAR struct semholder_s *nxsem_allocholder(sem_t *sem)
+static inline FAR struct semholder_s *nxsem_allocholder(FAR sem_t *sem,
+ FAR struct tcb_s *htcb)
Review comment:
```suggestion
static inline FAR struct semholder_s *
nxsem_allocholder(FAR sem_t *sem, FAR struct tcb_s *htcb)
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1036439552
LGTM.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r791994316
##########
File path: include/nuttx/sched.h
##########
@@ -656,6 +656,10 @@ struct tcb_s
sem_t *waitsem; /* Semaphore ID waiting on */
+#ifdef CONFIG_PRIORITY_INHERITANCE
+ FAR struct semholder_s *holdsem; /* List of held semaphores */
Review comment:
move to line 606-612 section
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +945,60 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
Review comment:
need add {
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,17 +1011,52 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+ {
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+ {
+ pholder = &sem->holder[i];
+#endif
+
+ if (pholder->counts <= 0)
+ continue;
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
> If the caller of sem_post is not a holder (such as task1 call sem_wait and task2 call sem_post), nxsem_findholder(sem, rtcb) must return NULL.
>
in this case, the caller use sem as event, not lock, he should disable priority inheritance first. Why we need take care the wrong usage? it's caller responsibility to fix his bug.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r780004722
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
Review comment:
Holder's count does not need to be checked because the holder will be freed in nxsem_restore_baseprio if it's count is 0.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r801412909
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +945,60 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
Review comment:
There is a '{', in the next line of "#endif".
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782747575
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -263,23 +276,16 @@ static int nxsem_foreachholder(FAR sem_t *sem, holderhandler_t handler,
next = pholder->flink;
Review comment:
No, the holder may be freed in the handler callback, so we must preserve pointer to the next holder before calling handler.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1011486595
I'm reviewing... there's quite a bit here so it will take some time to grok all the 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779261590
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,17 +1011,52 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+ {
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+ {
+ pholder = &sem->holder[i];
+#endif
+
+ if (pholder->counts <= 0)
+ continue;
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
If the caller of sem_post is not a holder (such as task1 call sem_wait and task2 call sem_post), nxsem_findholder(sem, rtcb) must return NULL.
If the semphore's value >= 2, and more than 1 tasks have called sem_wait, the semphore has multi holders.
If the semphore has only one holder, we can decrement the counts immediately.
Else, it's a bit of a hassle, holders can only be freed until the task exits or the sem destroys.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1032382647
> @zhaoxiu-zeng look like you mix the different change in one patch. Since this change is complicated, I would suggest to split the huge patch into small ones to fix the individual issue
OK. I has split the changes into 3 commits.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r801410967
##########
File path: include/nuttx/sched.h
##########
@@ -656,6 +656,10 @@ struct tcb_s
sem_t *waitsem; /* Semaphore ID waiting on */
+#ifdef CONFIG_PRIORITY_INHERITANCE
+ FAR struct semholder_s *holdsem; /* List of held semaphores */
Review comment:
Applied, thanks!
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1032484784
CI system encountered problems, how to restart build on macOS? @xiaoxiang781216
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782806030
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
Review comment:
If there are 2 holders (not the rtcb) with the same counts which is 1, whoever is chosen will break the priority inheritance.
If neither pholder->htcb == rtcb nor total == 1 is met, it should be the wrong usage of the semaphore. Currently do nothing, as the original code. Leave it for later for us to find a smarter way, or change the design.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782584997
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
+#endif
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
Review comment:
```suggestion
if (candidate == NULL)
{
candidate = pholder;
}
```
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
+#endif
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
Review comment:
But I would better go with
```suggestion
if ((candidate == NULL) || (candidate->counts < pholder->counts))
{
candidate = pholder;
}
```
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
Review comment:
I still have a question here. Why we rely on `counts` value and not on `if (pholder->htcb != NULL)`?
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
+#endif
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
I do not think that `total` is needed. If we do not hit `if (pholder->htcb == rtcb)` then we can simply pick the holder with the highest `counts` value and rely on `if (candidate != NULL)` here.
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
Review comment:
In general I would suggest to rewrite logic in the next way:
```
void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
FAR struct semholder_s *candidate = NULL;
/* Find the container for this holder */
#if CONFIG_SEM_PREALLOCHOLDERS > 0
for (pholder = sem->hhead; pholder; pholder = pholder->flink)
#else
int i;
/* We have two hard-allocated holder structures in sem_t */
for (i = 0, pholder = &sem->holder[0]; i < 2; pholder = &sem->holder[i++])
#endif
{
if (pholder->htcb == rtcb)
{
candidate = pholder;
break;
}
else if (pholder->htcb != NULL)
{
if ((candidate == NULL) || (candidate->counts < pholder->counts))
{
candidate = pholder;
}
}
}
/* We either found the task that is a holder or the best candidate */
if (candidate != NULL)
{
/* Decrement the counts on this holder -- the holder will be freed
* later in nxsem_restore_baseprio.
*/
candidate->counts--;
}
}
```
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
+#endif
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
{
- /* Decrement the counts on this holder -- the holder will be freed
- * later in nxsem_restore_baseprio.
+ /* If the sempahore has only one holder, we can decrement the counts
+ * simply.
*/
- pholder->counts--;
+ candidate->counts--;
+ return;
Review comment:
```suggestion
```
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -263,23 +276,16 @@ static int nxsem_foreachholder(FAR sem_t *sem, holderhandler_t handler,
next = pholder->flink;
Review comment:
Maybe `next` can be eliminated and moved into `for` loop with `pholder = pholder->flink`? The same as it is done in `nxsem_release_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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r783045941
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -954,11 +867,8 @@ void nxsem_add_holder_tcb(FAR struct tcb_s *htcb, FAR sem_t *sem)
pholder = nxsem_findorallocateholder(sem, htcb);
if (pholder != NULL)
{
- /* Then set the holder and increment the number of counts held by
- * this holder
- */
+ /* Increment the number of counts held by this holder */
- pholder->htcb = htcb;
pholder->counts++;
Review comment:
I still believe that this will bring bugs in case of 1 producer and 2 consumers signaling model. For example there are 3 tasks. Task 1 does `sem_post` and Task 2 and 3 are calling `sem_wait`. For example Task 1 is the highest priority and does 2 `sem_post` in a row. The Task 2 consumes 1 count and does some work and Task 3 consumes 1 count and does some work. Then it is repeated cyclically. In this case Task 2 and 3 will be sem holders always (because we do not decrement `pholders->counts` in `nxsem_release_holders`) and `pholders->counts` will overflow at some point and `nxsem_freecount0holder` or `nxsem_restoreholderprio` will finally execute `nxsem_freeholder(sem, pholder);` so all the system will be in the undefined behavior.
I really think that we need to limit `pholder->counts` with `SEM_VALUE_MAX` and have here something like
```
if (pholder->counts < SEM_VALUE_MAX)
{
pholder->counts++;
}
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779457547
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,17 +1011,52 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+ {
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+ {
+ pholder = &sem->holder[i];
+#endif
+
+ if (pholder->counts <= 0)
+ continue;
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
I see. You are trying to handle the case when task that posts semaphore never becomes a holder. As an alternative we might limit `pholders->counts` by `SEM_VALUE_MAX` before we increment.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779449804
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,17 +1011,52 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+ {
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+ {
+ pholder = &sem->holder[i];
+#endif
+
+ if (pholder->counts <= 0)
+ continue;
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
But `sem_post` can't feed more than `SEM_VALUE_MAX`, so `pholder->counts` is limited to same value implicitly, isn't 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782845734
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782912065
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +945,60 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ {
+ continue;
+ }
+#endif
+
+ DEBUGASSERT(pholder->htcb != NULL);
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
```suggestion
if (candidate != NULL)
```
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +945,60 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate;
Review comment:
```suggestion
FAR struct semholder_s *candidate = NULL;
```
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +945,60 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ {
+ continue;
+ }
+#endif
+
+ DEBUGASSERT(pholder->htcb != NULL);
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
Review comment:
```suggestion
candidate = (total == 0) ? pholder : NULL;
total++;
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r780052416
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
Why `pholder->counts <= 0` with `continue` is checked only in case if number if preallocated holders is 0?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r780051492
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
Yes. My bad. I just thought that we can eliminate `continue`
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
Why `pholder->counts <= 0` with `continue` is checked only in case if number if preallocated holders is 0?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782808690
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
Review comment:
Would it be okay to add a assertsion "pholder->htcb != NULL" when pholder->counts <= 0?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782806030
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
Review comment:
If there are 2 holders (not the rtcb) with the same counts which is 1, whoever is chosen will break the priority inheritance.
If neither pholder->htcb == rtcb nor total == 1 is met, it should be the wrong usage of semaphore. Currently do nothing, as the original code. Leave it for later for us to find a smarter way, or change the design.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782747575
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -263,23 +276,16 @@ static int nxsem_foreachholder(FAR sem_t *sem, holderhandler_t handler,
next = pholder->flink;
Review comment:
No, because the holder may be freed in the handler callback, so we must preserve pointer to the next holder before calling handler.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782787550
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
Review comment:
Yes. I understand. I just see that in all other places decision in done based on `htcb`, so suggest a common approach. We can add instrumentation to assert if `htcb != NULL` and `counts <= 0` to catch possible bugs
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782811721
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
Review comment:
I think we can add `DEBUGASSERT`
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1032713002
> CI system encountered problems, how to restart build on macOS? @xiaoxiang781216
Two method:
1. Push your patch with -f again(you can reset hard the master head if you can't change anything)
2. Or let maintain restart CI for you like I am doing 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779009113
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,17 +1011,52 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+ {
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+ {
+ pholder = &sem->holder[i];
+#endif
+
+ if (pholder->counts <= 0)
+ continue;
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
I'm not sure in this logic. We run this code even if `rtcb` does not match `pholder->htcb`. I'm out of laptop till 8 Jan, so will appreciate if you can describe more a use case that meets this situation.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779457573
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,17 +1011,52 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+ {
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+ {
+ pholder = &sem->holder[i];
+#endif
+
+ if (pholder->counts <= 0)
+ continue;
+
+ if (pholder->htcb == rtcb)
+ {
+ /* Decrement the counts on this holder -- the holder will be freed
+ * later in nxsem_restore_baseprio.
+ */
+
+ pholder->counts--;
+ return;
+ }
+
+ total++;
+ if (candidate == NULL)
+ candidate = pholder;
+ }
+
+ /* The current task is not a holder */
+
+ if (total == 1)
Review comment:
Yes, the sum of holder's counts must <= SEM_VALUE_MAX, but there may be multi holders. If the caller of sem_post is not a holder, how do we choose the holder to decrement it's counts?
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779510028
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
```suggestion
for (i = 0, pholder = &sem->holder[i];
i < 2 && pholder->counts > 0;
i++, pholder++)
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779510028
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
```suggestion
for (i = 0, pholder = &sem->holder[i];
i < 2 && pholder->counts > 0;
i++)
```
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+#endif
+ if (pholder->counts <= 0)
+ continue;
Review comment:
```suggestion
```
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
Review comment:
```suggestion
for (pholder = sem->hhead;
pholder != NULL && pholder->counts > 0;
pholder = pholder->flink)
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r780051492
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
Yes. My bad. I just thought that we can eliminate `continue`
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r780196314
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
If the holder's counts == 0, it will be removed from the list later in nxsem_restore_baseprio, so all holder's counts must > 0 when enter nxsem_release_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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r783045941
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -954,11 +867,8 @@ void nxsem_add_holder_tcb(FAR struct tcb_s *htcb, FAR sem_t *sem)
pholder = nxsem_findorallocateholder(sem, htcb);
if (pholder != NULL)
{
- /* Then set the holder and increment the number of counts held by
- * this holder
- */
+ /* Increment the number of counts held by this holder */
- pholder->htcb = htcb;
pholder->counts++;
Review comment:
I still believe that this will bring bugs in case of 1 producer and 2 consumers signaling model. For example there are 3 tasks. Task 1 does `sem_post` and Task 2 and 3 are calling `sem_wait`. For example Task 1 is the highest priority and does 2 `sem_post` in a row. The Task 2 consumes 1 count and does some work and Task 3 consumes 1 count and does some work. Then it is repeated cyclically. In this case Task 2 and 3 will be sem holders always (because we do not increment `pholders->counts` in `nxsem_release_holders`) and `pholders->counts` will overflow at some point and `nxsem_freecount0holder` or `nxsem_restoreholderprio` will finally execute `nxsem_freeholder(sem, pholder);` so all the system will be in the undefined behavior.
I really think that we need to limit `pholder->counts` with `SEM_VALUE_MAX` and have here something like
```
if (pholder->counts < SEM_VALUE_MAX)
{
pholder->counts++;
}
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng edited a comment on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng edited a comment on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1032382647
> @zhaoxiu-zeng look like you mix the different change in one patch. Since this change is complicated, I would suggest to split the huge patch into small ones to fix the individual issue
OK. I split the changes into 3 commits.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1032713002
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1032763897
Look like you have to rebase your patch to the latest mainline and push again to fix the CI temp failure.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r778982536
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -187,35 +194,36 @@ nxsem_findorallocateholder(sem_t *sem, FAR struct tcb_s *htcb)
static inline void nxsem_freeholder(sem_t *sem,
FAR struct semholder_s *pholder)
{
-#if CONFIG_SEM_PREALLOCHOLDERS > 0
- FAR struct semholder_s *curr;
- FAR struct semholder_s *prev;
-#endif
+ FAR struct semholder_s **curr;
Review comment:
```suggestion
struct semholder_s * FAR *curr;
```
##########
File path: include/semaphore.h
##########
@@ -58,19 +58,23 @@
#ifdef CONFIG_PRIORITY_INHERITANCE
struct tcb_s; /* Forward reference */
+struct sem_s;
+
struct semholder_s
{
#if CONFIG_SEM_PREALLOCHOLDERS > 0
- struct semholder_s *flink; /* Implements singly linked list */
+ struct semholder_s *flink; /* List of semphore's holder */
Review comment:
```suggestion
FAR struct semholder_s *flink; /* List of semphore's holder */
```
##########
File path: include/semaphore.h
##########
@@ -58,19 +58,23 @@
#ifdef CONFIG_PRIORITY_INHERITANCE
struct tcb_s; /* Forward reference */
+struct sem_s;
+
struct semholder_s
{
#if CONFIG_SEM_PREALLOCHOLDERS > 0
- struct semholder_s *flink; /* Implements singly linked list */
+ struct semholder_s *flink; /* List of semphore's holder */
#endif
- FAR struct tcb_s *htcb; /* Holder TCB */
+ struct semholder_s *tlink; /* List of task held semphores */
Review comment:
```suggestion
FAR struct semholder_s *tlink; /* List of task held semphores */
```
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1200,4 +1211,31 @@ int nxsem_nfreeholders(void)
}
#endif
+/****************************************************************************
+ * Name: nxsem_release_all
+ *
+ * Description:
+ * Release all semphore holders for the task.
+ *
+ * Input Parameters:
+ * stcb - TCB of the task
+ *
+ * Returned Value:
+ * None
+ *
+ * Assumptions:
+ *
+ ****************************************************************************/
+
+void nxsem_release_all(FAR struct tcb_s *htcb)
+{
+ while (htcb->holdsem != NULL)
+ {
+ FAR struct semholder_s *pholder = htcb->holdsem;
+ FAR sem_t *sem = pholder->sem;
+
+ nxsem_freeholder(sem, pholder);
+ }
+}
Review comment:
Minor
```suggestion
void nxsem_release_all(FAR struct tcb_s *htcb)
{
FAR struct semholder_s *pholder;
while ((pholder = htcb->holdsem) != NULL)
{
FAR sem_t *sem = pholder->sem;
nxsem_freeholder(sem, pholder);
}
}
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779978977
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
Holder[1]'s count is not necessarily 0 when holder[0]'s count is 0.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779979899
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
Review comment:
Holder's count does not need to be checked because it will be freed in nxsem_restore_baseprio if it's count is 0.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782787550
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
Review comment:
Yes. I understand. I just see that in all other places decision in done based on `htcb`, so suggest a common approach. We can add instrumentation to assert if `htcb != NULL` and `counts <= 0` to catch possible bugs. The same for `htcb == NULL` and `counts > 0`
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782910279
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +945,58 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
Review comment:
Applied, thanks!
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -66,7 +66,8 @@ static FAR struct semholder_s *g_freeholders;
* Name: nxsem_allocholder
****************************************************************************/
-static inline FAR struct semholder_s *nxsem_allocholder(sem_t *sem)
+static inline FAR struct semholder_s *nxsem_allocholder(FAR sem_t *sem,
+ FAR struct tcb_s *htcb)
Review comment:
Applied, thanks!
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] hartmannathan commented on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1016881374
> @hartmannathan please merge this PR after you finish review.
Hi, unfortunately I became busy with other work. I could try to circle back to this in a day or two, or you can go ahead and merge it if you feel confident. Thanks
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1016693119
@hartmannathan please merge this PR after you finish review.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779510028
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
```suggestion
for (i = 0, pholder = sem->holder;
i < 2 && pholder->counts > 0;
i++, pholder++)
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r780196314
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
If the holder's counts == 0, it will be removed from the list later in nxsem_restore_baseprio, so all holder's counts must > 0 when enter nxsem_release_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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r780675912
##########
File path: libs/libc/semaphore/sem_init.c
##########
@@ -78,10 +78,8 @@ int nxsem_init(FAR sem_t *sem, int pshared, unsigned int value)
# if CONFIG_SEM_PREALLOCHOLDERS > 0
sem->hhead = NULL;
# else
- sem->holder[0].htcb = NULL;
- sem->holder[0].counts = 0;
- sem->holder[1].htcb = NULL;
- sem->holder[1].counts = 0;
+ sem->holder[0] = (struct semholder_s) SEMHOLDER_INITIALIZER;
+ sem->holder[1] = (struct semholder_s) SEMHOLDER_INITIALIZER;
Review comment:
I'm not sure if this is allowed with C89. Please check and if it is not supported, then we need to rework this 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782775416
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
Review comment:
Same effect, the counts must > 0 if pholder->htcb != NULL, vice versa. The following code will access counts, so select judgment counts here.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782849747
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -66,7 +66,8 @@ static FAR struct semholder_s *g_freeholders;
* Name: nxsem_allocholder
****************************************************************************/
-static inline FAR struct semholder_s *nxsem_allocholder(sem_t *sem)
+static inline FAR struct semholder_s *nxsem_allocholder(FAR sem_t *sem,
+ FAR struct tcb_s *htcb)
Review comment:
```suggestion
FAR struct tcb_s *htcb)
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r783045941
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -954,11 +867,8 @@ void nxsem_add_holder_tcb(FAR struct tcb_s *htcb, FAR sem_t *sem)
pholder = nxsem_findorallocateholder(sem, htcb);
if (pholder != NULL)
{
- /* Then set the holder and increment the number of counts held by
- * this holder
- */
+ /* Increment the number of counts held by this holder */
- pholder->htcb = htcb;
pholder->counts++;
Review comment:
I still believe that this will bring bugs in case of 1 producer and 2 consumers signaling model. For example there are 3 tasks. Task 1 does `sem_post` and Task 2 and 3 are calling `sem_wait`. For example Task 1 is the highest priority and does 2 `sem_post` in a row. The Task 2 consumes 1 count and does some job and Task 3 consumes 1 count and does some work. Then it is repeated cyclically. In this case Task 2 and 3 will be sem holders always (because we do not increment `pholders->counts` in `nxsem_release_holders`) and `pholders->counts` will overflow at some point and `nxsem_freecount0holder` or `nxsem_restoreholderprio` will finally execute `nxsem_freeholder(sem, pholder);` so all the system will be in the undefined behavior.
I really think that we need to limit `pholder->counts` with `SEM_VALUE_MAX` and have here something like
```
if (pholder->counts < SEM_VALUE_MAX)
{
pholder->counts++;
}
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r781715865
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -66,7 +66,8 @@ static FAR struct semholder_s *g_freeholders;
* Name: nxsem_allocholder
****************************************************************************/
-static inline FAR struct semholder_s *nxsem_allocholder(sem_t *sem)
+static inline FAR struct semholder_s *nxsem_allocholder(sem_t *sem,
Review comment:
Applied, thanks!
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -187,61 +194,67 @@ nxsem_findorallocateholder(sem_t *sem, FAR struct tcb_s *htcb)
static inline void nxsem_freeholder(sem_t *sem,
Review comment:
Applied, thanks!
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#issuecomment-1032713002
> CI system encountered problems, how to restart build on macOS? @xiaoxiang781216
Two methods:
1. Push your patch with -f again(you can reset hard the master head if you can't change anything)
2. Or let maintain restart CI for you like I am doing 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779510028
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
```suggestion
for (i = 0, pholder = &sem->holder[0];
i < 2 && pholder->counts > 0;
i++, pholder++)
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779510028
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
Review comment:
```suggestion
for (i = 0, pholder = &sem->holder[i];
i < 2 && pholder->counts > 0;
i++)
```
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782816047
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +944,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
+#else
+ int i;
+
+ /* We have two hard-allocated holder structures in sem_t */
+
+ for (i = 0; i < 2; i++)
+#endif
+ {
+#if CONFIG_SEM_PREALLOCHOLDERS == 0
+ pholder = &sem->holder[i];
+ if (pholder->counts <= 0)
+ continue;
Review comment:
OK. The above is written incorrectly, add DEBUGASSERT(pholder->htcb != NULL) if pholder->counts > 0.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r782747575
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -263,23 +276,16 @@ static int nxsem_foreachholder(FAR sem_t *sem, holderhandler_t handler,
next = pholder->flink;
Review comment:
No, because the holder maybe freed in the handler callback, so we must preserve pointer to the next 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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r783780545
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -954,11 +867,8 @@ void nxsem_add_holder_tcb(FAR struct tcb_s *htcb, FAR sem_t *sem)
pholder = nxsem_findorallocateholder(sem, htcb);
if (pholder != NULL)
{
- /* Then set the holder and increment the number of counts held by
- * this holder
- */
+ /* Increment the number of counts held by this holder */
- pholder->htcb = htcb;
pholder->counts++;
Review comment:
Applied, thanks!
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r781423817
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -66,7 +66,8 @@ static FAR struct semholder_s *g_freeholders;
* Name: nxsem_allocholder
****************************************************************************/
-static inline FAR struct semholder_s *nxsem_allocholder(sem_t *sem)
+static inline FAR struct semholder_s *nxsem_allocholder(sem_t *sem,
Review comment:
add FAR before sem_t
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -187,61 +194,67 @@ nxsem_findorallocateholder(sem_t *sem, FAR struct tcb_s *htcb)
static inline void nxsem_freeholder(sem_t *sem,
Review comment:
Add FAR before sem_t
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779979899
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -1035,18 +1014,57 @@ void nxsem_release_holder(FAR sem_t *sem)
{
FAR struct tcb_s *rtcb = this_task();
FAR struct semholder_s *pholder;
+ FAR struct semholder_s *candidate = NULL;
+ unsigned int total = 0;
/* Find the container for this holder */
- pholder = nxsem_findholder(sem, rtcb);
- if (pholder != NULL && pholder->counts > 0)
+#if CONFIG_SEM_PREALLOCHOLDERS > 0
+ for (pholder = sem->hhead; pholder; pholder = pholder->flink)
Review comment:
Holder's count does not need to be checked because it will be freed in nxsem_restore_baseprio if it's count is 0.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779272504
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -187,35 +194,36 @@ nxsem_findorallocateholder(sem_t *sem, FAR struct tcb_s *htcb)
static inline void nxsem_freeholder(sem_t *sem,
FAR struct semholder_s *pholder)
{
-#if CONFIG_SEM_PREALLOCHOLDERS > 0
- FAR struct semholder_s *curr;
- FAR struct semholder_s *prev;
-#endif
+ FAR struct semholder_s **curr;
Review comment:
Applied, thanks!
##########
File path: include/semaphore.h
##########
@@ -58,19 +58,23 @@
#ifdef CONFIG_PRIORITY_INHERITANCE
struct tcb_s; /* Forward reference */
+struct sem_s;
+
struct semholder_s
{
#if CONFIG_SEM_PREALLOCHOLDERS > 0
- struct semholder_s *flink; /* Implements singly linked list */
+ struct semholder_s *flink; /* List of semphore's holder */
Review comment:
Applied, thanks!
##########
File path: include/semaphore.h
##########
@@ -58,19 +58,23 @@
#ifdef CONFIG_PRIORITY_INHERITANCE
struct tcb_s; /* Forward reference */
+struct sem_s;
+
struct semholder_s
{
#if CONFIG_SEM_PREALLOCHOLDERS > 0
- struct semholder_s *flink; /* Implements singly linked list */
+ struct semholder_s *flink; /* List of semphore's holder */
#endif
- FAR struct tcb_s *htcb; /* Holder TCB */
+ struct semholder_s *tlink; /* List of task held semphores */
Review comment:
Applied, thanks!
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r779019294
##########
File path: sched/semaphore/sem_holder.c
##########
@@ -86,30 +87,36 @@ static inline FAR struct semholder_s *nxsem_allocholder(sem_t *sem)
g_freeholders = pholder->flink;
pholder->flink = sem->hhead;
sem->hhead = pholder;
-
- /* Make sure the initial count is zero */
-
- pholder->counts = 0;
}
#else
if (sem->holder[0].htcb == NULL)
{
pholder = &sem->holder[0];
- pholder->counts = 0;
}
else if (sem->holder[1].htcb == NULL)
{
pholder = &sem->holder[1];
- pholder->counts = 0;
}
#endif
else
{
serr("ERROR: Insufficient pre-allocated holders\n");
pholder = NULL;
+ DEBUGASSERT(pholder != NULL);
Review comment:
`DEBUGASSERT(pholder != NULL);` here does not make any sense. It is equivalent to `DEBUGASSERT(0);`.
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] zhaoxiu-zeng commented on a change in pull request #5171: semphore: release all semphores' holder that the task held when exit
Posted by GitBox <gi...@apache.org>.
zhaoxiu-zeng commented on a change in pull request #5171:
URL: https://github.com/apache/incubator-nuttx/pull/5171#discussion_r780867709
##########
File path: libs/libc/semaphore/sem_init.c
##########
@@ -78,10 +78,8 @@ int nxsem_init(FAR sem_t *sem, int pshared, unsigned int value)
# if CONFIG_SEM_PREALLOCHOLDERS > 0
sem->hhead = NULL;
# else
- sem->holder[0].htcb = NULL;
- sem->holder[0].counts = 0;
- sem->holder[1].htcb = NULL;
- sem->holder[1].counts = 0;
+ sem->holder[0] = (struct semholder_s) SEMHOLDER_INITIALIZER;
+ sem->holder[1] = (struct semholder_s) SEMHOLDER_INITIALIZER;
Review comment:
Reworked, thanks!
--
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-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org