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/12/11 06:43:00 UTC
[GitHub] [incubator-nuttx] Donny9 opened a new pull request #4977: arch: add CONFIG_ARCH_STACKSIZE_ADJUSTMENT to reduce variability
Donny9 opened a new pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977
## Summary
Add CONFIG_ARCH_STACKSIZE_ADJUSTMENT to reduce variability between the different platform stack size setting
Signed-off-by: Jiuzhu Dong <do...@xiaomi.com>
## Impact
N/A
## Testing
daily test
--
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 a change in pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767300361
##########
File path: arch/sim/src/sim/up_createstack.c
##########
@@ -104,6 +104,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
}
#endif
+ stack_size += CONFIG_SIM_STACKSIZE_ADJUSTMENT;
Review comment:
move before line 95
##########
File path: arch/sim/Kconfig
##########
@@ -134,6 +134,13 @@ config SIM_WALLTIME_SIGNAL
endchoice
+config SIM_STACKSIZE_ADJUSTMENT
+ int "The adjustment of stack size for sim"
+ default 65536
+ ---help---
+ The adjustment of stack size for different arch. When the task is
Review comment:
"different arch" to sim?
##########
File path: sched/Kconfig
##########
@@ -1808,14 +1808,13 @@ menu "Stack and heap information"
config DEFAULT_TASK_STACKSIZE
int "The default stack size for tasks"
- default 65536 if ARCH_SIM
default 2048
---help---
The default stack size for tasks.
config IDLETHREAD_STACKSIZE
int "Idle thread stack size"
- default DEFAULT_TASK_STACKSIZE if ARCH_SIM
+ default 65536 if ARCH_SIM
default 1024
Review comment:
let's change default to DEFAULT_TASK_STACKSIZE?
--
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] Donny9 commented on a change in pull request #4977: arch: add CONFIG_ARCH_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
Donny9 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767152170
##########
File path: sched/Kconfig
##########
@@ -1806,17 +1806,24 @@ endmenu # Work Queue Support
menu "Stack and heap information"
+config ARCH_STACKSIZE_ADJUSTMENT
+ int "The adjustment of stack size for different arch"
+ default 65536 if ARCH_SIM
+ default 0 if !ARCH_SIM
+ ---help---
+ The adjustment of stack size for different arch. When the task is
+ created, the stack size enpands automatically depending on the arch.
Review comment:
Done! Thank you~
--
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 #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1024053566
> > > > > i'm not sure if this is a good idea because
> > > > >
> > > > > * this breaks existing configurations
> > > >
> > > >
> > > > Could you explain more?
> > >
> > >
> > > if you have a config with stack sizes adjusted carefully, it won't work as expected anymore.
> >
> >
> > > > > * it makes stack-related api very confusing (eg. pthread_attr_setstack and pthread_attr_setstacksize)
> > > >
> > > >
> > > > SIM_STACKSIZE_ADJUSTMENT is used only for sim, the stack on sim already has huge difference to other arch. This patch is try to fix the program hard code the stack size in code or config with a fixed default value.
> > >
> > >
> > > the difference from other arch is not a problem as it's arch-dependent in the first place.
> > > my concern is inconsistency within sim. with this change, an app using pthread_attr_setstack and an app using pthread_attr_setstacksize need to have different ideas of stack size.
> >
> >
> > If the user tune the stack size by pthread_attr_setstacksize for real device, it normally stop work on sim, so do you prefer the user define the different size between sim and other arch. But, is it good to let user take care about the stack size on sim?
>
> yes. it's something only the app can deal with some #ifdef.
>
Why do you want to see a general application add ifdef CONFIG_ARCH_SIM? As I said before, sim is just for developing, nobody want to fight the stack size issue on the simulator. We have 50+ developers use simulator daily who doesn't have the deep knowledge how sim work internally. The top 1 issue(complaint) is that the application run correctly on the real device, but crash on sim. After investigating by NuttX expert, 90+% is just because they copy the stack size config to sim.
> > > > > * stacksize is inherently arch-dependent
> > > >
> > > >
> > > > DEFAULT_TASK_STACKSIZE could cover the most arch difference.
> > >
> > >
> > > the stack usage is actually different. it's more natural to use different values to reflect the reality than trying to maintain the illusion of "one value fit all".
> >
> >
> > I just want to illusion of the sim(that's why we name it CONFIG_SIM_STACKSIZE_ADJUSTMENT) not other real arch. sim is just a test platform for functional development, nobody expect to tune the stack size on it, so it's boring to enforce the user to define the different stack size for sim either by defconfig or source code.
> > We have found many people just turn on some application under apps/, and crash immediately on sim just because that application tune the stack size to a particular value.
>
> how those applications tune the stack size? if they hardcode values for a specific arch, it's their problem, not sim.
If you don't like this patch, please revert DEFAULT_STACK_SIZE too. From my review, DEFAULT_STACK_SIZE provided by you also try to fix the same stack size. And let's modify all the hardcode stack size in application or add CONFIG_xxx_STACK_SIZE=65536 in all defconfig under boards/sim.
--
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 #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1011305739
> i'm not sure if this is a good idea because
>
> * this breaks existing configurations
Could you explain more?
> * it makes stack-related api very confusing (eg. pthread_attr_setstack and pthread_attr_setstacksize)
SIM_STACKSIZE_ADJUSTMENT is used only for sim, the stack on sim already has huge difference to other arch. This patch is try to fix the program hard code the stack size in code or config with a fixed default value.
> * stacksize is inherently arch-dependent
DEFAULT_TASK_STACKSIZE could cover the most arch difference.
--
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] Donny9 commented on a change in pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
Donny9 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767384491
##########
File path: sched/Kconfig
##########
@@ -1808,14 +1808,13 @@ menu "Stack and heap information"
config DEFAULT_TASK_STACKSIZE
int "The default stack size for tasks"
- default 65536 if ARCH_SIM
default 2048
---help---
The default stack size for tasks.
config IDLETHREAD_STACKSIZE
int "Idle thread stack size"
- default DEFAULT_TASK_STACKSIZE if ARCH_SIM
+ default 65536 if ARCH_SIM
default 1024
Review comment:
we can remove this "default DEFAULT_TASK_STACKSIZE if ARCH_SIM" by up_initial_state adjust.
--
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 a change in pull request #4977: arch: add CONFIG_ARCH_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767137509
##########
File path: sched/Kconfig
##########
@@ -1806,17 +1806,24 @@ endmenu # Work Queue Support
menu "Stack and heap information"
+config ARCH_STACKSIZE_ADJUSTMENT
+ int "The adjustment of stack size for different arch"
+ default 65536 if ARCH_SIM
+ default 0 if !ARCH_SIM
+ ---help---
+ The adjustment of stack size for different arch. When the task is
+ created, the stack size enpands automatically depending on the arch.
Review comment:
```suggestion
created, the stack size expands automatically depending on the arch.
```
--
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] yamt commented on pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1010632740
i'm not sure if this is a good idea because
* this breaks existing configurations
* it makes stack-related api very confusing (eg. pthread_attr_setstack and pthread_attr_setstacksize)
* stacksize is inherently arch-dependent
--
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 #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1016420546
> > > i'm not sure if this is a good idea because
> > >
> > > * this breaks existing configurations
> >
> >
> > Could you explain more?
>
> if you have a config with stack sizes adjusted carefully, it won't work as expected anymore.
>
> > > * it makes stack-related api very confusing (eg. pthread_attr_setstack and pthread_attr_setstacksize)
> >
> >
> > SIM_STACKSIZE_ADJUSTMENT is used only for sim, the stack on sim already has huge difference to other arch. This patch is try to fix the program hard code the stack size in code or config with a fixed default value.
>
> the difference from other arch is not a problem as it's arch-dependent in the first place.
>
> my concern is inconsistency within sim. with this change, an app using pthread_attr_setstack and an app using pthread_attr_setstacksize need to have different ideas of stack size.
If the user tune the stack size by pthread_attr_setstacksize for real device, it normally stop work on sim, so do you prefer the user define the different size between sim and other arch. But, is it good to let user take care about the stack size on sim?
>
> > > * stacksize is inherently arch-dependent
> >
> >
> > DEFAULT_TASK_STACKSIZE could cover the most arch difference.
>
> the stack usage is actually different. it's more natural to use different values to reflect the reality than trying to maintain the illusion of "one value fit all".
I just want to illusion of the sim(that's why we name it CONFIG_SIM_STACKSIZE_ADJUSTMENT) not other real arch. sim is just a test platform for functional development, nobody expect to tune the stack size on it, so it's boring to enforce the user to define the different stack size for sim either by defconfig or source code.
We have found many people just turn on some application under apps/, and crash immediately on sim just because that application tune the stack size to a particular value.
--
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] yamt commented on pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1023901484
https://github.com/apache/incubator-nuttx/pull/5365
--
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] yamt commented on pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1025254774
> > > > > > i'm not sure if this is a good idea because
> > > > > >
> > > > > > * this breaks existing configurations
> > > > >
> > > > >
> > > > > Could you explain more?
> > > >
> > > >
> > > > if you have a config with stack sizes adjusted carefully, it won't work as expected anymore.
> > >
> > >
> > > > > > * it makes stack-related api very confusing (eg. pthread_attr_setstack and pthread_attr_setstacksize)
> > > > >
> > > > >
> > > > > SIM_STACKSIZE_ADJUSTMENT is used only for sim, the stack on sim already has huge difference to other arch. This patch is try to fix the program hard code the stack size in code or config with a fixed default value.
> > > >
> > > >
> > > > the difference from other arch is not a problem as it's arch-dependent in the first place.
> > > > my concern is inconsistency within sim. with this change, an app using pthread_attr_setstack and an app using pthread_attr_setstacksize need to have different ideas of stack size.
> > >
> > >
> > > If the user tune the stack size by pthread_attr_setstacksize for real device, it normally stop work on sim, so do you prefer the user define the different size between sim and other arch. But, is it good to let user take care about the stack size on sim?
> >
> >
> > yes. it's something only the app can deal with some #ifdef.
>
> Why do you want to see a general application add ifdef CONFIG_ARCH_SIM?
i don't.
but sometimes it's necessary.
> As I said before, sim is just for developing, nobody want to fight the stack size issue on the simulator. We have 50+ developers use simulator daily who doesn't have the deep knowledge how sim work internally. The top 1 issue(complaint) is that the application run correctly on the real device, but crash on sim. After investigating by NuttX expert, 90+% is just because they copy the stack size config to sim.
interesting. thank you for the info.
what's your suggestion to them when their app was using pthread_attr_setstack-like api?
>
> > > > > > * stacksize is inherently arch-dependent
> > > > >
> > > > >
> > > > > DEFAULT_TASK_STACKSIZE could cover the most arch difference.
> > > >
> > > >
> > > > the stack usage is actually different. it's more natural to use different values to reflect the reality than trying to maintain the illusion of "one value fit all".
> > >
> > >
> > > I just want to illusion of the sim(that's why we name it CONFIG_SIM_STACKSIZE_ADJUSTMENT) not other real arch. sim is just a test platform for functional development, nobody expect to tune the stack size on it, so it's boring to enforce the user to define the different stack size for sim either by defconfig or source code.
> > > We have found many people just turn on some application under apps/, and crash immediately on sim just because that application tune the stack size to a particular value.
> >
> >
> > how those applications tune the stack size? if they hardcode values for a specific arch, it's their problem, not sim.
>
> If you don't like this patch, please revert DEFAULT_STACK_SIZE too. From my review, DEFAULT_STACK_SIZE provided by you also try to fix the same stack size. And let's modify all the hardcode stack size in application or add CONFIG_xxx_STACK_SIZE=65536 in all defconfig under boards/sim.
replied in the other 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] [incubator-nuttx] xiaoxiang781216 commented on pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1025347615
> > Why do you want to see a general application add ifdef CONFIG_ARCH_SIM?
>
> i don't. but sometimes it's necessary.
>
With this patch, developer don't need handle the stack size specially for sim in most case, except the code call pthread_attr_setstack with non-NULL stack pointer(Actually, no code in apps/nuttx call this api, which mean this approach work for all mainline code).
> > As I said before, sim is just for developing, nobody want to fight the stack size issue on the simulator. We have 50+ developers use simulator daily who doesn't have the deep knowledge how sim work internally. The top 1 issue(complaint) is that the application run correctly on the real device, but crash on sim. After investigating by NuttX expert, 90+% is just because they copy the stack size config to sim.
>
> interesting. thank you for the info. what's your suggestion to them when their app was using pthread_attr_setstack-like api?
I never see the case which call pthread_attr_setstack with no NULL pointer. All developer call pthread_attr_setstack just want to adjust the stack size, which can handle correctly with this approach.
--
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] yamt commented on pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1025359323
> > > Why do you want to see a general application add ifdef CONFIG_ARCH_SIM?
> >
> >
> > i don't. but sometimes it's necessary.
>
> With this patch, developer don't need handle the stack size specially for sim in most case, except the code call pthread_attr_setstack with non-NULL stack pointer(Actually, no code in apps/nuttx call this api, which mean this approach work for all mainline code).
>
> > > As I said before, sim is just for developing, nobody want to fight the stack size issue on the simulator. We have 50+ developers use simulator daily who doesn't have the deep knowledge how sim work internally. The top 1 issue(complaint) is that the application run correctly on the real device, but crash on sim. After investigating by NuttX expert, 90+% is just because they copy the stack size config to sim.
> >
> >
> > interesting. thank you for the info. what's your suggestion to them when their app was using pthread_attr_setstack-like api?
>
> I never see the case which call with no NULL pointer. All most developers call pthread_attr_setstack just want to adjust the stack size, which can handle correctly with this approach.
is it even a vaild usage of pthread_attr_setstack?
i guess you should use pthread_attr_setstacksize instead.
>
> Yes, this approach isn't perfect, but it can fix 99.99% problem.
maybe. but the api consistency outweigh IMO.
--
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] yamt commented on pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1017040342
> > > > i'm not sure if this is a good idea because
> > > >
> > > > * this breaks existing configurations
> > >
> > >
> > > Could you explain more?
> >
> >
> > if you have a config with stack sizes adjusted carefully, it won't work as expected anymore.
>
> > > > * it makes stack-related api very confusing (eg. pthread_attr_setstack and pthread_attr_setstacksize)
> > >
> > >
> > > SIM_STACKSIZE_ADJUSTMENT is used only for sim, the stack on sim already has huge difference to other arch. This patch is try to fix the program hard code the stack size in code or config with a fixed default value.
> >
> >
> > the difference from other arch is not a problem as it's arch-dependent in the first place.
> > my concern is inconsistency within sim. with this change, an app using pthread_attr_setstack and an app using pthread_attr_setstacksize need to have different ideas of stack size.
>
> If the user tune the stack size by pthread_attr_setstacksize for real device, it normally stop work on sim, so do you prefer the user define the different size between sim and other arch. But, is it good to let user take care about the stack size on sim?
yes. it's something only the app can deal with some #ifdef.
>
> > > > * stacksize is inherently arch-dependent
> > >
> > >
> > > DEFAULT_TASK_STACKSIZE could cover the most arch difference.
> >
> >
> > the stack usage is actually different. it's more natural to use different values to reflect the reality than trying to maintain the illusion of "one value fit all".
>
> I just want to illusion of the sim(that's why we name it CONFIG_SIM_STACKSIZE_ADJUSTMENT) not other real arch. sim is just a test platform for functional development, nobody expect to tune the stack size on it, so it's boring to enforce the user to define the different stack size for sim either by defconfig or source code.
>
> We have found many people just turn on some application under apps/, and crash immediately on sim just because that application tune the stack size to a particular value.
how those applications tune the stack size?
if they hardcode values for a specific arch, it's their problem, not sim.
--
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] Donny9 commented on a change in pull request #4977: arch: add CONFIG_ARCH_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
Donny9 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767152170
##########
File path: sched/Kconfig
##########
@@ -1806,17 +1806,24 @@ endmenu # Work Queue Support
menu "Stack and heap information"
+config ARCH_STACKSIZE_ADJUSTMENT
+ int "The adjustment of stack size for different arch"
+ default 65536 if ARCH_SIM
+ default 0 if !ARCH_SIM
+ ---help---
+ The adjustment of stack size for different arch. When the task is
+ created, the stack size enpands automatically depending on the arch.
Review comment:
Done! Thank you~
--
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] Donny9 commented on pull request #4977: arch: add CONFIG_ARCH_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
Donny9 commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-991641158
> @Donny9 can you please give an example when or how this is used?
We all know that a task may be somewhat larger on the sim than the stack on the board because some debug like asan etc。
Before this patch, the default stack size is 65536 for ARCH_SIM, but these task may overflow on the board because their stack size are 2048 because of !ARCH_SIM. So for the different arch, there will be two different configurations.
This patch can automatically extend the size of the extension stack without changing config so keeping a configuration.
--
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] Donny9 commented on a change in pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
Donny9 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767384541
##########
File path: arch/sim/src/sim/up_createstack.c
##########
@@ -104,6 +104,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
}
#endif
+ stack_size += CONFIG_SIM_STACKSIZE_ADJUSTMENT;
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] [incubator-nuttx] Donny9 commented on a change in pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
Donny9 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767289898
##########
File path: sched/Kconfig
##########
@@ -1806,17 +1806,24 @@ endmenu # Work Queue Support
menu "Stack and heap information"
+config ARCH_STACKSIZE_ADJUSTMENT
+ int "The adjustment of stack size for different arch"
+ default 65536 if ARCH_SIM
+ default 0 if !ARCH_SIM
+ ---help---
+ The adjustment of stack size for different arch. When the task is
+ created, the stack size expands automatically depending on the arch.
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] [incubator-nuttx] yamt commented on pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1016105024
> > i'm not sure if this is a good idea because
> >
> > * this breaks existing configurations
>
> Could you explain more?
if you have a config with stack sizes adjusted carefully, it won't work as expected anymore.
>
> > * it makes stack-related api very confusing (eg. pthread_attr_setstack and pthread_attr_setstacksize)
>
> SIM_STACKSIZE_ADJUSTMENT is used only for sim, the stack on sim already has huge difference to other arch. This patch is try to fix the program hard code the stack size in code or config with a fixed default value.
the difference from other arch is not a problem as it's arch-dependent in the first place.
my concern is inconsistency within sim.
with this change, an app using pthread_attr_setstack and an app using pthread_attr_setstacksize
need to have different ideas of stack size.
>
> > * stacksize is inherently arch-dependent
>
> DEFAULT_TASK_STACKSIZE could cover the most arch difference.
the stack usage is actually different.
it's more natural to use different values to reflect the reality than trying to maintain the illusion of "one value fit all".
--
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 a change in pull request #4977: arch: add CONFIG_ARCH_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767137509
##########
File path: sched/Kconfig
##########
@@ -1806,17 +1806,24 @@ endmenu # Work Queue Support
menu "Stack and heap information"
+config ARCH_STACKSIZE_ADJUSTMENT
+ int "The adjustment of stack size for different arch"
+ default 65536 if ARCH_SIM
+ default 0 if !ARCH_SIM
+ ---help---
+ The adjustment of stack size for different arch. When the task is
+ created, the stack size enpands automatically depending on the arch.
Review comment:
```suggestion
created, the stack size expands automatically depending on the arch.
```
--
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] Donny9 commented on pull request #4977: arch: add CONFIG_ARCH_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
Donny9 commented on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-991641158
> @Donny9 can you please give an example when or how this is used?
We all know that a task may be somewhat larger on the sim than the stack on the board because some debug like asan etc。
Before this patch, the default stack size is 65536 for ARCH_SIM, but these task may overflow on the board because their stack size are 2048 because of !ARCH_SIM. So for the different arch, there will be two different configurations.
This patch can automatically extend the size of the extension stack without changing config so keeping a configuration.
--
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 #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977
--
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 a change in pull request #4977: arch: add CONFIG_ARCH_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767162589
##########
File path: sched/Kconfig
##########
@@ -1806,17 +1806,24 @@ endmenu # Work Queue Support
menu "Stack and heap information"
+config ARCH_STACKSIZE_ADJUSTMENT
+ int "The adjustment of stack size for different arch"
+ default 65536 if ARCH_SIM
+ default 0 if !ARCH_SIM
+ ---help---
+ The adjustment of stack size for different arch. When the task is
+ created, the stack size expands automatically depending on the arch.
Review comment:
The adjustment of stack size for different arch. When the task is created, the stack size is increased by this amount.
--
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] Donny9 commented on a change in pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
Donny9 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767384234
##########
File path: arch/sim/Kconfig
##########
@@ -134,6 +134,13 @@ config SIM_WALLTIME_SIGNAL
endchoice
+config SIM_STACKSIZE_ADJUSTMENT
+ int "The adjustment of stack size for sim"
+ default 65536
+ ---help---
+ The adjustment of stack size for different arch. When the task is
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] [incubator-nuttx] davids5 commented on a change in pull request #4977: arch: add CONFIG_ARCH_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
davids5 commented on a change in pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#discussion_r767162589
##########
File path: sched/Kconfig
##########
@@ -1806,17 +1806,24 @@ endmenu # Work Queue Support
menu "Stack and heap information"
+config ARCH_STACKSIZE_ADJUSTMENT
+ int "The adjustment of stack size for different arch"
+ default 65536 if ARCH_SIM
+ default 0 if !ARCH_SIM
+ ---help---
+ The adjustment of stack size for different arch. When the task is
+ created, the stack size expands automatically depending on the arch.
Review comment:
The adjustment of stack size for different arch. When the task is created, the stack size is increased by this amount.
--
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 edited a comment on pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1025347615
> > Why do you want to see a general application add ifdef CONFIG_ARCH_SIM?
>
> i don't. but sometimes it's necessary.
>
With this patch, developer don't need handle the stack size specially for sim in most case, except the code call pthread_attr_setstack with non-NULL stack pointer(Actually, no code in apps/nuttx call this api, which mean this approach work for all mainline code).
> > As I said before, sim is just for developing, nobody want to fight the stack size issue on the simulator. We have 50+ developers use simulator daily who doesn't have the deep knowledge how sim work internally. The top 1 issue(complaint) is that the application run correctly on the real device, but crash on sim. After investigating by NuttX expert, 90+% is just because they copy the stack size config to sim.
>
> interesting. thank you for the info. what's your suggestion to them when their app was using pthread_attr_setstack-like api?
I never see the case which call pthread_attr_setstack with no NULL pointer. All most developers call pthread_attr_setstack just want to adjust the stack size, which can handle correctly with this approach.
--
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 edited a comment on pull request #4977: arch/sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4977:
URL: https://github.com/apache/incubator-nuttx/pull/4977#issuecomment-1025347615
> > Why do you want to see a general application add ifdef CONFIG_ARCH_SIM?
>
> i don't. but sometimes it's necessary.
>
With this patch, developer don't need handle the stack size specially for sim in most case, except the code call pthread_attr_setstack with non-NULL stack pointer(Actually, no code in apps/nuttx call this api, which mean this approach work for all mainline code).
> > As I said before, sim is just for developing, nobody want to fight the stack size issue on the simulator. We have 50+ developers use simulator daily who doesn't have the deep knowledge how sim work internally. The top 1 issue(complaint) is that the application run correctly on the real device, but crash on sim. After investigating by NuttX expert, 90+% is just because they copy the stack size config to sim.
>
> interesting. thank you for the info. what's your suggestion to them when their app was using pthread_attr_setstack-like api?
I never see the case which call pthread_attr_setstack with no NULL pointer. All most developers call pthread_attr_setstack just want to adjust the stack size, which can handle correctly with this approach.
Yes, this approach isn't perfect, but it can fix 99.99% 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