You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "pkarashchenko (via GitHub)" <gi...@apache.org> on 2023/04/03 14:18:05 UTC
[GitHub] [nuttx] pkarashchenko opened a new pull request, #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
pkarashchenko opened a new pull request, #8951:
URL: https://github.com/apache/nuttx/pull/8951
## Summary
Optimize semaphore holder check for priority inheritance
## Impact
Should be none. Code clean-up
## Testing
ostest
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1495529517
Just reproduced the crash
```
[CPU0] _assert: Current Version: NuttX 3.6.1 7b6a424c6d Apr 4 2023 10:56:00 arm
[CPU0] _assert: Assertion failed panic: at file: irq/irq_unexpectedisr.c:54 task(CPU0): CPU0 IDLE 0xd003339
[CPU0] up_dump_register: R0: 2d02bdc4 R1: 2d02bdb8 R2: 00154100 R3: 001540e0
[CPU0] up_dump_register: R4: 2d02bdb8 R5: 0d004861 R6: 00000000 FP: 00000001
[CPU0] up_dump_register: R8: 2d0298dc SB: 2d029a1c SL: 2d029a6c R11: 0d003339
[CPU0] up_dump_register: IP: 00000001 SP: 2d02bd40 LR: 0d00486f PC: 0d0047b0
[CPU0] up_dump_register: xPSR: a1000000 BASEPRI: 00000080 CONTROL: 00000000
[CPU0] up_dump_register: EXC_RETURN: ffffffe9
[CPU0] dump_stack: IRQ Stack:
[CPU0] dump_stack: sp: 0x2d028d68
[CPU0] dump_stack: base: 0x2d028650
[CPU0] dump_stack: size: 00002048
[CPU0] stack_dump: 0x2d028d60: 0d003339 00000000 00000000 7474754e 00000058 00000000 00000000 00000000
[CPU0] stack_dump: 0x2d028d80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[CPU0] stack_dump: 0x2d028da0: 362e3300 0000312e 00000000 00000000 00000000 62370000 32346136 64366334
[CPU0] stack_dump: 0x2d028dc0: 72704120 20342020 33323032 3a303120 303a3635 00000030 00000000 00000000
[CPU0] stack_dump: 0x2d028de0: 00000000 00000000 6d726100 00000000 00000000 00000000 00000000 00000000
[CPU0] stack_dump: 0x2d028e00: 00000000 00000000 2d0297b8 00000003 00000000 2d0298dc 2d029a1c 0d007e17
[CPU0] stack_dump: 0x2d028e20: 00000080 0d00367f 0d003669 0d00363f 00000000 0d001797 0d01cdbc 2d02bdb8
[CPU0] dump_stack: User Stack:
[CPU0] dump_stack: sp: 0x2d02bd40
[CPU0] dump_stack: base: 0
[CPU0] dump_stack: size: 00000000
[CPU0] dump_stack: ERROR: User Stack pointer is not within the stack
[CPU0] show_tasks: PID GROUP CPU PRI POLICY TYPE NPX STATE EVENT SIGMASK STACKBASE STACKSIZE COMMAND
[CPU0] show_tasks: ---- --- ---- --- -------- ------- --- ------- ---------- -------- 0x2d028650 2048 irq
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on pull request #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1502860280
@masayuki2009 could you please merge this PR as I do not see more activity 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] [nuttx] xiaoxiang781216 commented on a diff in pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#discussion_r1158560037
##########
sched/semaphore/sem_holder.c:
##########
@@ -891,6 +789,8 @@ void nxsem_restore_baseprio(FAR struct tcb_s *stcb, FAR sem_t *sem)
(sem->semcount <= 0 && stcb != NULL));
#endif
+ DEBUGASSERT(!up_interrupt_context());
Review Comment:
why can't be called from the interrupt context?
##########
sched/semaphore/sem_holder.c:
##########
@@ -792,61 +697,54 @@ void nxsem_boost_priority(FAR sem_t *sem)
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;
+ uint8_t prioinherit = sem->flags & SEM_PRIO_MASK;
+
+ /* If priority inheritance is disabled for this thread or it is IDLE
+ * thread, then do not add the holder.
+ * If there are never holders of the semaphore, the priority
+ * inheritance is effectively disabled.
+ */
+
+ if (!is_idle_task(rtcb) && prioinherit == SEM_PRIO_INHERIT)
Review Comment:
why skip in the idle task?
##########
sched/semaphore/sem_holder.c:
##########
@@ -792,61 +697,54 @@ void nxsem_boost_priority(FAR sem_t *sem)
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;
+ uint8_t prioinherit = sem->flags & SEM_PRIO_MASK;
+
+ /* If priority inheritance is disabled for this thread or it is IDLE
+ * thread, then do not add the holder.
+ * If there are never holders of the semaphore, the priority
+ * inheritance is effectively disabled.
+ */
+
+ if (!is_idle_task(rtcb) && prioinherit == SEM_PRIO_INHERIT)
+ {
+ FAR struct semholder_s *pholder;
- /* Find the container for this holder */
+ DEBUGASSERT(!up_interrupt_context());
Review Comment:
ditto
--
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] davids5 commented on pull request #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1578639029
@pkarashchenko We ran in to an issue with an older version of the PI code and I am wondering if it exits in the latest code?
sched_lock() is not respected on a call to sem_post that reprioritizes
Causing code like following to not work
```
sched_lock();
nxsem_post(&dev->d_bfsem); ------------>Swapped out if there is a higher priority waiter.
ret = nxsem_wait(&dev->d_wrsem);
sched_unlock();
```
Has this been fixed? If so, can you point me to the mitigation?
--
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 #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#discussion_r1158643351
##########
sched/semaphore/sem_holder.c:
##########
@@ -792,61 +697,54 @@ void nxsem_boost_priority(FAR sem_t *sem)
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;
+ uint8_t prioinherit = sem->flags & SEM_PRIO_MASK;
+
+ /* If priority inheritance is disabled for this thread or it is IDLE
+ * thread, then do not add the holder.
+ * If there are never holders of the semaphore, the priority
+ * inheritance is effectively disabled.
+ */
+
+ if (!is_idle_task(rtcb) && prioinherit == SEM_PRIO_INHERIT)
+ {
+ FAR struct semholder_s *pholder;
- /* Find the container for this holder */
+ DEBUGASSERT(!up_interrupt_context());
Review Comment:
In general we can't post semaphore with priority inheritance enabled from the interrupt
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1496486630
Please revert https://github.com/apache/nuttx/pull/8938 and apply your change after 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] [nuttx] acassis commented on pull request #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1500394267
> Current Version: NuttX 3.6.1
Any idea why it is happening? Seems there is some issue in our current script and sometimes when we aren't in the upstream branch it get confused.
--
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 pull request #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1499422585
@patacongo may I get your approval on this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on pull request #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1578718293
@davids5 I expect this to be fixed by https://github.com/apache/nuttx/pull/7464 and later optimised by https://github.com/apache/nuttx/pull/9039
But above example will be working only in non-SMP case since `sched_lock()/sched_unlock()` will not work if threads are running on the different cores. Or you need to add `pthread_attr_setaffinity_np()` for two pthreads and bind them to the same CPU.
I will prioritise addressing all comments from https://github.com/apache/nuttx-apps/pull/1711 this week as all prerequisites seems to be met.
--
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 #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#discussion_r1157655350
##########
sched/semaphore/sem_holder.c:
##########
@@ -102,21 +102,17 @@ nxsem_allocholder(FAR sem_t *sem, FAR struct tcb_s *htcb)
else
{
serr("ERROR: Insufficient pre-allocated holders\n");
- pholder = NULL;
- DEBUGPANIC();
+ PANIC();
}
- if (pholder != NULL)
- {
- pholder->sem = sem;
- pholder->htcb = htcb;
- pholder->counts = 0;
+ pholder->sem = sem;
Review Comment:
remove the extra sapce before = from line 88-99
--
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 #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#discussion_r1158590876
##########
sched/semaphore/sem_holder.c:
##########
@@ -891,6 +789,8 @@ void nxsem_restore_baseprio(FAR struct tcb_s *stcb, FAR sem_t *sem)
(sem->semcount <= 0 && stcb != NULL));
#endif
+ DEBUGASSERT(!up_interrupt_context());
Review Comment:
The same as `nxsem_checkholder` -> `if (up_interrupt_context()) { return false; }`
##########
sched/semaphore/sem_holder.c:
##########
@@ -792,61 +697,54 @@ void nxsem_boost_priority(FAR sem_t *sem)
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;
+ uint8_t prioinherit = sem->flags & SEM_PRIO_MASK;
+
+ /* If priority inheritance is disabled for this thread or it is IDLE
+ * thread, then do not add the holder.
+ * If there are never holders of the semaphore, the priority
+ * inheritance is effectively disabled.
+ */
+
+ if (!is_idle_task(rtcb) && prioinherit == SEM_PRIO_INHERIT)
Review Comment:
Improved
```
tid = gettid();
if (tid == 0)
{
return true;
}
```
from `nxsem_checkholder`.
I would even say that we can think about assertion if idle task tries to get semaphore with priority inheritance, but that is up to discussion.
##########
sched/semaphore/sem_holder.c:
##########
@@ -792,61 +697,54 @@ void nxsem_boost_priority(FAR sem_t *sem)
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;
+ uint8_t prioinherit = sem->flags & SEM_PRIO_MASK;
+
+ /* If priority inheritance is disabled for this thread or it is IDLE
+ * thread, then do not add the holder.
+ * If there are never holders of the semaphore, the priority
+ * inheritance is effectively disabled.
+ */
+
+ if (!is_idle_task(rtcb) && prioinherit == SEM_PRIO_INHERIT)
+ {
+ FAR struct semholder_s *pholder;
- /* Find the container for this holder */
+ DEBUGASSERT(!up_interrupt_context());
Review Comment:
The same as `nxsem_checkholder` -> `if (up_interrupt_context()) { return false; }`
--
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] davids5 commented on a diff in pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on code in PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#discussion_r1157074834
##########
sched/semaphore/sem_holder.c:
##########
@@ -103,20 +103,18 @@ nxsem_allocholder(FAR sem_t *sem, FAR struct tcb_s *htcb)
{
serr("ERROR: Insufficient pre-allocated holders\n");
pholder = NULL;
- DEBUGPANIC();
}
- if (pholder != NULL)
- {
- pholder->sem = sem;
- pholder->htcb = htcb;
- pholder->counts = 0;
+ DEBUGASSERT(pholder != NULL);
Review Comment:
If we can run out of holder 2 ways here. Why leave this debug detection only? The following code will corrupt memory. Not having enough holders, if it were survivable will still cause PI issues,
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1495587971
@masayuki2009 seems like we have some issue with the build system. The map file generated after `make menuconfig` and after `make clean` with the sam config are different (and binaries as well)
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1495492848
It is strange. I just rebuilt the code and can't reproduce the crash. I will do more experiments.
--
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] davids5 commented on pull request #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "davids5 (via GitHub)" <gi...@apache.org>.
davids5 commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1500801963
@acassis Fetch the tags.
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1495592669
@xiaoxiang781216 do you have any idea how we can debug this? I mean is that an issue of a build system or some interference with compiler version.
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1494420526
Just drafted changes, but really didn't have time to run ostest. I would appreciate someone helping with that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-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 #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#discussion_r1158307342
##########
sched/semaphore/sem_holder.c:
##########
@@ -102,21 +102,17 @@ nxsem_allocholder(FAR sem_t *sem, FAR struct tcb_s *htcb)
else
{
serr("ERROR: Insufficient pre-allocated holders\n");
- pholder = NULL;
- DEBUGPANIC();
+ PANIC();
}
- if (pholder != NULL)
- {
- pholder->sem = sem;
- pholder->htcb = htcb;
- pholder->counts = 0;
+ pholder->sem = sem;
Review Comment:
Done
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1494422906
Just drafted changes, but really didn't have time to run ostest. I would appreciate someone helping with that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] masayuki2009 commented on pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1495545586
> Just reproduced the crash
@pkarashchenko
What version of the arm gcc are you using?
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1496467067
> @xiaoxiang781216 do you have any idea how we can debug this? I mean is that an issue of a build system or some interference with compiler version.
I never expect that the incremental build will work after you change config. My suggestion is always to clean and rebuild the image in this case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on pull request #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1498777650
@xiaoxiang781216 please take a look if your question are answered. 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] [nuttx] pkarashchenko commented on pull request #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1500556103
> > Current Version: NuttX 3.6.1
>
> Any idea why it is happening? Seems there is some issue in our current script and sometimes when we aren't in the upstream branch it get confused.
I have no idea, but this is what I get from building the latest master
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1495745894
I will fill a ticket related to build problem. Anyway it is not directly related to this PR
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1495548897
> > Just reproduced the crash
>
> @pkarashchenko What version of the arm gcc are you using?
`arm-none-eabi-gcc (Arm GNU Toolchain 12.2.Rel1 (Build arm-12.24)) 12.2.1 20221205`
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1497237898
> Please revert #8938 and apply your change after it.
Done
--
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 #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#discussion_r1158761171
##########
sched/semaphore/sem_holder.c:
##########
@@ -792,61 +697,54 @@ void nxsem_boost_priority(FAR sem_t *sem)
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;
+ uint8_t prioinherit = sem->flags & SEM_PRIO_MASK;
+
+ /* If priority inheritance is disabled for this thread or it is IDLE
+ * thread, then do not add the holder.
+ * If there are never holders of the semaphore, the priority
+ * inheritance is effectively disabled.
+ */
+
+ if (!is_idle_task(rtcb) && prioinherit == SEM_PRIO_INHERIT)
+ {
+ FAR struct semholder_s *pholder;
- /* Find the container for this holder */
+ DEBUGASSERT(!up_interrupt_context());
Review Comment:
Yes, I think so. After we add more check, the semaphore with priority inheritance is more difficult to use in non-lock case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nuttx] pkarashchenko commented on pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1495458311
I noticed one strange thing. I tried to run `spresense:smp` with priority inheritance enabled and `CONFIG_SEM_PREALLOCHOLDERS == 0`, but the system crash on start-up. If I enable debug assertions the crash disappear and I can pass `ostest`. @masayuki2009 could you please take a look?
--
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 #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#discussion_r1157170933
##########
sched/semaphore/sem_holder.c:
##########
@@ -103,20 +103,18 @@ nxsem_allocholder(FAR sem_t *sem, FAR struct tcb_s *htcb)
{
serr("ERROR: Insufficient pre-allocated holders\n");
pholder = NULL;
- DEBUGPANIC();
}
- if (pholder != NULL)
- {
- pholder->sem = sem;
- pholder->htcb = htcb;
- pholder->counts = 0;
+ DEBUGASSERT(pholder != NULL);
Review Comment:
Done
--
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 commented on pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1495585959
>It is strange. I just rebuilt the code and can't reproduce the crash. I will do more experiments.
@pkarashchenko
I tried this PR with
```
--- a/boards/arm/cxd56xx/spresense/configs/smp/defconfig
+++ b/boards/arm/cxd56xx/spresense/configs/smp/defconfig
@@ -40,12 +40,14 @@ CONFIG_NSH_ARCHINIT=y
CONFIG_NSH_BUILTIN_APPS=y
CONFIG_NSH_READLINE=y
CONFIG_PREALLOC_TIMERS=4
+CONFIG_PRIORITY_INHERITANCE=y
CONFIG_RAM_SIZE=1572864
CONFIG_RAM_START=0x0d000000
CONFIG_READLINE_CMD_HISTORY=y
CONFIG_RR_INTERVAL=200
CONFIG_RTC=y
CONFIG_RTC_DRIVER=y
+CONFIG_SEM_PREALLOCHOLDERS=0
CONFIG_SMP=y
CONFIG_SMP_NCPUS=2
CONFIG_SPI=y
$ arm-none-eabi-gcc --version
arm-none-eabi-gcc (Arm GNU Toolchain 12.2.MPACBTI-Rel1 (Build arm-12-mpacbti.34)) 12.2.1 20230214
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
```
And the ostest passed without any errors.
```
NuttShell (NSH) NuttX-3.6.1
nsh> uname -a
NuttX 3.6.1 7b6a424c6d-dirty Apr 4 2023 17:35:14 arm spresense
nsh> ps
PID GROUP CPU PRI POLICY TYPE NPX STATE EVENT SIGMASK STACK COMMAND
0 0 0 0 FIFO Kthread N-- Assigned 0000000000000000 001000 CPU0 IDLE
1 1 1 0 FIFO Kthread N-- Running 0000000000000000 001000 CPU1 IDLE
3 3 --- 200 RR Task --- Waiting MQ empty 0000000000000000 000976 cxd56_pm_task
4 4 0 100 RR Task --- Running 0000000000000000 002000 spresense_main
nsh> free
total used free largest nused nfree
Umem: 1392840 15744 1377096 1374864 77 3
nsh> ostest
stdio_test: write fd=1
stdio_test: Standard I/O Check: printf
stdio_test: write fd=2
...
user_main: priority inheritance test
priority_inheritance: Started
priority_inheritance: Starting lowpri_thread-1 (of 1) at 1
priority_inheritance: Set lowpri_thread-1 priority to 1
priority_inheritance: Waiting...
lowpri_thread-1: Started
lowpri_thread-1: initial priority: 1
lowpri_thread-1: Waiting for the midle pri task to run
g_middlestate: 0
g_highstate[0]: 0
I still have a count on the semaphore
lowpri_thread-1: Waiting for the midle pri task to run
g_middlestate: 0
g_highstate[0]: 0
I still have a count on the semaphore
priority_inheritance: Starting medpri_thread at 99
priority_inheritance: Set medpri_thread priority to 99
priority_inheritance: Waiting...
medpri_thread: Started ... I won't let go of the CPU!
lowpri_thread-1: Sem count: 0, No. highpri thread: 0
lowpri_thread-1: SUCCESS priority before sem_post: 1
lowpri_thread-1: SUCCESS final priority: 1
lowpri_thread-1: Okay... I'm done!
priority_inheritance: Starting highpri_thread-1 (of 1) at 255
priority_inheritance: Set highpri_thread-1 priority to 255
priority_inheritance: Waiting for highpri_thread-1 to complete
highpri_thread-1: Started
highpri_thread-1: Calling sem_wait()
highpri_thread-1: SUCCESS midpri_thread is still running!
highpri_thread-1: Okay... I'm done!
priority_inheritance: Waiting for medpri_thread to complete
medpri_thread: Okay... I'm done!
priority_inheritance: Waiting for lowpri_thread-1 to complete
priority_inheritance: Restoration Test:
priority_inheritance: Task0 initial priority is:200
priority_inheritance: Task1 initial priority is:210
priority_inheritance: Waiting for Task-0 to complete
priority_inheritance: Task2 initial priority is:220
priority_inheritance: Task0 Started, waiting 0 uS to take count
priority_inheritance: Task1 Started, waiting 10000 uS to take count
priority_inheritance: Task2 Started, waiting 20000 uS to take count
priority_inheritance: Task0 Posted
priority_inheritance: Task0 priority was:200 is:200
priority_inheritance: Waiting for Task-1 to complete
priority_inheritance: Task2 Posted
priority_inheritance: Task2 priority was:220 is:220
priority_inheritance: Task1 Posted
priority_inheritance: Task1 priority was:210 is:210
priority_inheritance: Waiting for Task-2 to complete
priority_inheritance: PASSED Priority were correctly restored.
priority_inheritance: Finished
End of test memory usage:
VARIABLE BEFORE AFTER
======== ======== ========
arena 1540c8 1540c8
ordblks 11 12
mxordblk 143ed8 143ed8
uordblks 8520 a828
fordblks 14bba8 1498a0
user_main: vfork() test
vfork_test: Child 94 ran successfully
Final memory usage:
VARIABLE BEFORE AFTER
======== ======== ========
arena 1540c8 1540c8
ordblks 4 12
mxordblk 14b720 143ed8
uordblks 74e8 c028
fordblks 14cbe0 1480a0
user_main: Exiting
ostest_main: Exiting with status 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] [nuttx] pkarashchenko commented on a diff in pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#discussion_r1157158812
##########
sched/semaphore/sem_holder.c:
##########
@@ -103,20 +103,18 @@ nxsem_allocholder(FAR sem_t *sem, FAR struct tcb_s *htcb)
{
serr("ERROR: Insufficient pre-allocated holders\n");
pholder = NULL;
- DEBUGPANIC();
}
- if (pholder != NULL)
- {
- pholder->sem = sem;
- pholder->htcb = htcb;
- pholder->counts = 0;
+ DEBUGASSERT(pholder != NULL);
Review Comment:
I just didn't want to change existing behaviour that use `DEBUGPANIC();` and not `PANIC();`. But I agree that `ASSERT(pholder != NULL);` might be better 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] [nuttx] pkarashchenko commented on a diff in pull request #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on code in PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#discussion_r1158840012
##########
sched/semaphore/sem_holder.c:
##########
@@ -792,61 +697,54 @@ void nxsem_boost_priority(FAR sem_t *sem)
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;
+ uint8_t prioinherit = sem->flags & SEM_PRIO_MASK;
+
+ /* If priority inheritance is disabled for this thread or it is IDLE
+ * thread, then do not add the holder.
+ * If there are never holders of the semaphore, the priority
+ * inheritance is effectively disabled.
+ */
+
+ if (!is_idle_task(rtcb) && prioinherit == SEM_PRIO_INHERIT)
+ {
+ FAR struct semholder_s *pholder;
- /* Find the container for this holder */
+ DEBUGASSERT(!up_interrupt_context());
Review Comment:
The question is only where we should check it. Semaphore holders APIs are internal APIs and interrupt context check may be done once in nxsem_post instead of two times like in this PR. On the other hand the current approach will catch possible misuse of semaphore holders APIs.
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1495535327
And seems like I have the steps to reproduce:
1. `tools/configure.sh spresense:smp`
2. `make menuconfig` -- and enable priority inheritance
3. `make -j8` -- all is working now
4. `make menuconfig` -- and set `CONFIG_SEM_PREALLOCHOLDERS` to `0`
5. `make -j8` -- now I'm getting the crash
6. `make clean`
7. `make -j8` -- now all is working
So it seems like some dependencies are not refreshed with `make menuconfig` and build system is using object files from the previous configuration. I will try to compare binaries and map files after steps 5 and 7.
--
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 pull request #8951: sched/semaphore: optimize semaphore holder check for priority inheritance
Posted by "pkarashchenko (via GitHub)" <gi...@apache.org>.
pkarashchenko commented on PR #8951:
URL: https://github.com/apache/nuttx/pull/8951#issuecomment-1497240705
> > @xiaoxiang781216 do you have any idea how we can debug this? I mean is that an issue of a build system or some interference with compiler version.
>
> I never expect that the incremental build will work after you change config. My suggestion is always to clean and rebuild the image in this case.
Ok. Makes sense. I'll close the issue. Most of the code is rebuilt because of `#include <nuttx/config.h>`, but there are some files that do not include it directly, so stale objects are used.
--
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 #8951: sched/semaphore: rework semaphore holder check for priority inheritance
Posted by "masayuki2009 (via GitHub)" <gi...@apache.org>.
masayuki2009 merged PR #8951:
URL: https://github.com/apache/nuttx/pull/8951
--
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