lwip: provide sock_async support #13427

miri64 · 2020-02-20T17:33:34Z

Contribution description

This provides initial sock_async support for lwIP. ~~Currently only sock_udp and sock_ip are supported, sock_tcp needs some extra work, I don't have the time for right now.~~ (edit: was a bit tricky but not that hard after all ;-)) The change requires an update to upstream, which I provided in this PR as a patch.

Testing procedure

I updated tests/lwip to use sock_async_event for the udp server and ip server [edit: and the tcp server] command, so running make -C tests/lwip all test on native should still work.

Issues/PRs references

None.

miri64 · 2020-02-20T17:36:34Z

sys/net/sock/async/event/sock_async_event.c

@@ -98,7 +98,7 @@ void sock_tcp_queue_event_init(sock_tcp_queue_t *queue,
                               event_queue_t *ev_queue,
                               sock_tcp_queue_cb_t handler)
 {
-    sock_async_ctx_t *ctx = sock_tcp_queue_get_async_ctx(sock);
+    sock_async_ctx_t *ctx = sock_tcp_queue_get_async_ctx(queue);


Found this bug while trying to integrate sock_async with sock_tcp. Hope nobody minds I piggy-back this bug fix here. It is in a separate commit, so I can always factor it out if not.

miri64 · 2020-02-20T20:59:49Z

I changed the patch to be a cherry-pick of a feature that was introduced in lwIP's current master. See https://savannah.nongnu.org/patch/?9891#comment1

gschorcht · 2020-02-21T11:26:12Z

I have tested it with esp-wifi and UDP. Still works.

miri64 · 2020-02-21T11:29:09Z

I came around implementing it for TCP after all. Tests should now also work for TCP. In a later step I might even rework the tests a bit to reflect their new asynchronous nature better (e.g. by providing a way to stop the running server, provide support for more than one connection with TCP... etc)

gschorcht · 2020-02-21T11:42:37Z

I came around implementing it for TCP after all. Tests should now also work for TCP. In a later step I might even rework the tests a bit to reflect their new asynchronous nature better (e.g. by providing a way to stop the running server, provide support for more than one connection with TCP... etc)

Behavior for TCP has changed with this PR. After sending something with nc to the TCP server, the TCP server resets the connection after receiving the first data packet and nc can't send anymore.

> TCP client [2003:c1:e703:555:a89d:ce2:fdef:2bc4]:37586 connected
Received TCP data from client [2003:c1:e703:555:a89d:ce2:fdef:2bc4]:37586:
00000000  48  61  6C  6C  6F  0A
TCP connection to [2003:c1:e703:555:a89d:ce2:fdef:2bc4]:37586 reset

In master, I had to leave nc before the TCP connection is released:

> TCP client [2003:c1:e703:555:a89d:ce2:fdef:2bc4]:36896 connected
Received TCP data from client [2003:c1:e703:555:a89d:ce2:fdef:2bc4]:36896:
00000000  48  61  6C  6C  6F  0A
Received TCP data from client [2003:c1:e703:555:a89d:ce2:fdef:2bc4]:36896:
00000000  48  61  6C  6C  6F  0A
TCP connection to [2003:c1:e703:555:a89d:ce2:fdef:2bc4]:36896 reset, starting to accept again

miri64 · 2020-02-21T11:58:26Z

Mhhh curious. For me the behavior (with native) was as you described for the master case was the same when testing. Which netcat version are you using (I use the openbsd one, version 1.206_1-1)? Also which command did you use exactly?

gschorcht · 2020-02-21T12:17:47Z

I'm using the official Ubuntu 18.04 netcat version 1.187. The command I used:

nc -6 <prefix>:32ae:a4ff:fe41:60f8 5555

With wireshark I can observe the following:

PC            ESP8266
 ------ SYN ----->
 <--- SYN/ACK ----
 ------ ACK ----->
 ---- PSH/ACK --->  (contains 'Hallo' as payload)
 <--- FIN/ACK ----  (reaction from TCP server, an ACK but also a FIN)
 ------ ACK ----->

In master, I can observe:

PC            ESP8266
 ------ SYN ----->
 <--- SYN/ACK ----
 ------ ACK ----->
 ---- PSH/ACK --->  (contains 'Hallo' as payload)
 <----- ACK ------  (reaction from TCP server, just an ACK)

miri64 · 2020-02-21T12:33:13Z

Can you please revert the test change (53e6a81 currently) and try again? I'm trying to figure out if this happens due to my changes to the test or lwip_sock* (I guess the first, as none of the changes should effect the implementation of lwIP, but who knows).

gschorcht · 2020-02-21T13:16:44Z

Can you please revert the test change (53e6a81 currently) and try again? I'm trying to figure out if this happens due to my changes to the test or lwip_sock* (I guess the first, as none of the changes should effect the implementation of lwIP, but who knows).

Works without commit 53e6a81.

miri64 · 2020-02-21T13:29:45Z

Ok, I was able to reproduce it now. Trying to find a fix.

miri64 · 2020-02-21T18:09:21Z

Rebased to current master to incorporate #13438. Trying to fix the faulty behavior mentioned in #13438 (comment) now.

miri64 · 2020-02-21T19:19:21Z

Trying to fix the faulty behavior mentioned in #13438 (comment) now.

It turned out to be an error in the sock_tcp implementation itself: when the queue of available sock objects runs full, the netconn facing lwIP is never closed (keeping the TCP connection open in the background). The state machine then gets confused, having conns with socks and conns without (all however use the _netconn_cb as it is inherited from the listening conn). I piggy-backed two changes to prevent that:

close all unused netconn objects when the queue is full on sock_tcp_accept()
activate the TCP_LISTEN_BACKLOG module per default. It doesn't prevent netconns being accepted when the queue is full (as the netconn's internal accept queue could just have been emptied by sock_tcp_accept() e.g., but it at least prevents lwip to waste its resources a little bit.

miri64 · 2020-02-21T20:50:45Z

I also added a regression test for this bug now... If you want I can also move the bug fix to a separate PR. Not sure how easy it is to reproduce without sock_async though :-/

miri64 · 2020-02-21T20:53:22Z

tests/lwip/tests/01-run.py

+        try:
+            connect_addr = socket.getaddrinfo(
+                "%s%%tapbr0" % server_ip, port)[0][4]
+        except socket.gaierror as e:
+            print("SKIP_TEST INFO", e)
+            return


These tests need to be adapted for riotnode (#10949) once that is merged anyway, so I hope you don't mind tapbr0 being statically used here (and expect the boards to be native for the test not to be skipped above) and the test being skipped instead as a short workaround. Otherwise I would need a third "board" just for this test.

kb2ma · 2020-02-26T04:26:19Z

Preliminary testing with gcoap works on an ESP8266. See my gcoap/lwip_async_support branch, which includes IPv4 support.

2020-02-25 22:45:45,431 #  coap get -c 192.168.1.186 5683 /time
2020-02-25 22:45:45,436 # gcoap_cli: sending msg ID 34830, 11 bytes
> 2020-02-25 22:45:45,443 #  gcoap: response Success, code 2.05, 15 bytes
2020-02-25 22:45:45,443 # Feb 26 03:45:45

The down side is that the gcoap thread uses 1596 bytes for IPv4 and 1652 for IPv6 on ESP8266, according to ps.

gschorcht · 2020-02-26T06:56:19Z

The down side is that the gcoap thread uses 1596 bytes for IPv4 and 1652 for IPv6 on ESP8266, according to ps.

Unless I missed something, the stack size is derived from the default stack size as defined by the CPU, which is already 1024 bytes:

#define GCOAP_STACK_SIZE (THREAD_STACKSIZE_DEFAULT + DEBUG_EXTRA_STACKSIZE \
                          + sizeof(coap_pkt_t)

The question is how much stack is really needed. Maybe the stack size could be fine tuned a little bit for gcoap. Have you checked with DEVELHELP=1 what the stack usage for the gcoap thread is?

kb2ma · 2020-02-26T11:52:24Z

Have you checked with DEVELHELP=1 what the stack usage for the gcoap thread is?

Thanks for the feedback. The numbers I quoted above are actual stack usage with DEVELHELP enabled. By default the gcoap thread is allocated 1108 bytes (1024 + 84) on ESP8266. I will pull together a more detailed report to review the default value.

gschorcht · 2020-02-26T23:20:00Z

If you want I can also move the bug fix to a separate PR.

No, that's fine with me.

gschorcht · 2020-02-26T23:43:52Z

It turned out to be an error in the sock_tcp implementation itself: when the queue of available sock objects runs full, the netconn facing lwIP is never closed (keeping the TCP connection open in the background). The state machine then gets confused, having conns with socks and conns without (all however use the _netconn_cb as it is inherited from the listening conn). I piggy-backed two changes to prevent that:

close all unused netconn objects when the queue is full on sock_tcp_accept()

activate the TCP_LISTEN_BACKLOG module per default. It doesn't prevent netconns being accepted when the queue is full (as the netconn's internal accept queue could just have been emptied by sock_tcp_accept() e.g., but it at least prevents lwip to waste its resources a little bit.

Works now as expected.

miri64 · 2020-03-06T08:34:20Z

Ping @gschorcht @kb2ma?

gschorcht

Sorry I lost track of that PR. Looks good and works as expected. ACK

gschorcht · 2020-03-06T10:09:08Z

@miri64 Please squash.

Prevents unnecessary usage of resources, as the queue of `sock_tcp` is restricted.

miri64 · 2020-03-06T10:31:02Z

Squashed

gschorcht · 2020-03-06T13:38:54Z

@miri64 Many thanks for this contribution.

miri64 · 2020-03-06T15:04:40Z

Thanks for the review! :-)

miri64 added Area: network Area: Networking Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: pkg Area: External package ports labels Feb 20, 2020

miri64 added this to the Release 2020.04 milestone Feb 20, 2020

miri64 requested review from kb2ma and gschorcht February 20, 2020 17:33

miri64 requested review from aabadie, cgundogan, fjmolinas, haukepetersen, leandrolanzieri, MichelRottleuthner, MrKevinWeiss, PeterKietzmann and smlng as code owners February 20, 2020 17:33

miri64 mentioned this pull request Feb 20, 2020

net/gcoap: use sock_async and events #13386

Merged

miri64 changed the title ~~Lwip/enh/sock async~~ lwip: provide sock_async support for IP and UDP Feb 20, 2020

miri64 commented Feb 20, 2020

View reviewed changes

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 20, 2020

miri64 force-pushed the lwip/enh/sock_async branch from 5aaf4d0 to 386c3f1 Compare February 21, 2020 11:25

miri64 changed the title ~~lwip: provide sock_async support for IP and UDP~~ lwip: provide sock_async support Feb 21, 2020

miri64 removed the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 21, 2020

miri64 force-pushed the lwip/enh/sock_async branch from 24ce2ba to 0530e9d Compare February 21, 2020 18:08

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 21, 2020

miri64 commented Feb 21, 2020

View reviewed changes

gschorcht approved these changes Mar 6, 2020

View reviewed changes

miri64 added 6 commits March 6, 2020 11:28

lwip: provide sock_async support

fc67e61

tests/lwip: adapt test to use sock_async

ba2dd2c

lwip: fix style errors found by Vera++

e98e2f6

lwip_sock_tcp: close accepted connection if queue is full

1a4e6e9

lwip: activate TCP_LISTEN_BACKLOG per default with sock_tcp

a7ef506

Prevents unnecessary usage of resources, as the queue of `sock_tcp` is restricted.

tests/lwip: add regression test for multiple simultaneous connect case

37df4ea

miri64 force-pushed the lwip/enh/sock_async branch from 6eaa2c7 to 37df4ea Compare March 6, 2020 10:30

gschorcht merged commit fc37d7b into RIOT-OS:master Mar 6, 2020

miri64 deleted the lwip/enh/sock_async branch March 6, 2020 14:02

miri64 mentioned this pull request Mar 6, 2020

gcoap: can't build with network stacks other than GNRC #8130

Closed

This was referenced Mar 10, 2020

gcoap/esp8266: Stack overflow with gcoap example #13606

Open

net/gcoap: add lwIP support #13641

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lwip: provide sock_async support #13427

lwip: provide sock_async support #13427

miri64 commented Feb 20, 2020 •

edited

Loading

miri64 Feb 20, 2020

miri64 commented Feb 20, 2020

gschorcht commented Feb 21, 2020

miri64 commented Feb 21, 2020

gschorcht commented Feb 21, 2020

miri64 commented Feb 21, 2020 •

edited

Loading

gschorcht commented Feb 21, 2020 •

edited

Loading

miri64 commented Feb 21, 2020

gschorcht commented Feb 21, 2020

miri64 commented Feb 21, 2020

miri64 commented Feb 21, 2020

miri64 commented Feb 21, 2020 •

edited

Loading

miri64 commented Feb 21, 2020 •

edited

Loading

miri64 Feb 21, 2020 •

edited

Loading

kb2ma commented Feb 26, 2020 •

edited

Loading

gschorcht commented Feb 26, 2020

kb2ma commented Feb 26, 2020

gschorcht commented Feb 26, 2020

gschorcht commented Feb 26, 2020

miri64 commented Mar 6, 2020

gschorcht left a comment

gschorcht commented Mar 6, 2020

miri64 commented Mar 6, 2020

gschorcht commented Mar 6, 2020

miri64 commented Mar 6, 2020

lwip: provide sock_async support #13427

lwip: provide sock_async support #13427

Conversation

miri64 commented Feb 20, 2020 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

miri64 Feb 20, 2020

Choose a reason for hiding this comment

miri64 commented Feb 20, 2020

gschorcht commented Feb 21, 2020

miri64 commented Feb 21, 2020

gschorcht commented Feb 21, 2020

miri64 commented Feb 21, 2020 • edited Loading

gschorcht commented Feb 21, 2020 • edited Loading

miri64 commented Feb 21, 2020

gschorcht commented Feb 21, 2020

miri64 commented Feb 21, 2020

miri64 commented Feb 21, 2020

miri64 commented Feb 21, 2020 • edited Loading

miri64 commented Feb 21, 2020 • edited Loading

miri64 Feb 21, 2020 • edited Loading

Choose a reason for hiding this comment

kb2ma commented Feb 26, 2020 • edited Loading

gschorcht commented Feb 26, 2020

kb2ma commented Feb 26, 2020

gschorcht commented Feb 26, 2020

gschorcht commented Feb 26, 2020

miri64 commented Mar 6, 2020

gschorcht left a comment

Choose a reason for hiding this comment

gschorcht commented Mar 6, 2020

miri64 commented Mar 6, 2020

gschorcht commented Mar 6, 2020

miri64 commented Mar 6, 2020

miri64 commented Feb 20, 2020 •

edited

Loading

miri64 commented Feb 21, 2020 •

edited

Loading

gschorcht commented Feb 21, 2020 •

edited

Loading

miri64 commented Feb 21, 2020 •

edited

Loading

miri64 commented Feb 21, 2020 •

edited

Loading

miri64 Feb 21, 2020 •

edited

Loading

kb2ma commented Feb 26, 2020 •

edited

Loading