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/04 08:31:05 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
anchao opened a new pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154
## Summary
net/usrsock: replace xid type to uint64_t to avoid limitation of connections
## Impact
remove connect index logic and use usrsock_conn instance address
## Testing
usrsock rpmsg 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 #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778540698
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
Look like you use a very old arch(armv4t) which doesn't support the unaligned access at the hardware level. I compile your code with `-march=armv7e-m -mtune=cortex-m4 -mthumb -mfloat-abi=soft`:
```
.arch armv7e-m
.eabi_attribute 20, 1
.eabi_attribute 21, 1
.eabi_attribute 23, 3
.eabi_attribute 24, 1
.eabi_attribute 25, 1
.eabi_attribute 26, 1
.eabi_attribute 30, 6
.eabi_attribute 34, 1
.eabi_attribute 18, 4
.file "test.c"
.text
.align 1
.global f1
.syntax unified
.thumb
.thumb_func
.fpu softvfp
.type f1, %function
f1:
@ args = 0, pretend = 0, frame = 8
@ frame_needed = 1, uses_anonymous_args = 0
@ link register save eliminated.
push {r7}
sub sp, sp, #12
add r7, sp, #0
str r0, [r7, #4]
ldr r3, [r7, #4]
ldr r3, [r3] @ unaligned
mov r0, r3
adds r7, r7, #12
mov sp, r7
@ sp needed
pop {r7}
bx lr
.size f1, .-f1
.align 1
.global f2
.syntax unified
.thumb
.thumb_func
.fpu softvfp
.type f2, %function
f2:
@ args = 0, pretend = 0, frame = 8
@ frame_needed = 1, uses_anonymous_args = 0
@ link register save eliminated.
push {r7}
sub sp, sp, #12
add r7, sp, #0
str r0, [r7, #4]
ldr r3, [r7, #4]
ldr r3, [r3]
mov r0, r3
adds r7, r7, #12
mov sp, r7
@ sp needed
pop {r7}
bx lr
.size f2, .-f2
.ident "GCC: (GNU Arm Embedded Toolchain 10.3-2021.07) 10.3.1 20210621 (release)"
```
You can see compiler always load 4 bytes regardless whether packed tag.
--
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 #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r777941456
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -96,8 +96,8 @@ enum usrsock_message_types_e
begin_packed_struct struct usrsock_request_common_s
{
- int8_t reqid;
- uint8_t xid;
+ uint64_t xid;
+ int32_t reqid;
Review comment:
int16_t reqid?
--
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] anchao edited a comment on pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
anchao edited a comment on pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#issuecomment-1005480711
> @xiaoxiang781216 @anchao Yes, I use the latest apps but the renew command still does not work with gs2200m daemon.
@masayuki2009 san, could you please to try this PR? it seems the preamble scratch buffer is unenough
https://github.com/apache/incubator-nuttx-apps/pull/959
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778182912
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
@xiaoxiang781216 that is exactly what I have started from. The usecase is either native alignment or `packed`. In both cases any `reserved` does not make any sense because in case of native alignment all "reserved" will be added by the compiler and in case of `packed` to padding will be done.
Maybe I'm missing something, but please point me to why do we need to add any `reserved` into the structures?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778083404
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -96,8 +96,9 @@ enum usrsock_message_types_e
begin_packed_struct struct usrsock_request_common_s
{
- int8_t reqid;
- uint8_t xid;
+ uint64_t xid;
+ int8_t reqid;
+ int8_t reserved;
Review comment:
Why do we need `reserved` for packed structure?
--
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] anchao commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r777959547
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -96,8 +96,8 @@ enum usrsock_message_types_e
begin_packed_struct struct usrsock_request_common_s
{
- int8_t reqid;
- uint8_t xid;
+ uint64_t xid;
+ int32_t reqid;
Review comment:
restore the reqid type to int8_t, and add reserved byte to keep the aligned access
--
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 #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778178526
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
Yes, SoC which mix 32bit and 64bit arch is definitely the case we must to support, that's why:
1. All struct shared by multi-core add [begin|end]_packed_struct to avoid the different alignment on 32bit/64bit arch
2. All field use the explicit size type(e.g. int8_t/int16_t/int32_t) instead of (int/long/size_t/intptr_t)
In this particular case, @anchao change xid from uint8_t to uint64_t, because he want to save conn self(pionter) into xid, and then uint64_t is the most suitable choice compare with uintptr_t or uint32_t.
--
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] anchao commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778120305
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
usrsock is an IPC transparent transmission protocol, sometimes usrsock will communicate with other CPU through openAMP, this form is used in the asymmetric CPU core, such as CEVA(tl420,16bits)/XTENSA etc, due to the hardware and toolchain is inconsistency, most of time we will define aligned member type for the IPC structure definition, On some platforms, which not only solves the alignment issue, and also improves performance.
--
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] masayuki2009 commented on pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#issuecomment-1005399203
>Do you have this patch on apps side: apache/incubator-nuttx-apps#956
@xiaoxiang781216 @anchao
Yes, I use the latest apps but the renew command still does not work with gs2200m daemon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778149092
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
Such so called "optimization" costed me a lot when I was building communication in multi-core system between 32-bit and 64-bit cores :) and I would not recommend to apply it in any types generic communication.
I see that no "usrsock_swapX" interface is not available, so the interface currently supports messaging between same endian systems (that is friendly speaking not always true for some multi-core systems).
I will let other reviewers to comment on this.
What I'm curious why in some cases the elements are sorted in type size increase and in other with type size decrease? Like here and
```
begin_packed_struct struct usrsock_request_common_s
{
uint64_t xid;
int8_t reqid;
int8_t reserved;
} end_packed_struct;
```
And
```
begin_packed_struct struct usrsock_request_sendto_s
{
struct usrsock_request_common_s head;
int16_t usockid;
int32_t flags;
uint32_t buflen;
uint16_t addrlen;
} end_packed_struct;
begin_packed_struct struct usrsock_request_recvfrom_s
{
struct usrsock_request_common_s head;
int16_t usockid;
int32_t flags;
uint32_t max_buflen;
uint16_t max_addrlen;
} end_packed_struct;
```
even do not care about sorting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778171266
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
The different endianess is a weak argument, I agree, but mix of 32 bit and 64 bit will be more common usecase when more and more RISC-V based chips will be produced. I do not recall, but can search if TI has options with mix of 32 bit ARM and 64 bit DSP.
So sorting will might be impacted by the alignment of the particular core.
--
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 #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778201656
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
> @xiaoxiang781216 that is exactly what I have started from. The usecase is either native alignment
We can't leverage the compiler do the native alignment as discuss before since the different compiler may align to the different place.
> or `packed`.
packed can ensure all compiler place the field in the same location which is important for IPC.
> In Bothell cases any `reserved` does not make any sense because in case of native alignment all "reserved" will be added by the compiler and in case of `packed` to padding will be done.
packed will remove any pad from the struct, so if we don't add some reserved field here, many fields will become unalignment. This will impact the performance and code size:
1. If the hardware doesn't support unaligned access, compiler will split the bus transaction into small and aligned, which increase size and decrease performance both.
2. If the hardware does support unaligned access, compiler will issue the access directly, and hardware will split it into small and aligned if the access isn't aligned. In this case, only the performance get impact.
> Maybe I'm missing something, but please point me to why do we need to add any `reserved` into the structures?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778098949
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
In case is you use packed structures compiler does not take CPU alignment into account that generate "special" code to access unaligned data
--
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] anchao commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778095657
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
keep the data alignment and improve the performance
--
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 #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778164713
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
> In case if you use packed structures compiler does not take CPU alignment into account that generate "special" code to access unaligned data
Most modem CPU(e.g. ARMv7 A/R/M and RISCV) support the unalignment access at the hardware level, but the unaligned access still need the additional bus transaction than the aligned access. Manual alignment here could improve the performance.
> Such so called "optimization" costed me a lot when I was building communication in multi-core system between 32-bit and 64-bit cores :) and I would not recommend to apply it in any types generic communication.
>
> I see that no "usrsock_swapX" interface is not available, so the interface currently supports messaging between same endian systems (that is friendly speaking not always true for some multi-core systems).
>
Multi-core system is more fixed than the traditional network environment. The IC designer normally enforce all CPU/MCU/DSP share the same endianness in one SOC. Since multi-core has much better the speed/latency than multi-machine, it is important to avoid the unnecessary swapX in this case.
> I will let other reviewers to comment on this.
>
> What I'm curious why in some cases the elements are sorted in type size increase and in other with type size decrease? Like here and
>
> ```
> begin_packed_struct struct usrsock_request_common_s
> {
> uint64_t xid;
> int8_t reqid;
> int8_t reserved;
> } end_packed_struct;
> ```
>
> And
>
> ```
> begin_packed_struct struct usrsock_request_sendto_s
> {
> struct usrsock_request_common_s head;
> int16_t usockid;
> int32_t flags;
> uint32_t buflen;
> uint16_t addrlen;
> } end_packed_struct;
> begin_packed_struct struct usrsock_request_recvfrom_s
> {
> struct usrsock_request_common_s head;
> int16_t usockid;
> int32_t flags;
> uint32_t max_buflen;
> uint16_t max_addrlen;
> } end_packed_struct;
> ```
>
> even do not care about sorting.
Since the message/request split into two structure, ordering the field by reversed size in each structure can't get the optimized result. For example:
1. usrsock_request_common_s has 10Byte size
2. First field of usrsock_request_sendto_s/usrsock_request_recvfrom_s is int16_t which is aligned correctly here
but int32_t isn't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778319423
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
> We can't leverage the compiler do the native alignment as discuss before since the different compiler may align to the different place.
Do you have some examples where two different compilers apply different alignments when compiled for a same target? Is this is a theoretical compiler or you are referring to compiler options for default alignment? I mean that compiler can't violate the rules for target architecture and setting default alignment will possibly make structures smaller but will most probably lead to generation of the slower code.
> packed will remove any pad from the struct, so if we don't add some reserved field here, many fields will become unaligned. This will impact the performance and code size:
> 1. If the hardware doesn't support unaligned access, compiler will split the bus transaction into small and aligned, which increase code size and decrease performance both.
> 2. If the hardware does support unaligned access, compiler will issue the access directly, and hardware will split it into small and aligned if the access isn't aligned. In this case, only the performance get impact.
That is true but I do not agree with "if we don't add some reserved field here, many fields will become unaligned" in case of `packed`. Here is a test code and generated assembly:
test.c
```
struct s1 {
unsigned int a;
} __attribute__((packed));
struct s2 {
unsigned int a;
};
unsigned int f1(struct s1 *s)
{
return s->a;
}
unsigned int f2(struct s2 *s)
{
return s->a;
}
```
test.s
```
.cpu arm7tdmi
.arch armv4t
.fpu softvfp
.eabi_attribute 20, 1
.eabi_attribute 21, 1
.eabi_attribute 23, 3
.eabi_attribute 24, 1
.eabi_attribute 25, 1
.eabi_attribute 26, 1
.eabi_attribute 30, 6
.eabi_attribute 34, 0
.eabi_attribute 18, 4
.file "test.c"
.text
.align 1
.global f1
.syntax unified
.code 16
.thumb_func
.type f1, %function
f1:
@ Function supports interworking.
@ args = 0, pretend = 0, frame = 8
@ frame_needed = 1, uses_anonymous_args = 0
push {r7, lr}
sub sp, sp, #8
add r7, sp, #0
str r0, [r7, #4]
ldr r3, [r7, #4]
ldrb r2, [r3]
ldrb r1, [r3, #1]
lsls r1, r1, #8
orrs r2, r1
ldrb r1, [r3, #2]
lsls r1, r1, #16
orrs r2, r1
ldrb r3, [r3, #3]
lsls r3, r3, #24
orrs r3, r2
movs r0, r3
mov sp, r7
add sp, sp, #8
@ sp needed
pop {r7}
pop {r1}
bx r1
.size f1, .-f1
.align 1
.global f2
.syntax unified
.code 16
.thumb_func
.type f2, %function
f2:
@ Function supports interworking.
@ args = 0, pretend = 0, frame = 8
@ frame_needed = 1, uses_anonymous_args = 0
push {r7, lr}
sub sp, sp, #8
add r7, sp, #0
str r0, [r7, #4]
ldr r3, [r7, #4]
ldr r3, [r3]
movs r0, r3
mov sp, r7
add sp, sp, #8
@ sp needed
pop {r7}
pop {r1}
bx r1
.size f2, .-f2
.ident "GCC: (GNU Arm Embedded Toolchain 10.3-2021.10) 10.3.1 20210824 (release)"
```
As you can see there is no "reserved field", but in case of `packed` compiler still splits loads into multiple `ldrb`s + `orrs`s to make a value reassembly. The reason is that `packed` types can be spaced at any address in memory and even adding a "reserved field" will not lead to a better code generation.
Sorry for a long discussion, this is because I was looking into the code from GitHub web view only till now. Now I had an opportunity to checkout the code and I see what @anchao is trying to do here trying to achieve whole message best alignment. I was missing the inheritance part for common headers. Now things are clear to me. Thanks again for spending time explaining to 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] xiaoxiang781216 commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778540698
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
Look like you use a very old arch(armv4t) which doesn't support the unaligned access at the hardware level. I compile your code with `-march=armv7e-m -mtune=cortex-m4 -mthumb -mfloat-abi=soft`:
```
.arch armv7e-m
.eabi_attribute 20, 1
.eabi_attribute 21, 1
.eabi_attribute 23, 3
.eabi_attribute 24, 1
.eabi_attribute 25, 1
.eabi_attribute 26, 1
.eabi_attribute 30, 6
.eabi_attribute 34, 1
.eabi_attribute 18, 4
.file "test.c"
.text
.align 1
.global f1
.syntax unified
.thumb
.thumb_func
.fpu softvfp
.type f1, %function
f1:
@ args = 0, pretend = 0, frame = 8
@ frame_needed = 1, uses_anonymous_args = 0
@ link register save eliminated.
push {r7}
sub sp, sp, #12
add r7, sp, #0
str r0, [r7, #4]
ldr r3, [r7, #4]
ldr r3, [r3] @ unaligned
mov r0, r3
adds r7, r7, #12
mov sp, r7
@ sp needed
pop {r7}
bx lr
.size f1, .-f1
.align 1
.global f2
.syntax unified
.thumb
.thumb_func
.fpu softvfp
.type f2, %function
f2:
@ args = 0, pretend = 0, frame = 8
@ frame_needed = 1, uses_anonymous_args = 0
@ link register save eliminated.
push {r7}
sub sp, sp, #12
add r7, sp, #0
str r0, [r7, #4]
ldr r3, [r7, #4]
ldr r3, [r3]
mov r0, r3
adds r7, r7, #12
mov sp, r7
@ sp needed
pop {r7}
bx lr
.size f2, .-f2
.ident "GCC: (GNU Arm Embedded Toolchain 10.3-2021.07) 10.3.1 20210621 (release)"
```
You can see compiler always load 4 bytes regardless whether packed tag. Both dissemble code show that my conclusion is the right.
--
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 #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154
--
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 #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r777940865
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +227,8 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ uint64_t xid;
+ int32_t result;
Review comment:
Change to:
```
uint16_t reserved;
int32_t result;
uint64_t xid;
```
to keep the alignment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778098949
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
In case if you use packed structures compiler does not take CPU alignment into account that generate "special" code to access unaligned data
--
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] anchao commented on pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#issuecomment-1005480711
> @xiaoxiang781216 @anchao Yes, I use the latest apps but the renew command still does not work with gs2200m daemon.
@masayuki2009 san, could you please to try this PR? it seems the preamble scratch buffer is unenough
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778080793
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
Why do we need `reserved` for packed structure?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778182912
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
@xiaoxiang781216 that is exactly what I have started from. The usecase is either native alignment or `packed`. In both cases any `reserved` does not make any sense because in case of native alignment all "reserved" will be added by the compiler and in case of `packed` no padding will be done.
Maybe I'm missing something, but please point me to why do we need to add any `reserved` into the structures?
--
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 #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778540698
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
Look like you use a very old arch(armv4t) which doesn't support the unaligned access at the hardware level. I compile your code with -march=armv7e-m -mtune=cortex-m4 -mthumb -mfloat-abi=soft:
```
.arch armv7e-m
.eabi_attribute 20, 1
.eabi_attribute 21, 1
.eabi_attribute 23, 3
.eabi_attribute 24, 1
.eabi_attribute 25, 1
.eabi_attribute 26, 1
.eabi_attribute 30, 6
.eabi_attribute 34, 1
.eabi_attribute 18, 4
.file "test.c"
.text
.align 1
.global f1
.syntax unified
.thumb
.thumb_func
.fpu softvfp
.type f1, %function
f1:
@ args = 0, pretend = 0, frame = 8
@ frame_needed = 1, uses_anonymous_args = 0
@ link register save eliminated.
push {r7}
sub sp, sp, #12
add r7, sp, #0
str r0, [r7, #4]
ldr r3, [r7, #4]
ldr r3, [r3] @ unaligned
mov r0, r3
adds r7, r7, #12
mov sp, r7
@ sp needed
pop {r7}
bx lr
.size f1, .-f1
.align 1
.global f2
.syntax unified
.thumb
.thumb_func
.fpu softvfp
.type f2, %function
f2:
@ args = 0, pretend = 0, frame = 8
@ frame_needed = 1, uses_anonymous_args = 0
@ link register save eliminated.
push {r7}
sub sp, sp, #12
add r7, sp, #0
str r0, [r7, #4]
ldr r3, [r7, #4]
ldr r3, [r3]
mov r0, r3
adds r7, r7, #12
mov sp, r7
@ sp needed
pop {r7}
bx lr
.size f2, .-f2
.ident "GCC: (GNU Arm Embedded Toolchain 10.3-2021.07) 10.3.1 20210621 (release)"
```
You can see compiler always load 4 bytes regardless whether packed tag.
--
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] masayuki2009 commented on pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#issuecomment-1005396270
@anchao @xiaoxiang781216
Hmm, the gs2200m Wi-Fi daemon asserted with the renew command
Do I need to modify gs2200m_daemon.c to work with this PR?
```
nsh> gs2200m raspi3-g wifi-test-24g &
gs2200m [5:50]
nsh> renew wlan0
[ 3.501729] up_assert: Assertion failed at file:gs2200m_main.c line: 392 task: gs2200m
```
If I revert this PR, it works again.
```
nsh> ifconfig
wlan0 Link encap:Ethernet HWaddr 00:00:00:00:00:00 at UP
inet addr:0.0.0.0 DRaddr:0.0.0.0 Mask:0.0.0.0
nsh> gs2200m raspi3-g wifi-test-24g &
gs2200m [5:50]
nsh> renew wlan0
nsh> ifconfig
wlan0 Link encap:Ethernet HWaddr 3c:95:09:00:89:96 at UP
inet addr:192.168.10.22 DRaddr:192.168.10.1 Mask:255.255.255.0
```
--
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 #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#issuecomment-1005397792
Do you have this patch on apps side: https://github.com/apache/incubator-nuttx-apps/pull/956
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778182912
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
@xiaoxiang781216 that is exactly what I have started from. The usecase is either native alignment or `packed`. In Bothell cases any `reserved` does not make any sense because in case of native alignment all "reserved" will be added by the compiler and in case of `packed` to padding will be done.
Maybe I'm missing something, but please point me to why do we need to add any `reserved` into the structures?
--
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 #5154: net/usrsock: replace xid type to uint64_t to avoid limitation of connections
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5154:
URL: https://github.com/apache/incubator-nuttx/pull/5154#discussion_r778201656
##########
File path: include/nuttx/net/usrsock.h
##########
@@ -227,9 +228,9 @@ begin_packed_struct struct usrsock_message_req_ack_s
{
struct usrsock_message_common_s head;
- uint8_t xid;
- uint8_t reserved;
- int32_t result;
+ int16_t reserved;
Review comment:
> @xiaoxiang781216 that is exactly what I have started from. The usecase is either native alignment
We can't leverage the compiler do the native alignment as discuss before since the different compiler may align to the different place.
> or `packed`.
packed can ensure all compiler place the field in the same location which is important for IPC.
> In Bothell cases any `reserved` does not make any sense because in case of native alignment all "reserved" will be added by the compiler and in case of `packed` to padding will be done.
packed will remove any pad from the struct, so if we don't add some reserved field here, many fields will become unalignment. This will impact the performance and code size:
1. If the hardware doesn't support unaligned access, compiler will split the bus transaction into small and aligned, which increase code size and decrease performance both.
2. If the hardware does support unaligned access, compiler will issue the access directly, and hardware will split it into small and aligned if the access isn't aligned. In this case, only the performance get impact.
> Maybe I'm missing something, but please point me to why do we need to add any `reserved` into the structures?
--
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