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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
