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 2021/06/08 14:44:00 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #3871: arch: adjust idle stack offset to reserve space of stack info
anchao opened a new pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871
## Summary
arch: adjust idle stack offset to reserve space of stack info
## Impact
idle thread stack usage
## Testing
```
PID GROUP CPU PRI POLICY TYPE NPX STATE EVENT SIGMASK STACK USED FILLED CPU COMMAND
0 0 0 0 FIFO Kthread N-- Assigned 00000000 004096 004080 99.6%! 50.0% CPU0 IDLE
```
After patch:
```
PID GROUP CPU PRI POLICY TYPE NPX STATE EVENT SIGMASK STACK USED FILLED CPU COMMAND
0 0 0 0 FIFO Kthread N-- Assigned 00000000 004076 000920 22.5% 50.0% CPU0 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-856933479
> If the user wants a 4096 stack they will not get the value they set. That seam wrong and confusing.
>
> The TLS size should be added the the allocation So that USER+TLS == stack(USER's STACK) + (tls size) (rounded up for alignment)
I think the developer should not create this expectation that the defined stack size will be entirely available for usage.
The Linux Kernel seem to do the same thing:
https://elixir.bootlin.com/linux/v4.3/source/include/linux/sched.h#L2400
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-858243576
At least, TLS is required in the current implementation state. We have found the device can't boot without this because the minor(recoverable) error happen during the initialization will set errno in TLS.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-857781094
@patacongo @davids5 please review the new code which reserve tls_task_info by up_use_frame for idle task just like normal task. Node: we don't memset here since up_use_frame will do it for us.
--
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
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#discussion_r647645024
##########
File path: arch/arm/src/armv7-m/arm_initialstate.c
##########
@@ -65,8 +66,9 @@ void up_initial_state(struct tcb_s *tcb)
{
tcb->stack_alloc_ptr = (void *)(g_idle_topstack -
CONFIG_IDLETHREAD_STACKSIZE);
- tcb->stack_base_ptr = tcb->stack_alloc_ptr;
- tcb->adj_stack_size = CONFIG_IDLETHREAD_STACKSIZE;
+ tcb->stack_base_ptr = tcb->stack_alloc_ptr;
+ tcb->adj_stack_size = CONFIG_IDLETHREAD_STACKSIZE -
+ sizeof(struct task_info_s);
Review comment:
1. Shouldn't the task info be initialized to zero?
memset(tcb->stack_alloc_ptr, 0, sizeof(sizeof(struct task_info_s));
2. This does not work in SMP mode. There are multiple IDLE tasks and they do not all have pid == 0
Same comments on all similar changes.
--
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
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#discussion_r647645024
##########
File path: arch/arm/src/armv7-m/arm_initialstate.c
##########
@@ -65,8 +66,9 @@ void up_initial_state(struct tcb_s *tcb)
{
tcb->stack_alloc_ptr = (void *)(g_idle_topstack -
CONFIG_IDLETHREAD_STACKSIZE);
- tcb->stack_base_ptr = tcb->stack_alloc_ptr;
- tcb->adj_stack_size = CONFIG_IDLETHREAD_STACKSIZE;
+ tcb->stack_base_ptr = tcb->stack_alloc_ptr;
+ tcb->adj_stack_size = CONFIG_IDLETHREAD_STACKSIZE -
+ sizeof(struct task_info_s);
Review comment:
1. Shouldn't the task info be initialized to zero?
memset(tcb->stack_alloc_ptr, 0, sizeof(sizeof(struct task_info_s));
2. This does not work in SMP mode. There are multiple IDLE tasks and they do not all have pid == 0
Determining if a task is the IDLE thread by checking if pid == 0 is discouraged in all cases for this reason. In most places, the foolproof check is that tcb->flink == NULL which is true in all cases AFTER initialization when the TCB is placed in the ready-to-run list (single CPU mode) or in the assigned tasks list(s) (SMP mode). That IDLE task is always at the end of these lists and, hence, always has flink == NULL. That rule, however, probably not apply in this case, however. It is probably too early in initialization and the newly created task is not yet ready-to-run.
We might need to add a TDB_FLAG_IDLE_TASK flag to handle this case.
Same comments on all similar changes.
--
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
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-856933479
> If the user wants a 4096 stack they will not get the value they set. That seam wrong and confusing.
>
> The TLS size should be added the the allocation So that USER+TLS == stack(USER's STACK) + (tls size) (rounded up for alignment)
I think the developer should not create this expectation that the defined stack size will be entirely available for usage.
The Linux Kernel seems to do the same thing:
https://elixir.bootlin.com/linux/v5.12.9/source/include/linux/sched.h#L1742
--
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-856861221
If the user wants a 4096 stack they will not get the value they set. That seam wrong and confusing.
The TLS size should be added the the allocation So that USER+TLS == stack(USER's STACK) + (tls size) (rounded up for alignment)
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-857333901
> @anchao - I think the arguments go something like this:
>
> 1 ) "If we change internals, the users configuration should not be broken"
> 2) "Be consistent".
>
> That said the call stack penetration on idle has grown a LOT since joining ASF.
>
> I not a fan of not wasting peoples time trying to guess how to size the stacks. If a user asks for 4096 for the stack it should be 4096 (or rounded up) as it has always been.
>
> So yes I do insist (as I did on the original PR) but I am open to hearing the counter argument.
The additional size cost come from other PR, this PR just try to fix the regression introduced by that PR. Let's dissuss it in a new issue?
--
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
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#discussion_r647645024
##########
File path: arch/arm/src/armv7-m/arm_initialstate.c
##########
@@ -65,8 +66,9 @@ void up_initial_state(struct tcb_s *tcb)
{
tcb->stack_alloc_ptr = (void *)(g_idle_topstack -
CONFIG_IDLETHREAD_STACKSIZE);
- tcb->stack_base_ptr = tcb->stack_alloc_ptr;
- tcb->adj_stack_size = CONFIG_IDLETHREAD_STACKSIZE;
+ tcb->stack_base_ptr = tcb->stack_alloc_ptr;
+ tcb->adj_stack_size = CONFIG_IDLETHREAD_STACKSIZE -
+ sizeof(struct task_info_s);
Review comment:
1. Shouldn't the task info be initialized to zero?
memset(tcb->stack_alloc_ptr, 0, sizeof(sizeof(struct task_info_s));
2. This does not work in SMP mode. There are multiple IDLE tasks and they do not all have pid == 0
Determining if a task is the IDLE thread by checking if pid == 0 is discouraged in all cases for this reason. In most places, the foolproof check is that tcb->flink == NULL which is true in all cases AFTER initialization when the TCB is placed in the ready-to-run list (single CPU mode) or in the assigned tasks list(s) (SMP mode). That IDLE task is always at the end of these lists and, hence, always has flink == NULL. That rule probably not apply in this case, however. It is probably too early in initialization and the newly created task is not yet ready-to-run.
We might need to add a TDB_FLAG_IDLE_TASK flag to handle this case. See Issue #3875
Same comments on all similar changes.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-857665946
No, it's better to move the space reserve to sched/init.
--
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-856931419
@anchao - I think the arguments go something like this:
1 ) "If we change internals, the users configuration should not be broken"
2) "Be consistent".
That said the call stack penetration on idle has grown a LOT since joining ASF.
I not a fan of not wasting peoples time trying to guess how to size the stacks. If a user asks for 4096 for the stack it should be 4096 (or rounded up) as it has always been.
So yes I do insist (as I did on the original PR) but I am open to hearing the counter argument.
--
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
[GitHub] [incubator-nuttx] anchao commented on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-856904536
Hi @davids5
> If the user wants a 4096 stack they will not get the value they set. That seam wrong and confusing.
>
> The TLS size should be added the the allocation So that USER+TLS == stack(USER's STACK) + (tls size) (rounded up for alignment)
Idle thread is special, the stack size of idle thread cannot be round up because the most of boards are used to writing the definition of idle thread stack size in the link script, which is fixed, If the TLS or stack info part is increased by mistake, then Will cause unexpected issues,
Therefore, in the idle thread case, we will deduct the tls part from the current idle thread stack, so that although a part of the proc show is missing, it will not affect the operation of the program.
Seriously, Round up or alignment the TLS size for idle thread is meaningless, this is just for the proc show to the user. Of course, if you insist, I will make similar changes.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871
--
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
[GitHub] [incubator-nuttx] davids5 commented on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-857653734
> The additional size cost come from other PR, this PR just try to fix the regression introduced by that PR. Let's dissuss it in a new issue?
@xiaoxiang781216 So you would like this merged and then you will fix the issue?
--
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
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-856933479
> If the user wants a 4096 stack they will not get the value they set. That seam wrong and confusing.
>
> The TLS size should be added the the allocation So that USER+TLS == stack(USER's STACK) + (tls size) (rounded up for alignment)
I think the developer should not create this expectation that the defined stack size will be entirely available for usage.
The Linux Kernel seems to do the same thing:
https://elixir.bootlin.com/linux/v4.3/source/include/linux/sched.h#L2400
--
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
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#discussion_r647645024
##########
File path: arch/arm/src/armv7-m/arm_initialstate.c
##########
@@ -65,8 +66,9 @@ void up_initial_state(struct tcb_s *tcb)
{
tcb->stack_alloc_ptr = (void *)(g_idle_topstack -
CONFIG_IDLETHREAD_STACKSIZE);
- tcb->stack_base_ptr = tcb->stack_alloc_ptr;
- tcb->adj_stack_size = CONFIG_IDLETHREAD_STACKSIZE;
+ tcb->stack_base_ptr = tcb->stack_alloc_ptr;
+ tcb->adj_stack_size = CONFIG_IDLETHREAD_STACKSIZE -
+ sizeof(struct task_info_s);
Review comment:
1. Shouldn't the task info be initialized to zero?
memset(tcb->stack_alloc_ptr, 0, sizeof(sizeof(struct task_info_s));
2. This does not work in SMP mode. There are multiple IDLE tasks and they do not all have pid == 0
Determining if a task is the IDLE thread by checking if pid == 0 is discouraged in all cases for this reason. In most places, the foolproof check is that tcb->flink == NULL which is true in all cases AFTER initialization when the TCB is placed in the ready-to-run list (single CPU mode) or in the assigned tasks list(s) (SMP mode). That IDLE task is always at the end of these lists and, hence, always has flink == NULL. That rule, however, probably not apply in this case, however. It is probably too early in initialization and the newly created task is not yet ready-to-run.
We might need to add a TDB_FLAG_IDLE_TASK flag to handle this case. See Issue #3875
Same comments on all similar changes.
--
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
[GitHub] [incubator-nuttx] patacongo commented on pull request #3871: arch: adjust idle stack offset to reserve space of stack info
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #3871:
URL: https://github.com/apache/incubator-nuttx/pull/3871#issuecomment-857904114
> @patacongo yes, you are right. @no1wudi please update these two places too when you bring back your change again.
It seems a little wasteful to support this thread-specific data in the IDLE thread (or any other kernel thread) since they do not have pthreads. Is there a clean way to just omit most of TLS from IDLE and kernel threads? That would be a more embedded friendly thing to do.
In the past, my eventual goal was to make kernel threads very lightweight. Task threads have gotten heavier and because of all of shared logic, kernel threads have gotten heavier 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org