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/11 10:22:33 UTC
[GitHub] [incubator-nuttx] luojun1234 opened a new pull request, #6834: modify iob to support header padding and alignment features
luojun1234 opened a new pull request, #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834
Signed-off-by: luojun1 <lu...@xiaomi.com>
## Summary
modify iob to support buffer aligment and head padding features
## Impact
1.if config CONFIG_IOB_ALIGNMENT to 1 and CONFIG_IOB_HEADSIZE to 0, the iob_s initialization and memory usage are the same as before，no extra bytes are wasted.
2.In order to improve the performance on some platforms, the zero-copy mechanism can be used in the data transmission path, and the IP packets sent by TCPIP stack are directly sent to the HW for transmission, in this case, one feature is that cache line alignment may be required at the beginning of the buffer, at the same time we may need to provide a memory space for the HW controller (so that the HW controller can fill some data, such as the sending and receiving of USB RNDIS packets, the USB HW needs to put a 42-byte header before the original Ethernet frame), so we designed the io_head field in iob_s structure, and followed the principle of using io_data as the data buffer, we can configure the io_head field to align to the specified memory boundary. In addition, if the io_head length is also configured as an integer multiple of the cache line, then io_data can also be aligned to the cache line.
## Testing
On the emulator, configure arbitrary io_head size and alignment size, wget works fine, configure the io_head size to 0 and the alignment size to 1, no extra bytes are wasted.
--
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#discussion_r943739740
##########
mm/iob/iob_initialize.c:
##########
@@ -30,14 +30,32 @@
#include "iob.h"
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define ROUNDUP(x, y) (((x) + (y) - 1) / (y) * (y))
+
+/* Fix the I/O Buffer size with specified alignment size */
+
+#define IOB_ALIGN_SIZE ROUNDUP(sizeof(struct iob_s), CONFIG_IOB_ALIGNMENT)
+#define IOB_BUFFER_SIZE (IOB_ALIGN_SIZE * CONFIG_IOB_NBUFFERS + \
+ CONFIG_IOB_ALIGNMENT - 1)
+
/****************************************************************************
* Private Data
****************************************************************************/
-/* This is a pool of pre-allocated I/O buffers */
+/* Following raw buffer will be divided into iob_s instances, the initial
+ * procedure will ensure that the member io_head of each iob_s is aligned
+ * to the CONFIG_IOB_ALIGNMENT memory boundary.
+ */
+
+static uint8_t g_iob_buffer[IOB_BUFFER_SIZE];
Review Comment:
let's add a new string option(e.g. IOB_SECTION), so we can arrange the g_iob_buffer to special data region:
static uint8_t g_iob_buffer[IOB_BUFFER_SIZE] locate_data(CONFIG_IOB_SECTION);
It is important for some hardware which has the special requirement for DMA buffer.
--
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
luojun1234 commented on PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#issuecomment-1212007295
> IOB is shared mechanism used for different stacks. So on a multi-stack system where `CONFIG_IOB_HEADSIZE>0` all stacks are affected, maybe this is even a design limitation of the current IOB design. And could use some refactoring/redesign.
>
> But for the moment isn't there any way to introduce user storage for iob without affecting all the iob's? A rough idea from my side would just be using an union exclusively for the TCP side and not affect the others i.e. And just the pointer when using it in the TCP stack
>
> ```
> union iob_s_tcp_playload{
> uint_ptr user;
> uint8_t io_data[CONFIG_IOB_BUFSIZE-sizeof(uint_ptr)];
> } iob_s_tcp_playload;
>
> struct iob_s incoming_data;
>
> // Fetch iob
>
> iob_s_tcp_playload* tcp_iob = (iob_s_tcp_playload*)(&incoming_data.io_data);
>
> struct custom_struct = (struct custom_struct*)tcp_iob.user;
> memcpy(dst, &tcp_io.iodata[0], sizeof(iob_s_tcp_playload.io_data));
> ```
>
> If you guys could share the TCP changes that need this I can give better input on this dicussion, or maybe include this change with this TCP change.
In order to support high throughput, we have designed a zero-copy solution for the data transmission path in our system. Typically, the MTU in our environment is also 1500 bytes.
1. On one core, the iob_s buffer is set to be used directly by the hardware accelerator. We reserve 32 bytes of space in front of the 1500-length payload buffer for the hardware accelerator to use. To ensure data consistency (between RAM and CPU Cache), the reserved 32-byte space needs to be aligned according to the CPU cache line size. From the perspective of data flow, the iob data sent by the tcpip stack is directly consumed by the HW, and the iob data received from the HW is sent to the TCPIP stack for consumption, and only copied when it needs to be sent to the application.
2. On another core, it may be need to forward some IP frames received from one network driver to the USB RNDIS network driver, which is also a zero-copy implementation. We do not copy the data content during the forwarding process, when the ethernet packet is sent to the USB HW, a 42-byte space aligned with the current CPU cache line size is reserved before the Ethernet frame.
As mentioned above, one core needs cache line-aligned iob buffers of 1500+32 bytes, and the other core needs cache line-aligned iob buffers of 1500+42 bytes. Because iob has only one size configuration, other stacks can only use this size of iob buffer.
--
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6834:
URL: 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] xiaoxiang781216 commented on a diff in pull request #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#discussion_r943739740
##########
mm/iob/iob_initialize.c:
##########
@@ -30,14 +30,32 @@
#include "iob.h"
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define ROUNDUP(x, y) (((x) + (y) - 1) / (y) * (y))
+
+/* Fix the I/O Buffer size with specified alignment size */
+
+#define IOB_ALIGN_SIZE ROUNDUP(sizeof(struct iob_s), CONFIG_IOB_ALIGNMENT)
+#define IOB_BUFFER_SIZE (IOB_ALIGN_SIZE * CONFIG_IOB_NBUFFERS + \
+ CONFIG_IOB_ALIGNMENT - 1)
+
/****************************************************************************
* Private Data
****************************************************************************/
-/* This is a pool of pre-allocated I/O buffers */
+/* Following raw buffer will be divided into iob_s instances, the initial
+ * procedure will ensure that the member io_head of each iob_s is aligned
+ * to the CONFIG_IOB_ALIGNMENT memory boundary.
+ */
+
+static uint8_t g_iob_buffer[IOB_BUFFER_SIZE];
Review Comment:
let's add a new string option(e.g. IOB_SECTION), so we can arrange the g_iob_buffer to a special data region:
```
static uint8_t g_iob_buffer[IOB_BUFFER_SIZE] locate_data(CONFIG_IOB_SECTION);
```
It is important for some hardware which has the special requirement for DMA buffer.
--
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#issuecomment-1212265551
> A Zero-copy ethernet solution would be indeed a great addition, when implementing a enet driver myself I noticed as well that packets are double stored and have a unnecessary copy action,
The header and alignment is part of the offloading solution. The current design keep the old behavior as before, but give the implementation the possibility.
> furthermore hw TCP/IP offloading (HW checksum etc) is also thing NuttX doesn't support that well.
>
HW checksum is already supported, you can simply enable NET_ARCH_CHKSUM:
https://github.com/apache/incubator-nuttx/blob/master/net/utils/Kconfig#L6-L26
> But wouldn't it be better to come up with a proposal to offload DMA directly into a IOB buffer and start a discussion (i.e. NuttX mailing list)
It's better to show the code and
> how we can make this portable in such a way that it works all kinds of ethernet MAC's that have build-in DMA.
The offload change is simple from the concept:
1. TCP/IP stack pass IOB directly to netdev instead copy to d_buf
2. netdev request the hardware send the data in IOB and free IOB to pool after done.
3. The netdev allocate IOB and pass to hardware for receiving
4. Pass IOB to ipv4_input/ipv6_input. TCP/IP stack will release the buffer after the data copy to the user buffer
Of course, to keep compatibility, the new behavior need enabled by some flag.
> I think this requires more changes into how IOB works but also the API of the NuttX Networking to work with these changes.
Could you point out the detail change required for 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 a diff in pull request #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
luojun1234 commented on code in PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#discussion_r944229234
##########
mm/iob/iob_initialize.c:
##########
@@ -30,14 +30,32 @@
#include "iob.h"
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define ROUNDUP(x, y) (((x) + (y) - 1) / (y) * (y))
+
+/* Fix the I/O Buffer size with specified alignment size */
+
+#define IOB_ALIGN_SIZE ROUNDUP(sizeof(struct iob_s), CONFIG_IOB_ALIGNMENT)
+#define IOB_BUFFER_SIZE (IOB_ALIGN_SIZE * CONFIG_IOB_NBUFFERS + \
+ CONFIG_IOB_ALIGNMENT - 1)
+
/****************************************************************************
* Private Data
****************************************************************************/
-/* This is a pool of pre-allocated I/O buffers */
+/* Following raw buffer will be divided into iob_s instances, the initial
+ * procedure will ensure that the member io_head of each iob_s is aligned
+ * to the CONFIG_IOB_ALIGNMENT memory boundary.
+ */
+
+static uint8_t g_iob_buffer[IOB_BUFFER_SIZE];
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] PetervdPerk-NXP commented on pull request #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
PetervdPerk-NXP commented on PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#issuecomment-1212105894
A Zero-copy ethernet solution would be indeed a great addition, when implementing a enet driver myself I noticed as well that packets are double stored and have a unnecessary copy action, furthermore hw TCP/IP offloading (HW checksum etc) is also thing NuttX doesn't support that well.
But wouldn't it be better to come up with a proposal to offload DMA directly into a IOB buffer and start a discussion (i.e. NuttX mailing list) how we can make this portable in such a way that it works all kinds of ethernet MAC's that have build-in DMA. I think this requires more changes into how IOB works but also the API of the NuttX Networking to work with these changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a diff in pull request #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#discussion_r944513744
##########
mm/iob/iob_initialize.c:
##########
@@ -30,14 +30,36 @@
#include "iob.h"
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define ROUNDUP(x, y) (((x) + (y) - 1) / (y) * (y))
+
+/* Fix the I/O Buffer size with specified alignment size */
+
+#define IOB_ALIGN_SIZE ROUNDUP(sizeof(struct iob_s), CONFIG_IOB_ALIGNMENT)
+#define IOB_BUFFER_SIZE (IOB_ALIGN_SIZE * CONFIG_IOB_NBUFFERS + \
+ CONFIG_IOB_ALIGNMENT - 1)
+
/****************************************************************************
* Private Data
****************************************************************************/
-/* This is a pool of pre-allocated I/O buffers */
+/* Following raw buffer will be divided into iob_s instances, the initial
+ * procedure will ensure that the member io_head of each iob_s is aligned
+ * to the CONFIG_IOB_ALIGNMENT memory boundary.
+ */
+
+static uint8_t g_iob_buffer[IOB_BUFFER_SIZE]
+#ifdef CONFIG_IOB_SECTION
Review Comment:
Sure
--
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#issuecomment-1211877080
@pkarashchenko @PetervdPerk @davids5 please review this new patch instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] davids5 commented on a diff in pull request #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
davids5 commented on code in PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#discussion_r944409428
##########
mm/iob/iob_initialize.c:
##########
@@ -30,14 +30,36 @@
#include "iob.h"
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define ROUNDUP(x, y) (((x) + (y) - 1) / (y) * (y))
+
+/* Fix the I/O Buffer size with specified alignment size */
+
+#define IOB_ALIGN_SIZE ROUNDUP(sizeof(struct iob_s), CONFIG_IOB_ALIGNMENT)
+#define IOB_BUFFER_SIZE (IOB_ALIGN_SIZE * CONFIG_IOB_NBUFFERS + \
+ CONFIG_IOB_ALIGNMENT - 1)
+
/****************************************************************************
* Private Data
****************************************************************************/
-/* This is a pool of pre-allocated I/O buffers */
+/* Following raw buffer will be divided into iob_s instances, the initial
+ * procedure will ensure that the member io_head of each iob_s is aligned
+ * to the CONFIG_IOB_ALIGNMENT memory boundary.
+ */
+
+static uint8_t g_iob_buffer[IOB_BUFFER_SIZE]
+#ifdef CONFIG_IOB_SECTION
Review Comment:
Would you object to a #define IOB_LOCATION the is defined in Pre-processor Definitions
```
#ifdef CONFIG_IOB_SECTION
# define IOB_LOCATION locate_data(CONFIG_IOB_SECTION)
#else
# define IOB_LOCATTION
#endif
```
and used here?
```static uint8_t g_iob_buffer[IOB_BUFFER_SIZE] IOB_LOCATION;```
--
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#discussion_r943739740
##########
mm/iob/iob_initialize.c:
##########
@@ -30,14 +30,32 @@
#include "iob.h"
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define ROUNDUP(x, y) (((x) + (y) - 1) / (y) * (y))
+
+/* Fix the I/O Buffer size with specified alignment size */
+
+#define IOB_ALIGN_SIZE ROUNDUP(sizeof(struct iob_s), CONFIG_IOB_ALIGNMENT)
+#define IOB_BUFFER_SIZE (IOB_ALIGN_SIZE * CONFIG_IOB_NBUFFERS + \
+ CONFIG_IOB_ALIGNMENT - 1)
+
/****************************************************************************
* Private Data
****************************************************************************/
-/* This is a pool of pre-allocated I/O buffers */
+/* Following raw buffer will be divided into iob_s instances, the initial
+ * procedure will ensure that the member io_head of each iob_s is aligned
+ * to the CONFIG_IOB_ALIGNMENT memory boundary.
+ */
+
+static uint8_t g_iob_buffer[IOB_BUFFER_SIZE];
Review Comment:
let's add a new string option(e.g. IOB_SECTION), so we can arrange the g_iob_buffer to a special data region:
static uint8_t g_iob_buffer[IOB_BUFFER_SIZE] locate_data(CONFIG_IOB_SECTION);
It is important for some hardware which has the special requirement for DMA buffer.
--
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
luojun1234 commented on code in PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#discussion_r944229234
##########
mm/iob/iob_initialize.c:
##########
@@ -30,14 +30,32 @@
#include "iob.h"
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define ROUNDUP(x, y) (((x) + (y) - 1) / (y) * (y))
+
+/* Fix the I/O Buffer size with specified alignment size */
+
+#define IOB_ALIGN_SIZE ROUNDUP(sizeof(struct iob_s), CONFIG_IOB_ALIGNMENT)
+#define IOB_BUFFER_SIZE (IOB_ALIGN_SIZE * CONFIG_IOB_NBUFFERS + \
+ CONFIG_IOB_ALIGNMENT - 1)
+
/****************************************************************************
* Private Data
****************************************************************************/
-/* This is a pool of pre-allocated I/O buffers */
+/* Following raw buffer will be divided into iob_s instances, the initial
+ * procedure will ensure that the member io_head of each iob_s is aligned
+ * to the CONFIG_IOB_ALIGNMENT memory boundary.
+ */
+
+static uint8_t g_iob_buffer[IOB_BUFFER_SIZE];
Review Comment:
A new patch is updated.
--
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
PetervdPerk-NXP commented on PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#issuecomment-1211874042
IOB is shared mechanism used for different stacks.
So on a multi-stack system where `CONFIG_IOB_HEADSIZE>0` all stacks are affected, maybe this is even a design limitation of the current IOB design. And could use some refactoring/redesign.
But for the moment isn't there any way to introduce user storage for iob without affecting all the iob's?
A rough idea from my side would just be using an union exclusively for the TCP side and not affect the others i.e.
And just the pointer when using it in the TCP stack
```
union iob_s_tcp_playload{
uint_ptr user;
uint8_t io_data[CONFIG_IOB_BUFSIZE-sizeof(uint_ptr)];
} iob_s_tcp_playload;
iob_s_tcp_playload* tcp_iob = (iob_s_tcp_playload*)(&iob_s.io_data);
struct custom_struct = (struct custom_struct*)tcp_iob.user;
memcpy(dst, &tcp_io.iodata[0], sizeof(iob_s_tcp_playload.io_data));
```
If you guys could share the TCP changes that need this I can give better input on this dicussion, or maybe include this change with this TCP 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.
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#issuecomment-1213031311
@PetervdPerk-NXP @davids5 before we apply the complex dynamic allocation of IOB buffer, the approach used in this PR can keep the back compatibility and zero cost, and give the hardware possibility to implement the support of offloading.
I would suggest that we can implement the offload feature with the current IOB design in the first phase, and then extend the dynamic allocation in the next phase if you think it's really 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.
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#discussion_r944511905
##########
mm/iob/iob_initialize.c:
##########
@@ -30,14 +30,36 @@
#include "iob.h"
+/****************************************************************************
+ * Pre-processor Definitions
+ ****************************************************************************/
+
+#define ROUNDUP(x, y) (((x) + (y) - 1) / (y) * (y))
+
+/* Fix the I/O Buffer size with specified alignment size */
+
+#define IOB_ALIGN_SIZE ROUNDUP(sizeof(struct iob_s), CONFIG_IOB_ALIGNMENT)
+#define IOB_BUFFER_SIZE (IOB_ALIGN_SIZE * CONFIG_IOB_NBUFFERS + \
+ CONFIG_IOB_ALIGNMENT - 1)
+
/****************************************************************************
* Private Data
****************************************************************************/
-/* This is a pool of pre-allocated I/O buffers */
+/* Following raw buffer will be divided into iob_s instances, the initial
+ * procedure will ensure that the member io_head of each iob_s is aligned
+ * to the CONFIG_IOB_ALIGNMENT memory boundary.
+ */
+
+static uint8_t g_iob_buffer[IOB_BUFFER_SIZE]
+#ifdef CONFIG_IOB_SECTION
Review Comment:
how about change IOB_LOCATION to IOB_SECTION?
--
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 #6834: modify iob to support header padding and alignment features
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6834:
URL: https://github.com/apache/incubator-nuttx/pull/6834#issuecomment-1212047063
> IOB is shared mechanism used for different stacks. So on a multi-stack system where `CONFIG_IOB_HEADSIZE>0` all stacks are affected, maybe this is even a design limitation of the current IOB design. And could use some refactoring/redesign.
>
The redesign need extend the iob_buffer which was denied in #6780. In the current implementation, the default config keep the size as before, but the netdev implementor could extend the header or the alignment, which is a best compromise we can find without the major re-architecture.
> But for the moment isn't there any way to introduce user storage for iob without affecting all the iob's? A rough idea from my side would just be using an union exclusively for the TCP side and not affect the others i.e. And just the pointer when using it in the TCP stack
>
this approach insert a uint_ptr field but how to allocate the real memory to this pointer? Another mem pool?
> ```
> union iob_s_tcp_playload{
> uint_ptr user;
> uint8_t io_data[CONFIG_IOB_BUFSIZE-sizeof(uint_ptr)];
> } iob_s_tcp_playload;
>
> struct iob_s incoming_data;
>
> // Fetch iob
>
> iob_s_tcp_playload* tcp_iob = (iob_s_tcp_playload*)(&incoming_data.io_data);
>
> struct custom_struct = (struct custom_struct*)tcp_iob.user;
> memcpy(dst, &tcp_io.iodata[0], sizeof(iob_s_tcp_playload.io_data));
> ```
>
You can't do this since IP layer expect io_data start at the same location for all TCP/UDP/ICMP/ICMPv6.
> If you guys could share the TCP changes that need this I can give better input on this dicussion, or maybe include this change with this TCP change.
The goal of this patch is different from https://github.com/apache/incubator-nuttx/pull/6780/commits/d6affff1241004452267921f5291cfcacc39653d
This patch fix the netdev special requirement:
1. The cache alignment of io_data to improve the performance
2. Reserve the space before io_data for MAC layer to add the L2 header.
But the preserved header could be used for TCP/UDP/IP out of band data potentially.
--
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