You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "anchao (via GitHub)" <gi...@apache.org> on 2023/01/28 05:44:31 UTC
[GitHub] [nuttx] anchao opened a new pull request, #8321: sched/wqueue: semaphore count should be consistent with the number of work entries.
anchao opened a new pull request, #8321:
URL: https://github.com/apache/nuttx/pull/8321
## Summary
1. sched/wqueue: semaphore count should be consistent with the number of work entries.
The number of work entries will be inconsistent with semaphore count
if the work is canceled, in extreme case, semaphore count will overflow
and fallback to 0 the workqueue will stop scheduling the enqueue work.
2. sched/semaphore: correct the return value of sem_post()
sem_post() should return EOVERFLOW if maximum allowable value for
a semaphore would be exceeded.
Reference:
https://man7.org/linux/man-pages/man3/sem_post.3.html
## Impact
N/A
## Testing
sabre-6quad/netnsh_wb
--
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] [nuttx] anchao commented on a diff in pull request #8321: sched/wqueue: semaphore count should be consistent with the number of work entries.
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #8321:
URL: https://github.com/apache/nuttx/pull/8321#discussion_r1089656793
##########
sched/semaphore/sem_post.c:
##########
@@ -89,7 +89,11 @@ int nxsem_post(FAR sem_t *sem)
/* Check the maximum allowable value */
- DEBUGASSERT(sem_count < SEM_VALUE_MAX);
+ if (sem_count >= SEM_VALUE_MAX)
+ {
+ leave_critical_section(flags);
+ return -EOVERFLOW;
Review Comment:
Freebsd also has similar description about EOVERFLOW
https://man.freebsd.org/cgi/man.cgi?query=sem_post&sektion=3
--
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] [nuttx] pkarashchenko commented on a diff in pull request #8321: sched/wqueue: semaphore count should be consistent with the number of work entries.
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8321:
URL: https://github.com/apache/nuttx/pull/8321#discussion_r1089653048
##########
sched/semaphore/sem_post.c:
##########
@@ -89,7 +89,11 @@ int nxsem_post(FAR sem_t *sem)
/* Check the maximum allowable value */
- DEBUGASSERT(sem_count < SEM_VALUE_MAX);
+ if (sem_count >= SEM_VALUE_MAX)
+ {
+ leave_critical_section(flags);
+ return -EOVERFLOW;
Review Comment:
This violates POSIX. `sem_post` can't set errno to `EOVERFLOW`.
--
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] [nuttx] xiaoxiang781216 commented on a diff in pull request #8321: sched/wqueue: semaphore count should be consistent with the number of work entries.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8321:
URL: https://github.com/apache/nuttx/pull/8321#discussion_r1089682358
##########
sched/semaphore/sem_post.c:
##########
@@ -89,7 +89,11 @@ int nxsem_post(FAR sem_t *sem)
/* Check the maximum allowable value */
- DEBUGASSERT(sem_count < SEM_VALUE_MAX);
+ if (sem_count >= SEM_VALUE_MAX)
+ {
+ leave_critical_section(flags);
+ return -EOVERFLOW;
Review Comment:
I am wondering that whether the implementation can just allow to return the error code specified by spec or have some flexibility to return some additional error code to report the implementation detail. For example, most sem_t implementation has at least 32bit count which is very hard to hit the overflow issue, but NuttX use 16bit count and then hit this problem more frequenctly than others.
--
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] [nuttx] pkarashchenko commented on a diff in pull request #8321: sched/wqueue: semaphore count should be consistent with the number of work entries.
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8321:
URL: https://github.com/apache/nuttx/pull/8321#discussion_r1089666691
##########
sched/semaphore/sem_post.c:
##########
@@ -89,7 +89,11 @@ int nxsem_post(FAR sem_t *sem)
/* Check the maximum allowable value */
- DEBUGASSERT(sem_count < SEM_VALUE_MAX);
+ if (sem_count >= SEM_VALUE_MAX)
+ {
+ leave_critical_section(flags);
+ return -EOVERFLOW;
Review Comment:
While QNX does not have it :) http://www.qnx.com/developers/docs/7.0.0/index.html#com.qnx.doc.neutrino.lib_ref/topic/s/sem_post.html
Let's discuss, or maybe spin a vote in mailing list
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] masayuki2009 merged pull request #8321: sched/wqueue: semaphore count should be consistent with the number of work entries.
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 merged PR #8321:
URL: https://github.com/apache/nuttx/pull/8321
--
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] [nuttx] anchao commented on a diff in pull request #8321: sched/wqueue: semaphore count should be consistent with the number of work entries.
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on code in PR #8321:
URL: https://github.com/apache/nuttx/pull/8321#discussion_r1089676303
##########
sched/semaphore/sem_post.c:
##########
@@ -89,7 +89,11 @@ int nxsem_post(FAR sem_t *sem)
/* Check the maximum allowable value */
- DEBUGASSERT(sem_count < SEM_VALUE_MAX);
+ if (sem_count >= SEM_VALUE_MAX)
+ {
+ leave_critical_section(flags);
+ return -EOVERFLOW;
Review Comment:
This is a regression issue, the following commit remove the overflow check:
https://github.com/apache/nuttx/pull/7372/files#diff-d574e66ae803b2d22c93c05fa66a481b3d9a9bdd4459fd16d5223ba3cebcf549L95
Some OS and libc implementations already have similar implementations.
musl:
https://github.com/bminor/musl/blob/master/src/thread/sem_post.c#L11
There is a risk of sem_count overflowing if we do not limit it, at present, sabre-6quad/netnsh_wb is broken by #7372, I think we need fix the overflow issue first.
--
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] [nuttx] xiaoxiang781216 commented on a diff in pull request #8321: sched/wqueue: semaphore count should be consistent with the number of work entries.
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8321:
URL: https://github.com/apache/nuttx/pull/8321#discussion_r1089682358
##########
sched/semaphore/sem_post.c:
##########
@@ -89,7 +89,11 @@ int nxsem_post(FAR sem_t *sem)
/* Check the maximum allowable value */
- DEBUGASSERT(sem_count < SEM_VALUE_MAX);
+ if (sem_count >= SEM_VALUE_MAX)
+ {
+ leave_critical_section(flags);
+ return -EOVERFLOW;
Review Comment:
I am wondering that whether the implementation can just allow to return the error code specified by spec or have some flexibility to return some additional error to report the implementation detail. For example, most sem_t implementation has at least 32bit count which is very hard to hit the overflow issue, but NuttX use 16bit count and then hit this problem more frequenctly than others.
--
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] [nuttx] pkarashchenko commented on a diff in pull request #8321: sched/wqueue: semaphore count should be consistent with the number of work entries.
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8321:
URL: https://github.com/apache/nuttx/pull/8321#discussion_r1089700738
##########
sched/semaphore/sem_post.c:
##########
@@ -89,7 +89,11 @@ int nxsem_post(FAR sem_t *sem)
/* Check the maximum allowable value */
- DEBUGASSERT(sem_count < SEM_VALUE_MAX);
+ if (sem_count >= SEM_VALUE_MAX)
+ {
+ leave_critical_section(flags);
+ return -EOVERFLOW;
Review Comment:
We can try to get back this implementation specific error code since it was there in the past
--
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