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 2020/04/11 15:12:40 UTC
[GitHub] [incubator-nuttx] patacongo opened a new pull request #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
patacongo opened a new pull request #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765
Hello,
I am testing priority inversion feauture of Nuttx scheduler. I encounter with problem in tests.
Here is problem:
nxsched_readytorun_setpriority() function not works as expected when DEBUG_ASSERTION not enabled. Because sched_removereadytorun() and sched_addreadytorun() is not called in this case. These functions wrapped with DEBUGASSERT macro and with the effect of DEBUGASSERT macro, sched_removereadytorun() and sched_addreadytorun() not called when DEBUG_ASSERTION not enabled. Therefore priority inversion not works as expected.
Could you check this problem ? Is this a bug, if not what is the purpose of this code ?
Thanks,
Şükrü Bahadır Arslan
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612453692
This error has been around since August 24, 2019. It was introduced by commit e1202d2ed33a257cd6d812fde8c3a77a4d7c1d01 See https://github.com/apache/incubator-nuttx/commit/e1202d2ed33a257cd6d812fde8c3a77a4d7c1d01
That commit changed all occurrences of ASSERT to DEBUGASSERT. The logical flaw in do that is that the logic withing ASSERT runs unconditionally. Changing to DEBUGASSERT causes the logic to only execute conditionally, introducing this error.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612448531
Wait do no merge yet, there is a problem with the return values. DEBUGVERIFY expects ret < 0 to indicate and error. These functions do not return an error but a boolean. it will take a little more.
OK.. It is ready to merge when it passes the checks. I had to modify the original solution. We cannot just replace DEBUGASSERT with DEBUGVERIFY. That is because DEBUGVERIFY only works with functions that return ret < 0 on an error. The functions tested do not return an error, but rather a bool indicating if a context switch occurred. So the solution in the PR has to be a little more complex to match up with these semantics.
The check if not if there is a failure in the called functions, but rather if a context switch occurred. There should be no context switch and if there is, we must ASSERT.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612664631
> I did some searching to see if there might be other instances of the same problem.
You should submit a PR. The fix is just to change the DEBUASSERT back to ASSERT. There was a wholesale change of ASSERT to DEBUGASSERT with https://github.com/apache/incubator-nuttx/commit/e1202d2ed33a257cd6d812fde8c3a77a4d7c1d01 and this introduced the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612453692
This error has been around since August 24, 2019. It was introduced by commit e1202d2ed33a257cd6d812fde8c3a77a4d7c1d01 See https://github.com/apache/incubator-nuttx/commit/e1202d2ed33a257cd6d812fde8c3a77a4d7c1d01
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612447768
Good find. Yes that is a serious problem for anyone using priority with assertions disabled. In this case, the macro DEBUGVERIFY was intended, not DEBUGASSERT.
The end result is that that if DEBUG assertions were not enabled and priority inheritance is enabled, then the ordering of task is the ready-to-run list wills be screwed up. That is is ordered from highest priority to lowest. If DEBUG assertions are not enabled, then this sequence of code changes:
251 /* Remove the TCB from the ready-to-run task list that it resides in */
252
253 DEBUGASSERT(!sched_removereadytorun(tcb));
254
255 /* Change the task priority */
256
257 tcb->sched_priority = (uint8_t)sched_priority;
258
259 /* Put it back into the correct ready-to-run task list */
260
261 DEBUGASSERT(!sched_addreadytorun(tcb));
So, statements at lines 253 and 261 would not be executed; the TCB is not placed in a new location in the prioritized list but is, instead, its priority is changes at its existing location in the prioritized list. This could have subtle effects on real-time performance and could lead to other prioritization issues when other TCBs are merged into the list. I think this would not be fatal and a busy system should recover in some period of time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612448531
Wait do no merge yet, there is a problem with the return values. DEBUGVERIFY expects ret < 0 to indicate and error. These functions do not return an error but a boolean. it will take a little more.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] Ouss4 merged pull request #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
Ouss4 merged pull request #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612664631
> I did some searching to see if there might be other instances of the same problem.
@hartmannathan You should submit a PR. The fix is just to change the DEBUASSERT back to ASSERT. There was a wholesale change of ASSERT to DEBUGASSERT with https://github.com/apache/incubator-nuttx/commit/e1202d2ed33a257cd6d812fde8c3a77a4d7c1d01 and this introduced the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612664631
> I did some searching to see if there might be other instances of the same problem.
@hartmannathan You should submit a PR. The fix is just to change the DEBUASSERT back to ASSERT. There was a wholesale change of ASSERT to DEBUGASSERT with https://github.com/apache/incubator-nuttx/commit/e1202d2ed33a257cd6d812fde8c3a77a4d7c1d01 and this introduced the problem.
This is not the correct place for this discussion. You should open an issue or, better, a 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] hartmannathan commented on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
hartmannathan commented on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612662513
I did some searching to see if there might be other instances of the same problem.
There are thousands and thousands of DEBUGASSERT statements so it's a little challenging to wade through all of them, but I found four that I suspect might be wrong:
**(1) In function arm_boot(), line 343, arch/arm/src/tms570/tms570_boot.c:**
tms570_memtest_start(PBIST_RINFOL_ESRAM1_RAM);
DEBUGASSERT(tms570_memtest_complete() == OK);
If DEBUGASSERT not enabled, we will never rendezvous with completion of the test.
**(2) Later in the same function, line 382, arch/arm/src/tms570/tms570_boot.c:**
DEBUGASSERT(tms570_memtest_complete() == OK);
Same problem (?)
**(3) In function tms570_clockconfig(), line 842, arch/arm/src/tms570/tms570_clockconfig.c:**
DEBUGASSERT(tms570_efc_selftest_complete() == 0);
Same problem (?)
**(4) In function board_late_initialize(), line 92, boards/arm/sam34/sam4s-xplained-pro/src/sam_boot.c:**
DEBUGASSERT(sam_watchdog_initialize() >= 0);
Watchdog would not be initialized if DEBUGASSERT is not enabled.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612447768
Good find. Yes that is a serious problem for anyone using priority with assertions disabled. In this case, the macro DEBUGVERIFY was intended, not DEBUGASSERT.
The end result is that that if DEBUG assertions were not enabled and priority inheritance is enabled, then the ordering of task is the ready-to-run list wills be screwed up. That is is ordered from highest priority to lowest. If DEBUG assertions are not enabled, then this sequence of code changes:
251 /* Remove the TCB from the ready-to-run task list that it resides in */
252
253 DEBUGASSERT(!sched_removereadytorun(tcb));
254
255 /* Change the task priority */
256
257 tcb->sched_priority = (uint8_t)sched_priority;
258
259 /* Put it back into the correct ready-to-run task list */
260
261 DEBUGASSERt(!sched_addreadytorun(tcb));
So, statements at lines 253 and 261 would not be executed; the TCB is not placed in a new location in the prioritized list but is, instead, its priority is changes at its existing location in the prioritized list. This could have subtle effects on real-time performance and could lead to other prioritization issues when other TCBs are merged into the list. I think this would not be fatal and a busy system should recover in some period of time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #765:
sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #765: sched/sched_setpriority.c: DEBUGVERIFY, not DEBUGASSERT.
URL: https://github.com/apache/incubator-nuttx/pull/765#issuecomment-612453692
This error has been around since August 24, 2019. It was introduced by commit e1202d2ed33a257cd6d812fde8c3a77a4d7c1d01 See https://github.com/apache/incubator-nuttx/commit/e1202d2ed33a257cd6d812fde8c3a77a4d7c1d01
That commit changed all occurrences of ASSERT to DEBUGASSERT. The logical flaw in do that is that the logic withing ASSERT runs unconditionally. Changing to DEBUGASSERT causes the logic to only execute conditionally, introducing this error.
I suppose in retrospect that the simplest solution would have been to simply restore the ASSERT since the condition that is tested would be an OS 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services