Improvements in TCP connections allocation. #7525

fjpanag · 2022-11-04T10:50:23Z

Summary

This is an attempt to fix the issues reported in #6960:

Only a single connection is allocated, the one actually needed.
In tcp_free() the connection is actually deallocated, instead of leaving it existing forever in the list.

This is only the first step, possibly with more to come (list of pre-allocated connections, maximum connections limit etc).

For the moment, this PR handles only TCP, till we determine all the details.
Then the fix will be copied to all other connections that use similar logic.

Impact

Improved memory utilization in TCP.

Testing

Only minimal testing for the moment.
More testing is expected on actual hardware soon, I will report back.

fjpanag · 2022-11-04T10:53:56Z

Previously, when CONFIG_NET_SOLINGER was enabled, there was some functionality to drop an old connection and reuse it.

I believe this has no meaning now, as we cannot really run out of connections (as easily before at least), and since there is no free connections lists, the connection is nowhere to be returned.

Also, as there will never be any free connections, this will trigger this code every time, causing old connections to be dropped constantly, instead of only when actually needed.

anchao · 2022-11-07T02:20:27Z

@fjpanag

Only a single connection is allocated, the one actually needed.

On some platforms this behavior can cause the memory fragmentation, and also if you think the memory fragmentation is acceptable, please configure the numb er of connections to 1, which will consistent with your expectations.

In tcp_free() the connection is actually deallocated, instead of leaving it existing forever in the list.

as mentioned in #6960, please release the connections instance if the number of connections is configured to 1, otherwise keep the current behavior, in some cases memory fragmentation is unacceptable.

fjpanag · 2022-11-08T11:18:53Z

@fjpanag

Only a single connection is allocated, the one actually needed.

On some platforms this behavior can cause the memory fragmentation, and also if you think the memory fragmentation is acceptable, please configure the numb er of connections to 1, which will consistent with your expectations.

In tcp_free() the connection is actually deallocated, instead of leaving it existing forever in the list.

as mentioned in #6960, please release the connections instance if the number of connections is configured to 1, otherwise keep the current behavior, in some cases memory fragmentation is unacceptable.

As I mentioned, my intention is to add a pool of preallocated connections, to avoid heap fragmentation. So new connections will be allocated only when the pool is exhausted.

I will include this change in this PR.

fjpanag · 2022-11-25T12:15:28Z

This is my second take on improving dynamically allocated connections.

Now a pool of preallocated connections is created during start.
These connections are preferred, reducing the danger of heap fragmentation.

If nevertheless the connections are exhausted, a new one will be allocated, allowing normal operation of the system even during temporal high load.

When these "extra" connections are no longer needed, they are properly deallocated.

This is not fully tested yet, I am working on this and I will comment back. In the meantime please review the proposed solution.

I am planning on also adding a maximum connections limit. This way a misbehaving system, or possibly a network attack, will not exhaust the heap (which will possibly have disastrous consequences).

fjpanag · 2022-11-25T12:59:04Z

Also added the maximum connections limit.

fjpanag · 2022-11-26T14:04:15Z

Everything seems fine now.

Tested on a custom server with a few dozens of connections. It seems stable, and no memory leaks were observed.

net/tcp/tcp_conn.c

include/nuttx/net/netconfig.h

net/tcp/tcp_conn.c

fjpanag · 2022-12-03T14:37:08Z

I removed the extra Kconfig and all connections are allocated in a single batch.

Also I changed init a bit, so it is simpler now, and there is no code duplication.

net/tcp/tcp_conn.c

fjpanag · 2022-12-04T14:49:12Z

Here is another take on this issue.
Not fully tested yet, may change a few things here and there.

@xiaoxiang781216

NET_TCP_PREALLOC_CONNS to indicate the preallocated conn from .bss/data or heap:
zero disable the prealloation

Done. For the moment BSS is used, I am not sure whether I will change this to use the heap.

NET_TCP_ALLOC_CONNS to indicate the dynamic conn from heap:
0 mean disable dynamical allocation at all
1 mean allocate/free from heap every time
2... mean allocate batch from heap, but not return to heap

Done. This way it satisfies everyone.

Add NET_TCP_MAX_CONNS to limit the concurrent opened connection, actually I think it's better to impose the limitation from psocket level instead individual protocol(e.g. TCP/UDP/ICMP).

I left this in TCP level. I believe it is much better to be able to tune each protocol independently. Especially because each protocol has a different connection structure size, and different usage. For example the heap may have enough space for 50 UDP connections, but not for 50 TCP connections.

Finally, remove NET_ALLOC_CONNS once all conn convert to this style since we can achieve the same functionality by setting NET_xxx_ALLOC_CONNS to zero or non zero.

Yes will do, in a future PR though.

fjpanag · 2022-12-10T16:35:21Z

I just had the time to complete my tests, and this seems to behave as expected.

@xiaoxiang781216 What do you think, can we merge this?

Does anyone understand what is the issue with the CI?
Shall we just restart it?

xiaoxiang781216 · 2022-12-10T17:02:51Z

I just had the time to complete my tests, and this seems to behave as expected.

@xiaoxiang781216 What do you think, can we merge this?

I will take a look in tomorrow

Does anyone understand what is the issue with the CI? Shall we just restart it?

I am trying fix ci break here: #7842

net/tcp/tcp_conn.c

net/tcp/Kconfig

net/tcp/tcp_conn.c

include/nuttx/net/netconfig.h

net/tcp/Kconfig

net/tcp/tcp.h

net/tcp/Kconfig

xiaoxiang781216 · 2022-12-11T15:18:24Z

@fjpanag please rebase to the last master, ci break has been fixed.

xiaoxiang781216 · 2022-12-11T15:19:14Z

@fjpanag it's better to apply the similar change to all socket conn, not only tcp.

net/tcp/Kconfig

include/nuttx/net/netconfig.h

net/udp/Kconfig

net/icmp/Kconfig

net/icmpv6/icmpv6_conn.c

net/icmp/icmp_conn.c

net/usrsock/usrsock_conn.c

net/udp/udp_conn.c

net/tcp/tcp_conn.c

xiaoxiang781216 · 2023-02-18T13:33:38Z

not all comments get addressed?

fjpanag · 2023-02-18T14:11:07Z

not all comments get addressed?

Working on it... I will ping you.

xiaoxiang781216 · 2023-02-19T15:19:30Z

@fjpanag CONFIG_NET_ALLOC_SOCKETS need be removed from sim/dynconns/defconfig:

====================================================================================
Configuration/Tool: sim/dynconns
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
  Normalize sim/dynconns
49d48
< CONFIG_NET_ALLOC_SOCKETS=1
Saving the new configuration file

fjpanag · 2023-02-21T13:04:49Z

Users defconfigs are needed to be updated after this PR.
Maybe this must be marked to be included in the next release notes?

xiaoxiang781216 · 2023-02-21T15:33:35Z

Marked.

pkarashchenko · 2023-02-21T21:52:04Z

net/ieee802154/ieee802154_conn.c

+  else
+#endif
+    {
+      dq_addlast(&conn->sconn.node, &g_free_ieee802154_connections);


Maybe we need to move memset(conn, 0, sizeof(*conn)); here and do not spend extra cycles to memset memory that is going to be free-ed?

Yes this is a good idea, I will provide a new PR for this.

@pkarashchenko see #8669.

pkarashchenko · 2023-02-21T21:52:26Z

net/netlink/netlink_conn.c


  /* Reset structure */

  memset(conn, 0, sizeof(*conn));


pkarashchenko · 2023-02-21T21:52:43Z

net/pkt/pkt_conn.c


  /* Make sure that the connection is marked as uninitialized */

  memset(conn, 0, sizeof(*conn));


pkarashchenko · 2023-02-21T21:54:08Z

net/tcp/tcp_conn.c

+  else
+#endif
+    {
+      dq_addlast(&conn->sconn.node, &g_free_tcp_connections);


Do we need memset(conn, 0, sizeof(*conn)); before adding to the queue

I think not. It seems that tcp_free() sets the contents of conn to specific values, for example see this.

If someone knows for sure how this works, I can add a memset() and then set the flags, or whatever else is needed.

pkarashchenko · 2023-02-21T21:54:20Z

net/udp/udp_conn.c


  /* Clear the connection structure */

  memset(conn, 0, sizeof(*conn));


pkarashchenko · 2023-02-21T21:56:53Z

net/usrsock/usrsock_conn.c

  /* Reset structure */

  nxsem_destroy(&conn->resp.sem);
  memset(conn, 0, sizeof(*conn));


xiaoxiang781216 requested a review from anchao November 4, 2022 14:13

fjpanag marked this pull request as draft November 8, 2022 11:19

fjpanag force-pushed the conn_alloc branch 2 times, most recently from dbd4d5d to 6e8b289 Compare November 25, 2022 12:10

fjpanag force-pushed the conn_alloc branch from 6e8b289 to ee6c9fd Compare November 25, 2022 12:40

fjpanag force-pushed the conn_alloc branch 2 times, most recently from aaec433 to 46cb9c4 Compare November 26, 2022 13:44

fjpanag marked this pull request as ready for review November 26, 2022 14:04

xiaoxiang781216 reviewed Nov 26, 2022

View reviewed changes

net/tcp/tcp_conn.c Outdated Show resolved Hide resolved

include/nuttx/net/netconfig.h Outdated Show resolved Hide resolved

net/tcp/tcp_conn.c Outdated Show resolved Hide resolved

fjpanag force-pushed the conn_alloc branch from 46cb9c4 to 430ac0d Compare November 26, 2022 15:38

xiaoxiang781216 linked an issue Nov 27, 2022 that may be closed by this pull request

Behaviour of CONFIG_NET_ALLOC_CONNS #6960

Closed

fjpanag force-pushed the conn_alloc branch from 430ac0d to 1b19720 Compare December 3, 2022 14:33

fjpanag force-pushed the conn_alloc branch from 1b19720 to bce6ee8 Compare December 3, 2022 14:40

xiaoxiang781216 reviewed Dec 3, 2022

View reviewed changes

net/tcp/tcp_conn.c Show resolved Hide resolved

fjpanag force-pushed the conn_alloc branch 2 times, most recently from 0be2fb8 to 7398953 Compare December 4, 2022 14:44

xiaoxiang781216 reviewed Dec 11, 2022

View reviewed changes

net/tcp/tcp_conn.c Show resolved Hide resolved

net/tcp/Kconfig Show resolved Hide resolved

net/tcp/tcp_conn.c Outdated Show resolved Hide resolved

net/tcp/tcp_conn.c Outdated Show resolved Hide resolved

include/nuttx/net/netconfig.h Outdated Show resolved Hide resolved

anchao reviewed Dec 11, 2022

View reviewed changes

net/tcp/Kconfig Show resolved Hide resolved

net/tcp/tcp.h Outdated Show resolved Hide resolved

net/tcp/Kconfig Show resolved Hide resolved

fjpanag mentioned this pull request Feb 15, 2023

Add support for NuttX RTOS. nim-lang/Nim#21372

Merged

xiaoxiang781216 reviewed Feb 18, 2023

View reviewed changes

fjpanag force-pushed the conn_alloc branch from 46c94ec to cb48fb1 Compare February 18, 2023 13:25

fjpanag force-pushed the conn_alloc branch from cb48fb1 to 94d66ec Compare February 18, 2023 14:04

fjpanag force-pushed the conn_alloc branch from 94d66ec to 7b38336 Compare February 19, 2023 13:01

Fotis Panagiotopoulos added 11 commits February 19, 2023 15:23

Improvements in TCP connections allocation.

708751b

Improvements in UDP connections allocation.

b79240b

Improvements in usrsock connections allocation.

5ad4145

Improvements in ICMP connections allocation.

49c8f14

Improvements in ICMPv6 connections allocation.

2a4c61d

Improvements in Bluetooth connections allocation.

d37e9f7

Improvements in IEEE 802.15.4 connections allocation.

8ab6bdf

Improvements in packet connections allocation.

784302f

Improvements in Netlink connections allocation.

eb0a418

Improvements in sockets allocation.

cd6ca95

Improvements in CAN connections allocation.

1bf3f46

fjpanag force-pushed the conn_alloc branch from 7b38336 to 05fda96 Compare February 19, 2023 13:24

xiaoxiang781216 approved these changes Feb 19, 2023

View reviewed changes

Removed obsolete option NET_ALLOC_CONNS.

13139c8

fjpanag force-pushed the conn_alloc branch from 05fda96 to 13139c8 Compare February 19, 2023 15:39

xiaoxiang781216 merged commit b6eb3c8 into apache:master Feb 20, 2023

fjpanag deleted the conn_alloc branch February 20, 2023 11:05

xiaoxiang781216 added the breaking change This change requires a mitigation entry in the release notes. label Feb 21, 2023

pkarashchenko reviewed Feb 21, 2023

View reviewed changes

fjpanag mentioned this pull request Feb 28, 2023

Performance optimizations in connection allocations. #8669

Merged


		/* Make sure that the connection is marked as uninitialized */

		memset(conn, 0, sizeof(*conn));


		/* Clear the connection structure */

		memset(conn, 0, sizeof(*conn));

Improvements in TCP connections allocation. #7525

Improvements in TCP connections allocation. #7525

Uh oh!

Conversation

fjpanag commented Nov 4, 2022

Summary

Impact

Testing

Uh oh!

fjpanag commented Nov 4, 2022

Uh oh!

anchao commented Nov 7, 2022

Uh oh!

fjpanag commented Nov 8, 2022

Uh oh!

fjpanag commented Nov 25, 2022

Uh oh!

fjpanag commented Nov 25, 2022

Uh oh!

fjpanag commented Nov 26, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fjpanag commented Dec 3, 2022

Uh oh!

Uh oh!

fjpanag commented Dec 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjpanag commented Dec 10, 2022

Uh oh!

xiaoxiang781216 commented Dec 10, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiaoxiang781216 commented Dec 11, 2022

Uh oh!

xiaoxiang781216 commented Dec 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xiaoxiang781216 commented Feb 18, 2023

Uh oh!

fjpanag commented Feb 18, 2023

Uh oh!

xiaoxiang781216 commented Feb 19, 2023

Uh oh!

fjpanag commented Feb 21, 2023

Uh oh!

xiaoxiang781216 commented Feb 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjpanag commented Dec 4, 2022 •

edited

Loading