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/18 17:08:57 UTC
[GitHub] [incubator-nuttx] GUIDINGLI opened a new pull request #5266: idle: remove heap & stack check in idle thread
GUIDINGLI opened a new pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266
## Summary
idle: remove heap & stack check in idle thread
## Impact
idle
## Testing
VELA
--
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 #5266: IDLE thread will not boost up when IDLE hold sem
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1018169883
If we want to do the health check in idle thread and still keep real time behavior as much as possible, boosting idle thread priority is the best option as far as I know. But, the risk of this change is very high since it change the fundamental assumption that idle thread priority is always zero.
> At this moment I am of the strong belief that the Idle Task must do nothing at all. Everything it does should be moved to workers or elsewhere. Somewhere that they can be properly managed, and affect the system in a logical and expected way.
Yes, I agree too. How about we remove the health check instead?
--
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] fjpanag commented on pull request #5266: IDLE thread will not boost up when IDLE hold sem
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1017821283
Just a note:
> As you ready said, the check is just for debugging, the user who enable CONFIG_MM_DEBUG should expect the slow response.
I don't agree with this, because:
* During debugging you want a system that behaves somewhat similarly to the production application. If it exhibits vastly different behaviour, then its debugging value is diminished.
* Some systems, mostly safety-critical systems, continually check their state, validating that everything is working correctly. They need to catch any issue quickly and act to recover the system, instead of processing garbage. Such systems will want to keep these checks enabled in production firmwares too.
This PR (and this feature in general) is not that it makes the system slower.
It is that it changes how tasks are scheduled, and even can of violate priorities.
--
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 pull request #5266: IDLE thread will not boost up when IDLE hold sem
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1016438273
The idle thread should always have lowest possible priority and that is by its definition, so I like a way that we do not boost for idle thread.
--
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] fjpanag commented on pull request #5266: IDLE thread will not boost up when IDLE hold sem
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1017817251
> @fjpanag could you try this patch whether fix your problem.
Sorry, too busy yesterday.
I just had the chance to test this on actual hardware.
For the moment it seems that *at least on my application*, it resolves the issue.
However I think that this is a bad solution, because it has the potential for an ugly deadlock.
The other propositions harmed the real-time behavior of the system, but this can lock us outside of the mm!
Since the priority of the Idle Task cannot be boosted anymore, and since it holds the mm, if it doesn't get the chance to run again then we lost access to the mm. And this chance seems quite high, because the Idle task mostly holds this semaphore and because it is normal for real-time systems to never return to idle.
---
```
enter_crtitical_sectiion();
mm_checkcurrption();
leave_crtitical_sectiion();
```
This solution is better, since it cannot cause this issue.
On the other hand, it locks the system unecessarily most of the time, causes heavy jitter and harms the RT behaviour.
---
```
if ((sched_priority < SCHED_PRIORITY_MIN ||
sched_priority > SCHED_PRIORITY_MAX) &&
!(sched_priority == 0 && tcb->pid == 0))
{
return -EINVAL;
}
```
I believe that this is even better, because it has the disadvantages of the previous solution *only if it is actually needed*, not every time. It is still harmful though.
---
At this moment I am of the strong belief that the Idle Task must do nothing at all.
Everything it does should be moved to workers or elsewhere. Somewhere that they can be properly managed, and affect the system in a logical and expected way.
--
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] fjpanag commented on pull request #5266: IDLE thread will not boost up when IDLE hold sem
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1019920064
> Yes, I agree too. How about we remove the health check instead?
Let's move it, not remove it.
These are a bit busy days here at work. So unless someone wants to get into this issue, I will fix this, maybe by next week.
I am thinking of moving all these checks to a worker thread.
So we get the same functionality running at `CONFIG_SCHED_LPWORKPRIORITY`, with the possibility of priority inheritance and and Idle Task that it is actually idle.
--
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] davids5 commented on pull request #5266: IDLE thread will not boost up when IDLE hold sem
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1016649273
Granted it is a diagnostic mode. This effectively defeats PI. Is it a good idea for idle to not immediately yield? I would suspect this changes the RT nature of the system in a not ideal.
--
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] Ouss4 commented on pull request #5266: idle: remove heap & stack check in idle thread
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1015635222
@GUIDINGLI why were these removed?
We had to do some refactoring to `mm_trylock` to accommodate using `mm_checkcurrption` from the idle thread. If there is a reason to remove this, we are going to have to restore the old behaviour for `mm_trylock` 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] [incubator-nuttx] GUIDINGLI commented on pull request #5266: idle: remove heap & stack check in idle thread
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1016280233
> @GUIDINGLI why were these removed?
>
> We had to do some refactoring to `mm_trylock` to accommodate using `mm_checkcurrption` from the idle thread. If there is a reason to remove this, we are going to have to restore the old behaviour for `mm_trylock` as well.
@Ouss4 You can see https://github.com/apache/incubator-nuttx/commit/9a53601ba993d43684328dcae8750acb00a34cc7, that's why removed.
@fjpanag Actually, there is another way to resolve the issue caused by idle hold sem.
We can do:
enter_crtitical_sectiion();
mm_checkcurrption();
leave_crtitical_sectiion();
Thus, there is no way to switch out during mm_checkcurrption(), and the issue will not happen.
How do you think ?
--
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 #5266: IDLE thread will not boost up when IDLE hold sem
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266
--
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 #5266: IDLE thread will not boost up when IDLE hold sem
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1016446670
> The idle thread should always have lowest possible priority and that is by its definition, so I like a way that we do not boost for idle thread.
Yes, that's this patch want to enforce.
--
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] fjpanag edited a comment on pull request #5266: idle: remove heap & stack check in idle thread
Posted by GitBox <gi...@apache.org>.
fjpanag edited a comment on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1016259419
@GUIDINGLI as mentioned [here](https://github.com/apache/incubator-nuttx/commit/9a53601ba993d43684328dcae8750acb00a34cc7#commitcomment-64151785), removing the check does not solve the issue.
There is already a way to disable this check, through Kconfig.
The fix (even if temporary) should aim in having this check working specifically when priority inheritance is enabled.
---
My thought for a temporal fix would be something like this:
```
// sched/sched_setpriority.c line 336
if ((sched_priority < SCHED_PRIORITY_MIN ||
sched_priority > SCHED_PRIORITY_MAX) &&
!(sched_priority == 0 && tcb->pid == 0 )
{
return -EINVAL;
}
```
--
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] fjpanag commented on pull request #5266: idle: remove heap & stack check in idle thread
Posted by GitBox <gi...@apache.org>.
fjpanag commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1016259419
@GUIDINGLI as mentioned [here](https://github.com/apache/incubator-nuttx/commit/9a53601ba993d43684328dcae8750acb00a34cc7#commitcomment-64151785), removing the check does not solve the issue.
There is already a way to disable this check, through Kconfig.
The fix (even if temporary) should aim in having this check working specifically when priority inheritance is enabled.
---
My thought for a temporal fix would be something like this:
```
// sched/sched_setpriority.c line 336
if (sched_priority < SCHED_PRIORITY_MIN ||
sched_priority > SCHED_PRIORITY_MAX ||
!(sched_priority == 0 && tcb->pid == 0 )
{
return -EINVAL;
}
```
--
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] GUIDINGLI commented on pull request #5266: idle: remove heap & stack check in idle thread
Posted by GitBox <gi...@apache.org>.
GUIDINGLI commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1016411539
@fjpanag @Ouss4
Another way:
Only NON-IDLE thread can do `nxsem_add_holder_tcb()`
--
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 #5266: IDLE thread will not boost up when IDLE hold sem
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1016447051
@fjpanag could you try this patch whether fix your 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.
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 #5266: IDLE thread will not boost up when IDLE hold sem
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1016677539
> Granted it is a diagnostic mode. This effectively defeats PI. Is it a good idea for idle to not immediately yield? I would suspect this changes the RT nature of the system in a not ideal.
A better fix is boost the idle thread to the high priority, but this will make the idle thread not at the tail of the ready list, which may introduce more bad side effect.
As you ready said, the check is just for debugging, the user who enable CONFIG_MM_DEBUG should expect the slow response.
--
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] fjpanag edited a comment on pull request #5266: idle: remove heap & stack check in idle thread
Posted by GitBox <gi...@apache.org>.
fjpanag edited a comment on pull request #5266:
URL: https://github.com/apache/incubator-nuttx/pull/5266#issuecomment-1016259419
@GUIDINGLI as mentioned [here](https://github.com/apache/incubator-nuttx/commit/9a53601ba993d43684328dcae8750acb00a34cc7#commitcomment-64151785), removing the check does not solve the issue.
There is already a way to disable this check, through Kconfig.
The fix (even if temporary) should aim in having this check working specifically when priority inheritance is enabled.
---
My thought for a temporal fix would be something like this:
```
// sched/sched_setpriority.c line 336
if ((sched_priority < SCHED_PRIORITY_MIN ||
sched_priority > SCHED_PRIORITY_MAX) &&
!(sched_priority == 0 && tcb->pid == 0))
{
return -EINVAL;
}
```
--
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