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/08/04 07:04:35 UTC
[GitHub] [incubator-nuttx] luojun1234 opened a new pull request, #6780: iob_s add io_private filed, user can use it to keep context
luojun1234 opened a new pull request, #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780
Signed-off-by: luojun1 <lu...@xiaomi.com>
Change-Id: Ia065b2ba3910bad01b1548a9bac75e003c3ee823
## Summary
add io_private field to iob_s, user can use it to keep context
## Impact
none
## Testing
none
--
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 #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1205993756
> > How do you think if we add a KCONFIG option to turn it on or off? like following:
>
> Although this is a improvement, if you bring in your TCP/IP reassemble changes in then you probably select this setting by default. Which on a system that uses for instance TCP/IP, UDP/IP and SocketCAN, then the UDP/IP and SocketCAN stack still get the memory increase by allocating the iob_private and not using it.
>
UDP still need reassemble feature which is at IP level, but I am not sure where SocketCAN need IP reassemble. But the new option could address the major concern about the size expansion.
--
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] luojun1234 commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
luojun1234 commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1204947703
> > However the impact of this would increase memory usage overall for all IOB users. For example an imxrt config that has following config `CONFIG_IOB_NBUFFERS=128` Which gives you a gain of 512 bytes in memory usage with no real benefits.
> > Another example would be the nucleo-f446re:cansock which has `CONFIG_IOB_NBUFFERS=1024` Which would yield 4KB increase in memory usage.
>
> It depends on how we choose between performance and memory usage. The above-mentioned waste may be acceptable, the memory should not be too small now, otherwise, it will affect the throughput 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] PetervdPerk-NXP commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
PetervdPerk-NXP commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1204889482
What's the particular use case for this?
I only see the addition of this field but there's no example.
Maybe the particular data can just be appended to the io_data field for instance.
--
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] PetervdPerk-NXP commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
PetervdPerk-NXP commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1204913589
However the impact of this would increase memory usage overall for all IOB users.
For example an imxrt config that has following config `CONFIG_IOB_NBUFFERS=128`
Which gives you a gain of 512 bytes in memory usage with no real benefits.
--
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] luojun1234 closed pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
luojun1234 closed pull request #6780: iob_s add io_private filed, user can use it to keep context
URL: https://github.com/apache/incubator-nuttx/pull/6780
--
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 diff in pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#discussion_r939524752
##########
include/nuttx/mm/iob.h:
##########
@@ -108,7 +120,7 @@ struct iob_s
#endif
unsigned int io_pktlen; /* Total length of the packet */
- uint8_t io_data[CONFIG_IOB_BUFSIZE];
+ FAR uint8_t *io_data;
Review Comment:
let's keep it as before if CONFIG_IOB_ALIGNMENT equal 1
--
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] luojun1234 commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
luojun1234 commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1204976649
> Still we should consider that NuttX runs on small microcontrollers with SRAM varying between 8KB up to 1MB. Hence a small increases can still have big impact on smaller platforms.
>
> Regarding the problem you've got for IP reassemble maybe there's a different way to solve your problem without affecting other stacks such as UDP, SocketCAN and RAW packet.
>
> My first guess would just introduce a header to the data you copy into io_data. It's something similar I've did for timestamping for SocketCAN see below
>
> https://github.com/apache/incubator-nuttx/blob/0083a2e4f29ac8574eeda895803f2a7447d89680/net/can/can_callback.c#L123-L136
Thanks for you information and advice.
We are now modifying the TCPIP stack so that packets submitted by the NIC can be sent directly to L4(TCP, UDP etc.), for performance reasons it is not recommended to copy and move ip data during this process.
--
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] luojun1234 commented on a diff in pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
luojun1234 commented on code in PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#discussion_r943351964
##########
include/nuttx/mm/iob.h:
##########
@@ -108,7 +120,7 @@ struct iob_s
#endif
unsigned int io_pktlen; /* Total length of the packet */
- uint8_t io_data[CONFIG_IOB_BUFSIZE];
+ FAR uint8_t *io_data;
Review Comment:
We have a better solution, discard this PR. Please refer to https://github.com/apache/incubator-nuttx/pull/6834
##########
mm/iob/Kconfig:
##########
@@ -32,6 +32,18 @@ config IOB_BUFSIZE
chain. This setting determines the data payload each preallocated
I/O buffer.
+config IOB_ALIGNMENT
+ int "Alignment size of each I/O buffer"
+ default 1
Review Comment:
We have a better solution, discard this PR. Please refer to https://github.com/apache/incubator-nuttx/pull/6834
--
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 pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1206111060
> > > How do you think if we add a KCONFIG option to turn it on or off? like following:
> >
> >
> > Although this is a improvement, if you bring in your TCP/IP reassemble changes in then you probably select this setting by default. Which on a system that uses for instance TCP/IP, UDP/IP and SocketCAN, then the UDP/IP and SocketCAN stack still get the memory increase by allocating the iob_private and not using it.
>
> UDP still need reassemble feature which is at IP level, but I am not sure where SocketCAN need IP reassemble. But the new option could address the major concern about the size expansion.
SocketCAN does not need IP reassemble.
--
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 diff in pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#discussion_r938505225
##########
mm/iob/Kconfig:
##########
@@ -32,6 +32,18 @@ config IOB_BUFSIZE
chain. This setting determines the data payload each preallocated
I/O buffer.
+config IOB_ALIGNMENT
+ int "Alignment size of each I/O buffer"
+ default 1
Review Comment:
Please check if 1 or 2 should be default. The low level stack assumes 2 bytes alignment, but I'm not sure what goes into IOB
--
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] luojun1234 commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
luojun1234 commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1204937054
> However the impact of this would increase memory usage overall for all IOB users. For example an imxrt config that has following config `CONFIG_IOB_NBUFFERS=128` Which gives you a gain of 512 bytes in memory usage with no real benefits.
>
> Another example would be the nucleo-f446re:cansock which has `CONFIG_IOB_NBUFFERS=1024` Which would yield 4KB increase in memory usage.
It depends on how we choose between performance and memory usage. The above-mentioned waste may be acceptable, the memory should not be too small now, otherwise, it will affect the throughput 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] pkarashchenko commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1205203626
The main issue is that all the network and not only the network use a single IOB mechanism. That has more drawbacks that advantages IMO, but still it is what we have now. For example LWIP pbuf allows you to chain the buffers and add any meta information into a chain if needed. Also each pbuf can be allocated with customised size. The NuttX IOB interface is simpler from this point of view, but quite a compact fixed sized interface.
When I observe the current change I see that we are adding 8 bytes (for 32-bit systems) per IOB with no gain (4 for private data and 4 for pointer to data) in case if IP reassembly is not needed. That is quite a big punch to a systems with limited amount of memory.
I agree that showing the draft version of IP reassembly and getting feedback on design points is the best way to move forward.
--
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 #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1205995200
> It might not be obvious that the design patterns used in NuttX were intentionally very memory/resource conscious from the start. These patterns are extremely important to preserve!
>
> The optimization of the memory footprint was taken into account using CONFIG_IOB_BUFSIZE to save a **single** byte per struct. https://github.com/luojun1234/incubator-nuttx/blob/d6affff1241004452267921f5291cfcacc39653d/include/nuttx/mm/iob.h#L104-L110
>
But, there is no real saving, since the compiler move io_pktlen to the next 4byes alignment. We need rearrange the sequence to save the 2bytes.
> So it would not make sense to add 4 bytes per struct that is an optional use field without being able to to disable it.
>
Yes, agree.
--
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] PetervdPerk-NXP commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
PetervdPerk-NXP commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1204960766
Still we should consider that NuttX runs on small microcontrollers with SRAM varying between 8KB up to 1MB.
Hence a small increases can still have big impact on smaller platforms.
Regarding the problem you've got for IP reassemble maybe there's a different way to solve your problem without affecting other stacks such as UDP, SocketCAN and RAW packet.
My first guess would just introduce a header to the data you copy into io_data.
It's something similar I've did for timestamping for SocketCAN see below
https://github.com/apache/incubator-nuttx/blob/0083a2e4f29ac8574eeda895803f2a7447d89680/net/can/can_callback.c#L123-L136
--
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] luojun1234 commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
luojun1234 commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1205073094
How do you think if we add a KCONFIG option to turn it on or off? like following:
#if defined(CONFIG_IOB_PRIVATE)
FAR uintptr_t iob_private;
#endif
This configuration will be opened if required by the project.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
davids5 commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1204990858
It might not be obvious that the design patterns used in NuttX were intentionally very memory/resource conscious from the start. These patterns are extremely important to preserve!
The optimization of the memory footprint was taken into account using CONFIG_IOB_BUFSIZE to save a **single** byte per struct. https://github.com/luojun1234/incubator-nuttx/blob/d6affff1241004452267921f5291cfcacc39653d/include/nuttx/mm/iob.h#L104-L110
So it would not make sense to add 4 bytes per struct that is an optional use field without being able to to disable it.
Inheritance or the default 0 length array would be ways to solve this problem as well.
As a project we should be all for flexibility and serving all users use cases, but not at the expense of other users.
--
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] PetervdPerk-NXP commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
PetervdPerk-NXP commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1205098524
> How do you think if we add a KCONFIG option to turn it on or off? like following:
Although this is a improvement, if you bring in your TCP/IP reassemble changes in then you probably select this setting by default.
Which on a system that uses for instance TCP/IP, UDP/IP and SocketCAN, then the UDP/IP and SocketCAN stack still get the memory increase by allocating the iob_private and not using it.
Would it be possible the showcase the draft of the TCP/IP reassemble change and see if we can make it such it only affects the net/tcp?
--
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 #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1211877214
let's close this 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.
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 closed pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 closed pull request #6780: iob_s add io_private filed, user can use it to keep context
URL: https://github.com/apache/incubator-nuttx/pull/6780
--
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 diff in pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#discussion_r937444322
##########
include/nuttx/mm/iob.h:
##########
@@ -97,6 +97,8 @@ struct iob_s
FAR struct iob_s *io_flink;
+ FAR void *io_private; /* Interpreted by user */
Review Comment:
Should this be optional? Also maybe change type to uintptr_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] luojun1234 commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
luojun1234 commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1204910447
It is optional, some modules may use it to store the pointer of running context.
For example, in the IP reassemble function we are developing, multiple fragments are organized in an IOB chain. When all fragments belonging to one IP frame are received, all fragments need to be sorted according to the IP offset value in each fragment IP header. The offset value can be stored in this memory pointed by this io_private pointer. This way we can avoid querying the offset value from the IP header multiple times.
--
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] luojun1234 commented on pull request #6780: iob_s add io_private filed, user can use it to keep context
Posted by GitBox <gi...@apache.org>.
luojun1234 commented on PR #6780:
URL: https://github.com/apache/incubator-nuttx/pull/6780#issuecomment-1205989646
OK, I need a few days to prepare the draft version of IP reassembly before uploading
--
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