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 2021/12/29 13:32:08 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #5110: net/devif: allocate devif callback dynamically
anchao opened a new pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110
## Summary
devif: allocate devif callback dynamically
net/devif: rename NET_NACTIVESOCKETS to NET_NACTIVESOCKETS_PER_BLOCK
## Impact
netdev interface callback allocate
## Testing
iperf client/server 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 #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r777106891
##########
File path: net/devif/devif_callback.c
##########
@@ -258,11 +235,31 @@ FAR struct devif_callback_s *
FAR struct devif_callback_s **list_tail)
{
FAR struct devif_callback_s *ret;
+ int i;
/* Check the head of the free list */
net_lock();
- ret = g_cbfreelist;
+ ret = g_cbfreelist;
+
+ /* Allocate the callback entry from heap */
Review comment:
Let's apply the similar change as #5104
--
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] Ouss4 commented on pull request #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#issuecomment-1002737168
> > Since now we are allocating the callback structure dynamically, shouldn't `devif_callback_free` also free this buffer?
>
> In order to avoid frequent access to the allocator, the current implement just re-queue the callback entry back to the freelist，we will reuse this area to avoid memory fragmentation
Looking at the implementation of `devif_callback_alloc` right now, it seems to be allocating the whole pool for each call. How is this working?
--
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 #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r776363168
##########
File path: net/devif/devif_callback.c
##########
@@ -262,7 +276,7 @@ FAR struct devif_callback_s *
/* Check the head of the free list */
net_lock();
- ret = g_cbfreelist;
+ ret = devif_callback_alloc_internal();
Review comment:
let's merge devif_callback_alloc_internal into devif_callback_alloc?
--
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 #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r777113066
##########
File path: net/devif/devif_callback.c
##########
@@ -258,11 +235,31 @@ FAR struct devif_callback_s *
FAR struct devif_callback_s **list_tail)
{
FAR struct devif_callback_s *ret;
+ int i;
/* Check the head of the free list */
net_lock();
- ret = g_cbfreelist;
+ ret = g_cbfreelist;
+
+ /* Allocate the callback entry from heap */
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] xiaoxiang781216 merged pull request #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110
--
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 #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r776363282
##########
File path: net/devif/devif_callback.c
##########
@@ -35,14 +35,15 @@
#include <nuttx/net/net.h>
#include <nuttx/net/netdev.h>
+#include <nuttx/kmalloc.h>
Review comment:
move before line 34
--
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 #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r776444492
##########
File path: net/socket/Kconfig
##########
@@ -5,13 +5,12 @@
menu "Socket Support"
-config NET_NACTIVESOCKETS
- int "Max socket operations"
- default 16 if !DEFAULT_SMALL
- default 4 if DEFAULT_SMALL
+config NET_NACTIVESOCKETS_PER_BLOCK
+ int "Max socket operations per block"
+ default 4
---help---
- Maximum number of concurrent socket operations (recv, send,
- connection monitoring, etc.). Default: 16
+ Maximum number of concurrent socket operations per block
Review comment:
Done
##########
File path: net/socket/Kconfig
##########
@@ -5,13 +5,12 @@
menu "Socket Support"
-config NET_NACTIVESOCKETS
- int "Max socket operations"
- default 16 if !DEFAULT_SMALL
- default 4 if DEFAULT_SMALL
+config NET_NACTIVESOCKETS_PER_BLOCK
+ int "Max socket operations per block"
+ default 4
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] anchao commented on pull request #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#issuecomment-1002724163
> Since now we are allocating the callback structure dynamically, shouldn't `devif_callback_free` also free this buffer?
In order to avoid frequent access to the allocator, the current implement just re-queue the callback entry back to the freelist，we will reuse this area to avoid memory fragmentation
--
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 #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r776444455
##########
File path: net/devif/devif_callback.c
##########
@@ -35,14 +35,15 @@
#include <nuttx/net/net.h>
#include <nuttx/net/netdev.h>
+#include <nuttx/kmalloc.h>
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] anchao commented on pull request #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
anchao commented on pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#issuecomment-1002741733
> Looking at the implementation of `devif_callback_alloc` right now, it seems to be allocating the whole pool for each call. How is this working?
Yes, each request size is pool based(NET_NACTIVESOCKETS_PER_ALLOC),
if the freelist is empty, we will allocate for a sequence of callback entries and queue them into the freelist, this is to reduce the memory fragmentation and allocator access cycles.
--
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 #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r776769876
##########
File path: net/devif/devif_callback.c
##########
@@ -258,11 +235,31 @@ FAR struct devif_callback_s *
FAR struct devif_callback_s **list_tail)
{
FAR struct devif_callback_s *ret;
+ int i;
/* Check the head of the free list */
net_lock();
- ret = g_cbfreelist;
+ ret = g_cbfreelist;
+
+ /* Allocate the callback entry from heap */
Review comment:
Of course, let us wait for the conclusion of [#5104](https://github.com/apache/incubator-nuttx/pull/5104), I think this pull request needs to be further enhanced
--
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] Ouss4 commented on pull request #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#issuecomment-1002996214
> > Looking at the implementation of `devif_callback_alloc` right now, it seems to be allocating the whole pool for each call. How is this working?
>
> Yes, each request size is pool based(NET_NACTIVESOCKETS_PER_ALLOC), if the freelist is empty, we will allocate for a sequence of callback entries and queue them into the freelist, this is to reduce the memory fragmentation and allocator access cycles.
I see. Thanks for the explanation. I thought the allocation was based on an element not a pool and I was going to ask about the impact of this long lived allocation on fragmentation. But it looks like you've already considered it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao edited a comment on pull request #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
anchao edited a comment on pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#issuecomment-1002724163
> Since now we are allocating the callback structure dynamically, shouldn't `devif_callback_free` also free this buffer?
In order to avoid frequent access to the allocator, the current implement just re-queue the callback entry back to the freelist，we will reuse this area to avoid memory fragmentation
--
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 #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r776363965
##########
File path: net/socket/Kconfig
##########
@@ -5,13 +5,12 @@
menu "Socket Support"
-config NET_NACTIVESOCKETS
- int "Max socket operations"
- default 16 if !DEFAULT_SMALL
- default 4 if DEFAULT_SMALL
+config NET_NACTIVESOCKETS_PER_BLOCK
+ int "Max socket operations per block"
+ default 4
---help---
- Maximum number of concurrent socket operations (recv, send,
- connection monitoring, etc.). Default: 16
+ Maximum number of concurrent socket operations per block
Review comment:
The number of concurrent socket operations per allocation
--
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 #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r776364093
##########
File path: net/socket/Kconfig
##########
@@ -5,13 +5,12 @@
menu "Socket Support"
-config NET_NACTIVESOCKETS
- int "Max socket operations"
- default 16 if !DEFAULT_SMALL
- default 4 if DEFAULT_SMALL
+config NET_NACTIVESOCKETS_PER_BLOCK
+ int "Max socket operations per block"
+ default 4
Review comment:
default to 1 if DEFAULT_SMALL
--
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 #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r776444423
##########
File path: net/devif/devif_callback.c
##########
@@ -262,7 +276,7 @@ FAR struct devif_callback_s *
/* Check the head of the free list */
net_lock();
- ret = g_cbfreelist;
+ ret = devif_callback_alloc_internal();
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] Ouss4 commented on a change in pull request #5110: net/devif: allocate devif callback dynamically
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #5110:
URL: https://github.com/apache/incubator-nuttx/pull/5110#discussion_r776694408
##########
File path: net/devif/devif_callback.c
##########
@@ -258,11 +235,31 @@ FAR struct devif_callback_s *
FAR struct devif_callback_s **list_tail)
{
FAR struct devif_callback_s *ret;
+ int i;
/* Check the head of the free list */
net_lock();
- ret = g_cbfreelist;
+ ret = g_cbfreelist;
+
+ /* Allocate the callback entry from heap */
Review comment:
@anchao can you please add part of https://github.com/apache/incubator-nuttx/pull/5110#issuecomment-1002741733 as an explanation here?
--
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