You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/08/11 13:35:16 UTC
[GitHub] [incubator-nuttx] johannes-nivus opened a new pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
johannes-nivus opened a new pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562
## Summary
The adjusted stack size should be without task local storage overhead.
## Impact
E.g. external tools for task aware debugging are using adjusted stack size for stack debugging (stack coloration).
They interpret tls as used stack.
## Testing
Tested with Kinetis Freedom-K28f and Lauterbach Trace32 NuttX awareness.
I searched the source code for possible side effects of the change but found none. According to documentation the variable adj_stack_size is only there for debugging purposes. (Please correct me if I'm wrong).
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677852607
If you are comfortable, please remove the DRAFT indication and I will merge the code now.
----------------------------------------------------------------
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] johannes-nivus commented on a change in pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468605220
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
Review comment:
I could, but I cannot test.
Perhaps we should wait for some more comments concerning the change in general...
----------------------------------------------------------------
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 edited a comment on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677852607
If you are comfortable, please remove the DRAFT indication and I will merge the code now.
Do not include any additional changes for any other architectures that you cannot test. And do not introduce any coupling between architectures.
----------------------------------------------------------------
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] johannes-nivus commented on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677867664
I'm fine with your recommendation, and I also don't like adding code I can't test.
We're not in a hurry. If you don't mind, I remove the draft flag tomorrow and open the proposed issue.
But allow a last comment on my last proposal:
I didn't want to introduce coupling but doubling:
I just wanted to write kind of stack function templates which can be copied to the arch, configured and tested by the commiter (not by an automated process).
Which means all architectures still have their own standalone fileset, which can be freely changed afterwards, but it would help start resolving this issue throughout the archs.
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-675980690
@johannes-nivus the change looks good to me, could you apply the change to other 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677846000
>
>
> @xiaoxiang781216 @patacongo
> I have an idea:
> I'll write generic versions for createstack, usestack, checkstack and perhaps other stack functions with parameters for full/empty, descending/ascending and alignment which can then be called from the arch functions with the correct parameter set.
> What do you think about?
Please DO NO DO THAT. That violates the modular architecture of the architectures. A change like that must never come into to the OS!!!!!!!!!!!!!!!!
Please see the top-level INVIOLABLES.txt and also https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=139629399
Lets avoid thinking in this way. Please think of all architectures as fully self contained and NOT couple through any common logic like this. In this case, duplication of code is a very good thing.
----------------------------------------------------------------
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] johannes-nivus edited a comment on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus edited a comment on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-672081823
At all:
I'll rework the PR since I'm sure it is correct to signal the actual available amount of cache inside adj_stack_size.
But I must admit, it is not perfectly thought out for now.
Besides: I had a look at the other architectures as well, and there are subtle differences...
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r469976309
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -102,26 +102,26 @@
int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
{
- /* Add the size of the TLS information structure */
+ size_t alloc_size;
- stack_size += sizeof(struct tls_info_s);
+ /* Add the size of the TLS information structure and align */
+
+ alloc_size = STACK_ALIGN_UP(stack_size + sizeof(struct tls_info_s));
#ifdef CONFIG_TLS_ALIGNED
/* The allocated stack size must not exceed the maximum possible for the
* TLS feature.
*/
- DEBUGASSERT(stack_size <= TLS_MAXSTACK);
- if (stack_size >= TLS_MAXSTACK)
+ DEBUGASSERT(alloc_size <= TLS_MAXSTACK);
Review comment:
Isn't this assertion backward?
```suggestion
DEBUGASSERT(alloc_size > TLS_MAXSTACK);
```
We will fail the assertion if the condition is false.
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677856773
>
>
> Then I'd like to modify my suggestion:
> Write generic functions, configurable by in file preprocessor macros and use them in the archs.
> I just want to avoid solving similar problems in several different ways.
NO. Please do not do that. Do NOT introduce any coupling between architectures. That MUST NOT HAPPEN. No change like that should ever be merged. And it it is, I will revert 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468619696
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
Review comment:
> But I think arm_vfork.c is save. (I don't see it beeing set there).
I was surprised to see it there too. Turns out, my grep was just bad and caught this link in arm_vfork.c:
stacksize = parent->adj_stack_size + CONFIG_STACK_ALIGNMENT - 1;
It has bother adj_stack_size and " = ". Sorry I was not careful. My mistake.
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
davids5 commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-673560335
@johannes-nivus - the align on 8 was critical in the past for calling convention. Is it maintained here in?
----------------------------------------------------------------
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] johannes-nivus commented on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677825124
@xiaoxiang781216
I'll try to do the changes one after the other over the next days. But I can't test them.
I have to look for stack layout (full/empty)-(descending/ascending) in literature and implement accordingly.
E.g. Arm is currently implemented as empty-descending (on master) but is in fact full-descending.
I'll convert the PR again back to a draft.
----------------------------------------------------------------
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 a change in pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r472870570
##########
File path: arch/arm/src/common/arm_usestack.c
##########
@@ -105,51 +102,54 @@ int up_use_stack(struct tcb_s *tcb, void *stack, size_t stack_size)
up_release_stack(tcb, tcb->flags & TCB_FLAG_TTYPE_MASK);
}
+ /* The ARM uses a push-down stack: the stack grows toward lower
+ * addresses in memory. The stack pointer register, points to
+ * the lowest, valid work address (the "top" of the stack). Items
+ * on the stack are referenced as positive word offsets from sp.
+ */
+
+ /* We align all sizes and pointer to CONFIG_STACK_ALIGNMENT.
+ * Since the stack ptr is decremented before
+ * the first write, we can directly save our variables to struct
+ * tcb_s.
+ */
+
/* Save the new stack allocation */
tcb->stack_alloc_ptr = stack;
- /* The ARM uses a push-down stack: the stack grows toward lower addresses
- * in memory. The stack pointer register, points to the lowest, valid
- * work address (the "top" of the stack). Items on the stack are
- * referenced as positive word offsets from sp.
- */
+ /* Align stack top */
- top_of_stack = (uint32_t)tcb->stack_alloc_ptr + stack_size - 4;
+ tcb->adj_stack_ptr =
+ (FAR void *)STACK_ALIGN_DOWN((uintptr_t)stack + stack_size);
- /* The ARM stack must be aligned to 8-byte alignment for EABI.
- * If necessary top_of_stack must be rounded down to the next
- * boundary
- */
+ /* Offset by tls_size */
- top_of_stack = STACK_ALIGN_DOWN(top_of_stack);
+ stack = (FAR void *)((uintptr_t)stack + sizeof(struct tls_info_s));
- /* The size of the stack in bytes is then the difference between
- * the top and the bottom of the stack (+4 because if the top
- * is the same as the bottom, then the size is one 32-bit element).
- * The size need not be aligned.
- */
+ /* Is there enough room for at least TLS ? */
- size_of_stack = top_of_stack - (uint32_t)tcb->stack_alloc_ptr + 4;
+ if ((uintptr_t)stack <= (uintptr_t)tcb->adj_stack_ptr)
Review comment:
could we change to:
```
if ((uintptr_t)stack > (uintptr_t)tcb->adj_stack_ptr)
{
return -ENOMEM;
}
```
to mininize the code change.
----------------------------------------------------------------
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] johannes-nivus commented on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-672081823
@all
I'll rework the PR since I'm sure it is correct to signal the actual available amount of cache inside adj_stack_size.
But I must admit, it is not perfectly thought out for now.
Besides: I had a look at the other architectures as well, and there are subtle differences...
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r469976309
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -102,26 +102,26 @@
int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
{
- /* Add the size of the TLS information structure */
+ size_t alloc_size;
- stack_size += sizeof(struct tls_info_s);
+ /* Add the size of the TLS information structure and align */
+
+ alloc_size = STACK_ALIGN_UP(stack_size + sizeof(struct tls_info_s));
#ifdef CONFIG_TLS_ALIGNED
/* The allocated stack size must not exceed the maximum possible for the
* TLS feature.
*/
- DEBUGASSERT(stack_size <= TLS_MAXSTACK);
- if (stack_size >= TLS_MAXSTACK)
+ DEBUGASSERT(alloc_size <= TLS_MAXSTACK);
Review comment:
Isn't this assertion backward?
```suggestion
DEBUGASSERT(alloc_size > TLS_MAXSTACK);
```
We will take the assertion if the condition is false.
----------------------------------------------------------------
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] johannes-nivus commented on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-673584553
Side effect:
With the new stack start calclulation it isn't needed to differentiate between interrupt and task stack in arm_checkstack any more.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468622371
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
+ sizeof(struct tls_info_s);
Review comment:
Maybe"
```suggestion
((sizeof(struct tls_info_s) + 7) & ~7);
```
----------------------------------------------------------------
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] johannes-nivus commented on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-675856604
@patacongo @xiaoxiang781216
Is this PR still considered?
Any questions or suggestions?
----------------------------------------------------------------
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 edited a comment on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677852607
If you are comfortable, please remove the DRAFT indication and I will merge the code now.
Do not include any additional changes for any other architectures that you cannot test. And do not introduce any coupling between architectures. It is sufficient to simply open an Issue and indicate that this change must be appled to the other architectures.
----------------------------------------------------------------
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 edited a comment on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677856773
>
>
> Then I'd like to modify my suggestion:
> Write generic functions, configurable by in file preprocessor macros and use them in the archs.
> I just want to avoid solving similar problems in several different ways.
NO. Please do not do that. Do NOT introduce any coupling between architectures. That MUST NOT HAPPEN. No change like that should ever be merged. And it it is, I will revert it.
This is not negotiable: NO COUPLING of any kind must be introduced. That is absolutely forbidden by the architecural principles of the OS. I understand that you want to cut corners and do less work. But don't do this. It is work. No shitty enginerring permitted here that introduces coupling.
And do not change any architectures that you cannot test! That is a very bad engineering practice.
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677858887
My recommendation:
* Open an Issue indicating that the the change of this PR must be implemented AND TESTED in other architectures.
* Remove the DRAFT
* I will merge the ARM only change.
Don't let people force you into cutting corners or changing code without testing 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] johannes-nivus edited a comment on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus edited a comment on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-673569533
>
> the align on 8 was critical in the past for calling convention. Is it maintained here in?
Yes it is:
In createstack the total size (stack + tls) is up aligned to 8 and then allocated with memalign 8. Which means adj_stack_ptr is 8 aligned.
In usestack the end adress is calulated and then down aligned. Which also means adj_stack_ptr is 8 aligned.
The alignment of the bottom address is ignored (which is ok).
Johannes
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468621366
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
+ sizeof(struct tls_info_s);
Review comment:
What if sizeof(struct tls_info_s) is not an multiple of 8 bytes. Does it matter if the far end of the adjusted stack is not aligned properly?
----------------------------------------------------------------
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] johannes-nivus closed pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus closed pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562
----------------------------------------------------------------
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] johannes-nivus commented on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-673569533
>
> the align on 8 was critical in the past for calling convention. Is it maintained here in?
Yes it is:
In createstack the total size (stack + tls) is up aligned to 8 and the allocated with memalign 8. Which means adj_stack_ptr is 8 aligned.
In usestack the end adress is calulated, and then down aligned. Which also means adj_stack_ptr is 8 aligned.
The alignment of the bottom address is ignored (which is ok).
Johannes
----------------------------------------------------------------
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 a change in pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468592514
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
Review comment:
@johannes-nivus could you apply the same change to other 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468622371
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
+ sizeof(struct tls_info_s);
Review comment:
Maybe:
```suggestion
((sizeof(struct tls_info_s) + 7) & ~7);
```
But this is probably not necessary. Just something to think about. I don't think it is required for the PR to be merged.
----------------------------------------------------------------
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] Ouss4 commented on a change in pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468646249
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
Review comment:
https://github.com/apache/incubator-nuttx/blob/b256b2055f7e519104052499e6855013cb06cc5f/arch/arm/src/common/arm_createstack.c#L237-L238
Then, this needs to be adjusted too? No need to take sizeof(tls_info_s) off anymore.
----------------------------------------------------------------
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] johannes-nivus commented on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677843538
@xiaoxiang781216 @patacongo
I have an idea:
I'll write generic versions for createstack, usestack and perhaps other stack functions with parameters for full/empty, descending/ascending and alignment which can then be called from the arch functions with the correct parameter set.
What do you think about?
----------------------------------------------------------------
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] johannes-nivus commented on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677854219
Then I'd like to modify my suggestion:
Write generic functions, configurable by preprocessor macros, and copy them to the architectures.
I want to avoid solving similar problems always
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468606251
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
Review comment:
The same change would be needed in arm_usestack.c.
adj_stack_size is also set in arm_vfork.c, I would need to review to understand what, if anything, is needed.
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677852212
I am merging this now because the remaining comments are unjust.
----------------------------------------------------------------
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 removed a comment on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677852212
I am merging this now because the remaining comments are unjust.
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468622371
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
+ sizeof(struct tls_info_s);
Review comment:
Maybe:
```suggestion
((sizeof(struct tls_info_s) + 7) & ~7);
```
But this is probably not necessary. Just something to think about and I don't think it is required for the PR to be merged.
----------------------------------------------------------------
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 merged pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo merged pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562
----------------------------------------------------------------
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] johannes-nivus commented on a change in pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468616456
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
Review comment:
Yes you're right. I missed arm_usestack.c. But I think arm_vfork.c is save. (I don't see it beeing set there).
----------------------------------------------------------------
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] johannes-nivus commented on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-672883843
@patacongo
I rewrote the createstack and usestack functions.
Alignment is assured by memalign.
There was logic inside that made sure that the initial highest stack address (adj_stack_ptr) is within the allocated range.
I think this is not neccessary, because stack pointer is decremented before write during push.
----------------------------------------------------------------
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 edited a comment on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677852607
If you are comfortable, please remove the DRAFT indication and I will merge the code now.
Do not include any additional changes for any other architectures that you cannot 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r468619696
##########
File path: arch/arm/src/common/arm_createstack.c
##########
@@ -220,7 +220,8 @@ int up_create_stack(FAR struct tcb_s *tcb, size_t stack_size, uint8_t ttype)
/* Save the adjusted stack values in the struct tcb_s */
tcb->adj_stack_ptr = (uint32_t *)top_of_stack;
- tcb->adj_stack_size = size_of_stack;
+ tcb->adj_stack_size = size_of_stack -
Review comment:
> But I think arm_vfork.c is save. (I don't see it beeing set there).
I was surprised to see it there too. Turns out, my grep was just bad and caught this line in arm_vfork.c:
stacksize = parent->adj_stack_size + CONFIG_STACK_ALIGNMENT - 1;
It has bother adj_stack_size and " = ". Sorry I was not careful. My mistake.
----------------------------------------------------------------
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] johannes-nivus edited a comment on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus edited a comment on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677843538
@xiaoxiang781216 @patacongo
I have an idea:
I'll write generic versions for createstack, usestack, checkstack and perhaps other stack functions with parameters for full/empty, descending/ascending and alignment which can then be called from the arch functions with the correct parameter set.
What do you think about?
----------------------------------------------------------------
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] johannes-nivus edited a comment on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus edited a comment on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677854219
Then I'd like to modify my suggestion:
Write generic functions, configurable by in file preprocessor macros and use them in the archs.
I just want to avoid solving similar problems in several different ways.
----------------------------------------------------------------
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] johannes-nivus commented on a change in pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus commented on a change in pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#discussion_r474188960
##########
File path: arch/arm/src/common/arm_usestack.c
##########
@@ -105,51 +102,54 @@ int up_use_stack(struct tcb_s *tcb, void *stack, size_t stack_size)
up_release_stack(tcb, tcb->flags & TCB_FLAG_TTYPE_MASK);
}
+ /* The ARM uses a push-down stack: the stack grows toward lower
+ * addresses in memory. The stack pointer register, points to
+ * the lowest, valid work address (the "top" of the stack). Items
+ * on the stack are referenced as positive word offsets from sp.
+ */
+
+ /* We align all sizes and pointer to CONFIG_STACK_ALIGNMENT.
+ * Since the stack ptr is decremented before
+ * the first write, we can directly save our variables to struct
+ * tcb_s.
+ */
+
/* Save the new stack allocation */
tcb->stack_alloc_ptr = stack;
- /* The ARM uses a push-down stack: the stack grows toward lower addresses
- * in memory. The stack pointer register, points to the lowest, valid
- * work address (the "top" of the stack). Items on the stack are
- * referenced as positive word offsets from sp.
- */
+ /* Align stack top */
- top_of_stack = (uint32_t)tcb->stack_alloc_ptr + stack_size - 4;
+ tcb->adj_stack_ptr =
+ (FAR void *)STACK_ALIGN_DOWN((uintptr_t)stack + stack_size);
- /* The ARM stack must be aligned to 8-byte alignment for EABI.
- * If necessary top_of_stack must be rounded down to the next
- * boundary
- */
+ /* Offset by tls_size */
- top_of_stack = STACK_ALIGN_DOWN(top_of_stack);
+ stack = (FAR void *)((uintptr_t)stack + sizeof(struct tls_info_s));
- /* The size of the stack in bytes is then the difference between
- * the top and the bottom of the stack (+4 because if the top
- * is the same as the bottom, then the size is one 32-bit element).
- * The size need not be aligned.
- */
+ /* Is there enough room for at least TLS ? */
- size_of_stack = top_of_stack - (uint32_t)tcb->stack_alloc_ptr + 4;
+ if ((uintptr_t)stack <= (uintptr_t)tcb->adj_stack_ptr)
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] johannes-nivus edited a comment on pull request #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
johannes-nivus edited a comment on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-673569533
>
> the align on 8 was critical in the past for calling convention. Is it maintained here in?
Yes it is:
In createstack the total size (stack + tls) is up aligned to 8 and the allocated with memalign 8. Which means adj_stack_ptr is 8 aligned.
In usestack the end adress is calulated, and then down aligned. Which also means adj_stack_ptr is 8 aligned.
The alignment of the bottom address is ignored (which is ok).
Johannes
----------------------------------------------------------------
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 #1562: arm_createstack.c: Save tcb->adj_stack_size without tls overhead.
Posted by GitBox <gi...@apache.org>.
patacongo commented on pull request #1562:
URL: https://github.com/apache/incubator-nuttx/pull/1562#issuecomment-677851809
I personally believe that the ARM change is independent and we should NOT hold up merging the change because additional functionality has been unfairly dumped onto the contributor. That is wrong. We should not do that.
This encourages the contributor to cut corners and propose shitty modifications. And the submitter cannot test these changes ... we should not let untested changes into there repository either.
I think you are treating the contributor unfairly. Let him merge the good, tested. ARM changes without forcing him to do a bunch of blind, shit changes to other architectures that he is unfamiliar with. That is stupid.
----------------------------------------------------------------
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