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/28 05:29:40 UTC
[GitHub] [incubator-nuttx] yamt opened a new pull request #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
yamt opened a new pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365
## Summary
This reverts commit 6b5a7a73ba76671c81dc70485befb5ed5e818fe0.
Because:
* it broke existing configurations.
* it makes stack-related api very confusing.
eg. the "stack_size" argument of nxtask_init has different meanings
depending on "stack".
eg. pthread_attr_setstack and pthread_attr_setstacksize
* stacksize is inherently arch-dependent.
the goal to share the same stack size configurations among archs
is questionable.
## Impact
sim
## Testing
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1024144531
> > > ## Summary
> > > This reverts commit [6b5a7a7](https://github.com/apache/incubator-nuttx/commit/6b5a7a73ba76671c81dc70485befb5ed5e818fe0).
> > > Because:
> > >
> > > * it broke existing configurations.
> >
> >
> > could you point out which config broken?
>
> eg. my local config, which is now using much larger stack than intended.
>
DEFAULT_STACK_SIZE on sim equals 65536 before the change, SIM_STACKSIZE_ADJUSTMENT equals 65536 after change. Both config has the similar functionality and same default value, so the stack consumption should be similar.
> > > * it makes stack-related api very confusing.
> > > eg. the "stack_size" argument of nxtask_init has different meanings
> > > depending on "stack".
> >
> >
> > If you don't like the stack size reported from ps, I can change the report size as user passed one, but allocate the bigger memory.
>
> i was not talking about the report size.
>
> nxtast_init takes "non adjusted" stack_size for stack==NULL while it takes "adjusted" stack_size for stack!=NULL.
>
Yes, this is the only case we can't adjust, but this feature is seldom used.
> > > eg. pthread_attr_setstack and pthread_attr_setstacksize
> > >
> > > * stacksize is inherently arch-dependent.
> >
> >
> > Yes, that's why the adjust is located in arch/sim.
> > > the goal to share the same stack size configurations among archs
> > > is questionable.
> >
> >
> > As I explain before no one try to the same stack config for all arch, I just want to fix sim problem.
>
> ok. maybe "the goal to share the same stack size configurations between arch X and sim is questionable" is a better description?
>
All programs(except pthread_attr_setstack) which has the stack size with fine tuning or hardcode value can run on sim correctly. this approach is better than DEFAULT_STACK_SIZE. So, please revert both in this PR if you like.
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1025380047
> > > api inconsistency only happen for pthread_attr_setstack with non NULL pointer. If you like, we can fix this inconsistency too: let up_use_stack ignore the preallocated stack pointer and allocate a new stack with the adjusted size. This fix is reasonable since the preallocated memory isn't difference from the dynamically allocated memory.
> >
> >
> > i've seen an app which is actually checking if it's running on the preallocated stack. i'm not saying it's a good idea. i'm just saying such an app exists.
>
> app should trust the kernel, the fix could be simply remove the check.
i vaguely remember that the code was actually changing its behavior depending on the stack on which it's running.
anyway, i guess such a "fix" needs a deep understanding on the app code.
it can be more difficult than tweaking the stack size for 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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1025354526
> > Yes, this is the only case we can't adjust, but this feature is seldom used.
>
> i guess pthread_attr_setstacksize is seldom used too, right? or, is it somehow important?
>
pthread_attr_setstacksize is important, but SIM_STACKSIZE_ADJUSTMENT can handle this case correctly. What this approach can't handle is pthread_attr_setstack with non NULL pointer, but this case is rarely used.
> > All programs(except pthread_attr_setstack) which has the stack size with fine tuning or hardcode value can run on sim correctly. this approach is better than DEFAULT_STACK_SIZE. So, please revert both in this PR if you like.
>
> my biggest complaint is the api inconsistency.
api inconsistency only happen for pthread_attr_setstack with non NULL pointer. If you like, we can fix this inconsistency too: let up_use_stack ignore the preallocated stack pointer and allocate a new stack with the adjusted size.
> that's why i prefer Kconfig-level (or maybe Application.mk level) adjustment over this approach.
Kconfig/Application.mk level adjustment can't handle these cases automatically:
1. The default stack size in Kconfig is a fine tune value, not DEFAULT_STACK_SIZE
2. The code call pthread_attr_setstack with the hard code value
But, the above cases happen frequently. Before we find a better solution, please don't revert this patch. It's a waste of time to fix the stack size issue on simulator.
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1024054529
Copy from https://github.com/apache/incubator-nuttx/pull/4977:
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.
@yamt please revert DEFAULT_STAck_SIZE related change too, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#discussion_r794360477
##########
File path: sched/Kconfig
##########
@@ -1828,12 +1828,14 @@ menu "Stack and heap information"
config DEFAULT_TASK_STACKSIZE
int "The default stack size for tasks"
+ default 65536 if ARCH_SIM
Review comment:
No, it's wrong to add the arch specific code in common place. Let me highlight INVIOLABLES rule here again:
https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md#modular-architecture
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1025374612
> > api inconsistency only happen for pthread_attr_setstack with non NULL pointer. If you like, we can fix this inconsistency too: let up_use_stack ignore the preallocated stack pointer and allocate a new stack with the adjusted size. This fix is reasonable since the preallocated memory isn't difference from the dynamically allocated memory.
>
> i've seen an app which is actually checking if it's running on the preallocated stack. i'm not saying it's a good idea. i'm just saying such an app exists.
>
app should trust the kernel, the fix could be simply remove the check. Anyway, I provide a patch here: https://github.com/apache/incubator-nuttx/pull/5384.
> > > that's why i prefer Kconfig-level (or maybe Application.mk level) adjustment over this approach.
> >
> >
> > Kconfig/Application.mk level adjustment can't handle these cases automatically:
> > ```
> > 1. The default stack size in Kconfig is a fine tune value, not DEFAULT_STACK_SIZE
> >
> > 2. The code call pthread_attr_setstack with the hard code value
> > But, the above cases happen frequently. Before we find a better solution, please don't revert this patch. It's a waste of time to fix the stack size issue on simulator.
>
> i feel what you really want is a way to ignore ~all user-specified stack addr / size. how do you think?
Yes, but just for sim. Simulator is a very special target, it doesn't make sense to let developer fix the stack overflow issue on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1025374612
> > api inconsistency only happen for pthread_attr_setstack with non NULL pointer. If you like, we can fix this inconsistency too: let up_use_stack ignore the preallocated stack pointer and allocate a new stack with the adjusted size. This fix is reasonable since the preallocated memory isn't difference from the dynamically allocated memory.
>
> i've seen an app which is actually checking if it's running on the preallocated stack. i'm not saying it's a good idea. i'm just saying such an app exists.
>
app should trust the kernel, the fix could be simply remove the check.
> > > that's why i prefer Kconfig-level (or maybe Application.mk level) adjustment over this approach.
> >
> >
> > Kconfig/Application.mk level adjustment can't handle these cases automatically:
> > ```
> > 1. The default stack size in Kconfig is a fine tune value, not DEFAULT_STACK_SIZE
> >
> > 2. The code call pthread_attr_setstack with the hard code value
> > But, the above cases happen frequently. Before we find a better solution, please don't revert this patch. It's a waste of time to fix the stack size issue on simulator.
>
> i feel what you really want is a way to ignore ~all user-specified stack addr / size. how do you think?
Yes, but just for sim. Simulator is a very special target, it doesn't make sense to let developer fix the stack overflow issue on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#discussion_r794360477
##########
File path: sched/Kconfig
##########
@@ -1828,12 +1828,14 @@ menu "Stack and heap information"
config DEFAULT_TASK_STACKSIZE
int "The default stack size for tasks"
+ default 65536 if ARCH_SIM
Review comment:
No, it's wrong to add the arch specific code in common place. Let me highlight INVIOLABLES rule here again:
https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md#modular-architecture
so please append the new patch in the same PR, so I can understand how it work.
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1025369003
> > > Yes, this is the only case we can't adjust, but this feature is seldom used.
> >
> >
> > i guess pthread_attr_setstacksize is seldom used too, right? or, is it somehow important?
>
> pthread_attr_setstacksize is important, but SIM_STACKSIZE_ADJUSTMENT can handle this case correctly. What this approach can't handle is pthread_attr_setstack with non NULL pointer, but this case is rarely used.
>
> > > All programs(except pthread_attr_setstack) which has the stack size with fine tuning or hardcode value can run on sim correctly. this approach is better than DEFAULT_STACK_SIZE. So, please revert both in this PR if you like.
> >
> >
> > my biggest complaint is the api inconsistency.
>
> api inconsistency only happen for pthread_attr_setstack with non NULL pointer. If you like, we can fix this inconsistency too: let up_use_stack ignore the preallocated stack pointer and allocate a new stack with the adjusted size. This fix is reasonable since the preallocated memory isn't difference from the dynamically allocated memory.
i've seen an app which is actually checking if it's running on the preallocated stack.
i'm not saying it's a good idea. i'm just saying such an app exists.
>
> > that's why i prefer Kconfig-level (or maybe Application.mk level) adjustment over this approach.
>
> Kconfig/Application.mk level adjustment can't handle these cases automatically:
>
> 1. The default stack size in Kconfig is a fine tune value, not DEFAULT_STACK_SIZE
>
> 2. The code call pthread_attr_setstack with the hard code value
>
>
> But, the above cases happen frequently. Before we find a better solution, please don't revert this patch. It's a waste of time to fix the stack size issue on simulator.
i feel what you really want is a way to ignore ~all user-specified stack addr / size.
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] yamt closed pull request #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
yamt closed pull request #5365:
URL: 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] xiaoxiang781216 commented on pull request #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1024054529
Copy from https://github.com/apache/incubator-nuttx/pull/4977:
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] yamt commented on a change in pull request #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#discussion_r794235712
##########
File path: sched/Kconfig
##########
@@ -1828,12 +1828,14 @@ menu "Stack and heap information"
config DEFAULT_TASK_STACKSIZE
int "The default stack size for tasks"
+ default 65536 if ARCH_SIM
Review comment:
if/once this revert is merged, we can consider introducing an indirection like ARCH_USE_xxx or whatever.
i personally feel it's simpler to have if ARCH_SIM here though.
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#discussion_r794360477
##########
File path: sched/Kconfig
##########
@@ -1828,12 +1828,14 @@ menu "Stack and heap information"
config DEFAULT_TASK_STACKSIZE
int "The default stack size for tasks"
+ default 65536 if ARCH_SIM
Review comment:
No, it's wrong to add the arch specific code in common place. Let me highlight INVIOLABLES rule here again:
https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md#modular-architecture
so please append the new patch in the same 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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1025447757
In this very special case, app code can add #ifndef CONFIG_ARCH_SIM to skip the check.
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1024054529
Copy from https://github.com/apache/incubator-nuttx/pull/4977:
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 issue. And let's modify all the hardcode stack size in application or add CONFIG_xxx_STACK_SIZE=65536 in all defconfig under boards/sim.
@yamt please revert DEFAULT_STAck_SIZE related change too, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on pull request #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1023925560
> > ## Summary
> > This reverts commit [6b5a7a7](https://github.com/apache/incubator-nuttx/commit/6b5a7a73ba76671c81dc70485befb5ed5e818fe0).
> > Because:
> >
> > * it broke existing configurations.
>
> could you point out which config broken?
eg. my local config, which is now using much larger stack than intended.
>
> > * it makes stack-related api very confusing.
> > eg. the "stack_size" argument of nxtask_init has different meanings
> > depending on "stack".
>
> If you don't like the stack size reported from ps, I can change the report size as user passed one, but allocate the bigger memory.
i was not talking about the report size.
nxtast_init takes "non adjusted" stack_size for stack==NULL
while it takes "adjusted" stack_size for stack!=NULL.
>
> > eg. pthread_attr_setstack and pthread_attr_setstacksize
> >
> > * stacksize is inherently arch-dependent.
>
> Yes, that's why the adjust is located in arch/sim.
>
> > the goal to share the same stack size configurations among archs
> > is questionable.
>
> As I explain before no one try to the same stack config for all arch, I just want to fix sim problem.
ok.
maybe "the goal to share the same stack size configurations between arch X and sim is questionable" is a better description?
>
> > ## Impact
> > sim
> > ## Testing
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1060065020
https://github.com/apache/incubator-nuttx/pull/5384 is acceptable for me
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1025249231
> Copy from #4977: 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 issue. And let's modify all the hardcode stack size in application or add CONFIG_xxx_STACK_SIZE=65536 in all defconfig under boards/sim. @yamt please revert DEFAULT_STAck_SIZE related change too, thanks.
i don't follow your logic.
DEFAULT_STACK_SIZE doesn't have the api inconsistency issue CONFIG_SIM_STACKSIZE_ADJUSTMENT has.
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1023908065
> ## Summary
> This reverts commit [6b5a7a7](https://github.com/apache/incubator-nuttx/commit/6b5a7a73ba76671c81dc70485befb5ed5e818fe0).
>
> Because:
>
> * it broke existing configurations.
could you point out which config broken?
> * it makes stack-related api very confusing.
> eg. the "stack_size" argument of nxtask_init has different meanings
> depending on "stack".
If you don't like the stack size reported from ps, I can change the report size as user passed one, but allocate the bigger memory.
> eg. pthread_attr_setstack and pthread_attr_setstacksize
> * stacksize is inherently arch-dependent.
Yes, that's why the adjust is located in arch/sim.
> the goal to share the same stack size configurations among archs
> is questionable.
As I explain before no one try to the same stack config for all arch, I just want to fix sim problem.
>
> ## Impact
> sim
>
> ## Testing
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1023908065
> ## Summary
> This reverts commit [6b5a7a7](https://github.com/apache/incubator-nuttx/commit/6b5a7a73ba76671c81dc70485befb5ed5e818fe0).
>
> Because:
>
> * it broke existing configurations.
could you point out which config broken?
> * it makes stack-related api very confusing.
> eg. the "stack_size" argument of nxtask_init has different meanings
> depending on "stack".
> eg. pthread_attr_setstack and pthread_attr_setstacksize
> * stacksize is inherently arch-dependent.
Yes, that's why the adjust is located in arch/sim.
> the goal to share the same stack size configurations among archs
> is questionable.
As I explain no one try to the same stack config for all arch, I just want to fix sim problem.
>
> ## Impact
> sim
>
> ## Testing
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1025354526
> > Yes, this is the only case we can't adjust, but this feature is seldom used.
>
> i guess pthread_attr_setstacksize is seldom used too, right? or, is it somehow important?
>
pthread_attr_setstacksize is important, but SIM_STACKSIZE_ADJUSTMENT can handle this case correctly. What this approach can't handle is pthread_attr_setstack with non NULL pointer, but this case is rarely used.
> > All programs(except pthread_attr_setstack) which has the stack size with fine tuning or hardcode value can run on sim correctly. this approach is better than DEFAULT_STACK_SIZE. So, please revert both in this PR if you like.
>
> my biggest complaint is the api inconsistency.
api inconsistency only happen for pthread_attr_setstack with non NULL pointer. If you like, we can fix this inconsistency too: let up_use_stack ignore the preallocated stack pointer and allocate a new stack with the adjusted size. This fix is reasonable since the preallocated memory isn't difference from the dynamically allocated memory.
> that's why i prefer Kconfig-level (or maybe Application.mk level) adjustment over this approach.
Kconfig/Application.mk level adjustment can't handle these cases automatically:
1. The default stack size in Kconfig is a fine tune value, not DEFAULT_STACK_SIZE
2. The code call pthread_attr_setstack with the hard code value
But, the above cases happen frequently. Before we find a better solution, please don't revert this patch. It's a waste of time to fix the stack size issue on simulator.
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1025447757
In this very special case, app code can add #ifndef CONFIG_ARCH_SIM to skip the check. If the application code write in this way, it has to deal the difference with either solution.
--
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 #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
yamt commented on pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#issuecomment-1025259202
> > > > ## Summary
> > > > This reverts commit [6b5a7a7](https://github.com/apache/incubator-nuttx/commit/6b5a7a73ba76671c81dc70485befb5ed5e818fe0).
> > > > Because:
> > > >
> > > > * it broke existing configurations.
> > >
> > >
> > > could you point out which config broken?
> >
> >
> > eg. my local config, which is now using much larger stack than intended.
>
> DEFAULT_STACK_SIZE on sim equals 65536 before the change, SIM_STACKSIZE_ADJUSTMENT equals 65536 after change. Both config has the similar functionality and same default value, so the stack consumption should be similar.
>
> > > > * it makes stack-related api very confusing.
> > > > eg. the "stack_size" argument of nxtask_init has different meanings
> > > > depending on "stack".
> > >
> > >
> > > If you don't like the stack size reported from ps, I can change the report size as user passed one, but allocate the bigger memory.
> >
> >
> > i was not talking about the report size.
> > nxtast_init takes "non adjusted" stack_size for stack==NULL while it takes "adjusted" stack_size for stack!=NULL.
>
> Yes, this is the only case we can't adjust, but this feature is seldom used.
i guess pthread_attr_setstacksize is seldom used too, right?
or, is it somehow important?
>
> > > > eg. pthread_attr_setstack and pthread_attr_setstacksize
> > > >
> > > > * stacksize is inherently arch-dependent.
> > >
> > >
> > > Yes, that's why the adjust is located in arch/sim.
> > > > the goal to share the same stack size configurations among archs
> > > > is questionable.
> > >
> > >
> > > As I explain before no one try to the same stack config for all arch, I just want to fix sim problem.
> >
> >
> > ok. maybe "the goal to share the same stack size configurations between arch X and sim is questionable" is a better description?
>
> All programs(except pthread_attr_setstack) which has the stack size with fine tuning or hardcode value can run on sim correctly. this approach is better than DEFAULT_STACK_SIZE. So, please revert both in this PR if you like.
my biggest complaint is the api inconsistency.
that's why i prefer Kconfig-level (or maybe Application.mk level) adjustment over 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 commented on a change in pull request #5365: Revert "sim: add CONFIG_SIM_STACKSIZE_ADJUSTMENT to reduce variability"
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5365:
URL: https://github.com/apache/incubator-nuttx/pull/5365#discussion_r794219857
##########
File path: sched/Kconfig
##########
@@ -1828,12 +1828,14 @@ menu "Stack and heap information"
config DEFAULT_TASK_STACKSIZE
int "The default stack size for tasks"
+ default 65536 if ARCH_SIM
Review comment:
please don't add sim specific setting in common place.
##########
File path: sched/Kconfig
##########
@@ -1828,12 +1828,14 @@ 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
Review comment:
same, remove.
--
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