sys/posix/socket: implement MSG_PEEK for recvfrom #16850

HendrikVE · 2021-09-14T12:31:45Z

Contribution description

These changes make it possible to peek bytes from the socket instead of reading them only once. When in peek mode all read bytes are stored in a ringbuffer for later use.

Testing procedure

You can test this PR e.g. in conjunction with everything else in #15969.

Issues/PRs references

This PR is split out from the chunky #15969.

sys/posix/sockets/posix_sockets.c

miri64

Accidentally only commented.

chrysn · 2021-09-17T17:37:03Z

I've had a look at this for implementing CoAP-over-TCP (with some thoughts at rust-embedded-community/embedded-nal#59).

Why is there a need for an extra buffer? AIU each TCP socket from GNRC already has a buffer, so peeking could just read that buffer without advancing the read cursor. Other stacks may or may not support this, but then the read call fails.

miri64 · 2021-09-20T07:56:08Z

Why is there a need for an extra buffer? AIU each TCP socket from GNRC already has a buffer, so peeking could just read that buffer without advancing the read cursor. Other stacks may or may not support this, but then the read call fails.

See #16739 (comment) ff

chrysn · 2021-09-20T09:34:41Z

I don't understand how the 16739 comment relates. Right now we have

the GNRC packet buffers
the per-socket reassembled stream

This commit adds on top of this a

the per-socket peeked data that was moved over

Now what comes out of 16739 might smush 1 and 2 into the same thing, but either way, a read that does not advance the read cursor (inside 1 or the combined 1+2 pktbuf chain) would achieve the same without an additional buffer (and, above that, do backpressure better).

miri64 · 2021-09-20T10:48:33Z

This commit adds on top of this a
1. the per-socket peeked data that was moved over
Now what comes out of 16739 might smush 1 and 2 into the same thing, but either way, a read that does not advance the read cursor (inside 1 or the combined 1+2 pktbuf chain) would achieve the same without an additional buffer (and, above that, do backpressure better).

Ahh yes. Sorry, I forgot that yab (yet another buffer) was introduced here and I thought you were referring to the reassembled buffers.

HendrikVE · 2021-09-20T15:13:05Z

So using a read cursor sounds like a good idea if we can remove the additional buffer with it. If everything is based on gnrc (correct me if I'm wrong please) then this mechanism would we available from gnrc through lwip to posix in the end, right? I think I should let you folks figure this one out as I'm not a network developer and quite frankly I have no intention of becoming one. I won't be able to see the implications of implementing such a cursor (I don't even know where to start...). In the end I just had to touch some network stuff to get my work done.

chrysn · 2021-09-20T15:13:46Z

The discussions on size and size source for the PEEK_BUF would be moot if this would be done at socket level.

I encourage you to look at sock_tcp_read and gnrc_tcp_recv, the FSM_EVENT_CALL_RECV event, and how they could be extended: Running a parallel chain (sock_tcp_peek, gnrc_tcp_recv2?) or running with flags that'll eventually call ringbuffer_peek rather than ringbuffer_get would allow playing all this in the existing buffers.

(That is, if @miri64 approves of this direction; she has the better overview of the directions these layers should take.)

miri64 · 2021-09-20T15:14:05Z

then this mechanism would we available from gnrc through lwip to posix in the end, right?

If you use lwip there is no GNRC. Not sure how to parse this sentence otherwise.

miri64 · 2021-09-20T15:15:31Z

I encourage you to look at sock_tcp_read and gnrc_tcp_recv, the FSM_EVENT_CALL_RECV event, and how they could be extended: Running a parallel chain (sock_tcp_peek, gnrc_tcp_recv2?) or running with flags that'll eventually call ringbuffer_peek rather than ringbuffer_get would allow playing all this in the existing buffers.

I think, since this is originally from #15969, that @HendrikVE is using lwIP, so just looking at GNRC won't help much.

HendrikVE · 2021-09-20T15:16:15Z

then this mechanism would we available from gnrc through lwip to posix in the end, right?

If you use lwip there is no GNRC. Not sure how to parse this sentence otherwise.

Then what good does it bring against my problem with posix sockets and lwip implementing a read cursor in gnrc?

HendrikVE · 2021-09-20T15:16:35Z

I encourage you to look at sock_tcp_read and gnrc_tcp_recv, the FSM_EVENT_CALL_RECV event, and how they could be extended: Running a parallel chain (sock_tcp_peek, gnrc_tcp_recv2?) or running with flags that'll eventually call ringbuffer_peek rather than ringbuffer_get would allow playing all this in the existing buffers.

I think, since this is originally from #15969, that @HendrikVE is using lwIP, so just looking at GNRC won't help much.

Exactly^^

miri64 · 2021-09-20T15:18:44Z

But it might be a good idea to look how in lwIP they implemented MSG_PEEK for their POSIX socket implementation to get a hint on that. We could extend sock_tcp in that direction. Since the gnrc port for that is currently WIP, integrating that later would be easy(er?).

chrysn · 2021-09-20T15:20:57Z

Then what good does it bring against my problem with posix sockets and lwip implementing a read cursor in gnrc?

I was unaware that lwIP was your target platform -- but the large-scale approach would be the same: sock_tcp_read would need a sock_tcp_peak counterpart, and then initially that could be implemented for lwIP (as miri suggested, by looking at how they do MSG_PEEK for their own POSIX layer), and would be initially unavailable for GNRC backed sockets.

miri64 · 2021-09-20T15:22:19Z

But it might be a good idea to look how in lwIP they implemented MSG_PEEK for their POSIX socket implementation to get a hint on that. We could extend sock_tcp in that direction. Since the gnrc port for that is currently WIP, integrating that later would be easy(er?).

See https://git.savannah.nongnu.org/cgit/lwip.git/tree/src/api/sockets.c?h=STABLE-2_1_x#n932 ff

HendrikVE · 2022-01-09T13:44:29Z

I finally found the time to come back to this. I did a major rework of this PR. As suggested I integrated the peek behaviour directly into lwIP which yields a few advantages:

I was able to make use of the existing test infrastructure for lwip_sock_*
No extra buffer introduced by the posix socket

The disadvantages are:
- Available for lwIP only at the moment
- Similiar code (duplication, thus more code) accross lwip_sock_ip, lwip_sock_udp and lwip_sock_tcp

The advantages clearly outweigh the disadvantages.

For tcp I moved sock_tcp_read to _tcp_read for compatibility reasons so that I could change the function signature and add the new parameter bool peek. For ip and udp I was able to make use of the existing auxiliary flags. I know they were probably not initially thought for this, but this way the function signatures do not need to be modified. I think we could do the same with tcp too.

pkg/lwip/contrib/sock/ip/lwip_sock_ip.c

chrysn · 2022-10-06T15:04:27Z

sys/include/net/sock.h

+    /**
+     * @brief   Just peek data and keep it for later use
+     *
+     * @note    Currently only supported for lwIP!


Suggested change

* @note Currently only supported for lwIP!

* @note Select module `sock_aux_peek` and a compatible network stack

* to use this. Currently only supported for lwIP.

I also renamed the module to sock_aux_peek since the aux part was missing

I don't think that the aux part is needed: that part just means that there is an extra _aux in the argument. So a _peek should not have an _aux in it as it takes no _aux argument.

The other aux options sock_aux_local, sock_aux_rssi and sock_aux_time.stamp also have this prefix. Sure, unlike peek, they define what else to request, not how to request. But I think for consistency of the enum it is better to use this prefix here as well, even if SOCK_AUX_PEEK was smuggled in there just to make use of the aux-function infrastructure.

pkg/lwip/contrib/sock/ip/lwip_sock_ip.c

chrysn · 2022-10-06T21:19:44Z

Pondering how this would best be implemented on GNRC I was briefly wondering whether this would need special considerations for interactions with async -- given that (in the level triggered "something is available" mode of POSIX's select), a failed peek might lead to a deadlock where no new event happens that would wake up the thread that is hoping for a sufficiently long peek.

This is not an issue here: async TCP sockets generate a callback on every recv event, so it can be handled just fine. (If that callback sets something level-ish, that's fine as long as the peek clears that level).

So I think on the async side it's all good.

pkg/lwip/contrib/sock/ip/lwip_sock_ip.c

benpicco · 2022-10-17T20:31:18Z

pkg/lwip/include/sock_types.h

+    mutex_t mutex;                  /**< Mutex to protect the sock */
+    struct netbuf *last_buf;        /**< Last received data */
+    bool peek_buf_avail;            /**< Whether there is a buffer left from a previous peek operation */


Should those be inside an

#ifdef MODULE_SOCK_AUX_PEEK … #endif

?

That would unfortunately entail ifdefs all over, since IS_USED(MODULE_SOCK_AUX_PEEK) is not enough for the application to compile (tons of has no member named). This would clutter the code too much. In my opinion the trade-off that these variables are always included vs cluttering the code with ifdefs is ok, but I can understand that people here might disagree.

But it will add 16 bytes to each socket, even when not used.

Btw: Why is the mutex needed now? Can sockets now be shared between multiple threads?

But it will add 16 bytes to each socket, even when not used.

I added a fixup commit showing how it would look like when removing the members from the struct instead. If you prefer this I'll merge this into the branch.

Btw: Why is the mutex needed now? Can sockets now be shared between multiple threads?

Now that you mention it... Seems like they are not intended to be thread-safe at all. I removed the mutex entirely.

benpicco · 2022-10-17T20:31:27Z

pkg/lwip/include/sock_types.h

@@ -127,6 +130,9 @@ struct sock_tcp_queue {
 */
 struct sock_udp {
    lwip_sock_base_t base;          /**< parent class */
+    mutex_t mutex;                  /**< Mutex to protect the sock */
+    struct netbuf *last_buf;        /**< Last received data */
+    bool peek_buf_avail;            /**< Whether there is a buffer left from a previous peek operation */


btw is there a connection between last_buf and peek_buf_avail?

We keep last_buf always in the sock as is until it is received without the peek flag. peek_buf_avail is basically used to keep track of whether we already read from last_buf or not which is important for sock_ip_recv_aux which is looping over sock_ip_recv_buf_aux. The first call of sock_ip_recv_buf_aux will return the peeked data and a result > 0. When calling sock_ip_recv_buf_aux a second time (since the result was > 0) we reset the flag and return 0 to break the loop. Returning 0 instantly would be another option, but that does not seem right.

sys/posix/sockets/posix_sockets.c

benpicco · 2023-05-30T17:51:23Z

pkg/lwip/contrib/sock/ip/lwip_sock_ip.c

+         * This assertion will trigger should the behavior change in the future.
+         */
+        if (old_ctx) {
+            assert(res == 0 && ctx == NULL);


args so @miri64 pointed out that it's actually LWIP that makes use of that and that comment is wrong 😬

github-actions bot added the Area: sys Area: System label Sep 14, 2021

HendrikVE added the Type: new feature The issue requests / The PR implemements a new feature for RIOT label Sep 14, 2021

HendrikVE mentioned this pull request Sep 14, 2021

pkg/wolfmqtt: add wolfmqtt #15969

Open

8 tasks

HendrikVE added the Area: POSIX Area: POSIX API wrapper label Sep 14, 2021

benpicco requested a review from miri64 September 14, 2021 15:50

miri64 reviewed Sep 16, 2021

View reviewed changes

sys/posix/sockets/posix_sockets.c Outdated Show resolved Hide resolved

sys/posix/sockets/posix_sockets.c Outdated Show resolved Hide resolved

sys/posix/sockets/posix_sockets.c Outdated Show resolved Hide resolved

sys/posix/sockets/posix_sockets.c Outdated Show resolved Hide resolved

miri64 previously requested changes Sep 16, 2021

View reviewed changes

HendrikVE force-pushed the pr/wolfmqtt_split_2 branch from d91ceba to d10f512 Compare January 9, 2022 13:41

HendrikVE requested a review from maribu as a code owner January 9, 2022 13:41

github-actions bot added Area: network Area: Networking Area: pkg Area: External package ports Area: tests Area: tests and testing framework labels Jan 9, 2022

maribu reviewed Jan 9, 2022

View reviewed changes

pkg/lwip/contrib/sock/ip/lwip_sock_ip.c Show resolved Hide resolved

HendrikVE force-pushed the pr/wolfmqtt_split_2 branch from d10f512 to b33b221 Compare January 9, 2022 22:10

HendrikVE added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 9, 2022

github-actions bot added the Area: build system Area: Build system label Jan 13, 2022

chrysn reviewed Oct 6, 2022

View reviewed changes

chrysn mentioned this pull request Oct 6, 2022

gnrc/tcp: Add support for peeking #18704

Draft

HendrikVE force-pushed the pr/wolfmqtt_split_2 branch from 9954e85 to 6b6a5d9 Compare October 7, 2022 13:47

HendrikVE added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 7, 2022

HendrikVE force-pushed the pr/wolfmqtt_split_2 branch 2 times, most recently from fe1fcde to e85704c Compare October 7, 2022 14:07

aabadie added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 7, 2022

HendrikVE force-pushed the pr/wolfmqtt_split_2 branch 2 times, most recently from 45d8d4e to 01de0e1 Compare October 10, 2022 12:21

benpicco requested a review from yarrick October 17, 2022 20:22

benpicco reviewed Oct 17, 2022

View reviewed changes

HendrikVE force-pushed the pr/wolfmqtt_split_2 branch from 01de0e1 to 098df85 Compare October 18, 2022 08:54

HendrikVE and others added 14 commits May 30, 2023 17:24

pkg/lwip: add peek to sock_ip

a8f07c5

pkg/lwip: add peek to sock_udp

60baa39

pkg/lwip: add peek to sock_tcp

bc0a80c

sys/posix: add flag definitions

64beb73

sys/posix/socket: implement MSG_PEEK for recvfrom

b30e78b

sock_aux_peek: add and use pseudomodule

c6ef08b

tests/lwip_sock_tcp: test both ipv4 and ipv6 by default

363f839

tests/lwip_sock_udp: test both ipv4 and ipv6 by default

03ac5b4

tests/lwip_sock_ip: test both ipv4 and ipv6 by default

0976622

tests/lwip_sock_ip: exclude a few boards

d0dc832

tests/lwip_sock_udp: exclude a few boards

fcbdbde

fixup! remove mutex

e12ed33

fixup! compile members only in when using peek module

69d4b6f

fixup! check assertion for second chunk

9dd2aa0

HendrikVE force-pushed the pr/wolfmqtt_split_2 branch from 98e00b1 to 9dd2aa0 Compare May 30, 2023 15:41

benpicco reviewed May 30, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys/posix/socket: implement MSG_PEEK for recvfrom #16850

sys/posix/socket: implement MSG_PEEK for recvfrom #16850

HendrikVE commented Sep 14, 2021

miri64 left a comment

chrysn commented Sep 17, 2021

miri64 commented Sep 20, 2021

chrysn commented Sep 20, 2021

miri64 commented Sep 20, 2021

HendrikVE commented Sep 20, 2021

chrysn commented Sep 20, 2021

miri64 commented Sep 20, 2021

miri64 commented Sep 20, 2021

HendrikVE commented Sep 20, 2021

HendrikVE commented Sep 20, 2021

miri64 commented Sep 20, 2021

chrysn commented Sep 20, 2021

miri64 commented Sep 20, 2021

HendrikVE commented Jan 9, 2022

chrysn Oct 6, 2022

HendrikVE Oct 7, 2022

HendrikVE Oct 7, 2022

chrysn Oct 7, 2022

HendrikVE Oct 10, 2022

chrysn commented Oct 6, 2022

benpicco Oct 17, 2022

HendrikVE Oct 18, 2022

benpicco Oct 18, 2022 •

edited

Loading

HendrikVE Oct 18, 2022

benpicco Oct 17, 2022

benpicco Oct 18, 2022

HendrikVE Oct 18, 2022

benpicco May 30, 2023

	* @note Currently only supported for lwIP!
	* @note Select module `sock_aux_peek` and a compatible network stack
	* to use this. Currently only supported for lwIP.

sys/posix/socket: implement MSG_PEEK for recvfrom #16850

Are you sure you want to change the base?

sys/posix/socket: implement MSG_PEEK for recvfrom #16850

Conversation

HendrikVE commented Sep 14, 2021

Contribution description

Testing procedure

Issues/PRs references

miri64 left a comment

Choose a reason for hiding this comment

chrysn commented Sep 17, 2021

miri64 commented Sep 20, 2021

chrysn commented Sep 20, 2021

miri64 commented Sep 20, 2021

HendrikVE commented Sep 20, 2021

chrysn commented Sep 20, 2021

miri64 commented Sep 20, 2021

miri64 commented Sep 20, 2021

HendrikVE commented Sep 20, 2021

HendrikVE commented Sep 20, 2021

miri64 commented Sep 20, 2021

chrysn commented Sep 20, 2021

miri64 commented Sep 20, 2021

HendrikVE commented Jan 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrysn commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpicco Oct 18, 2022 •

edited

Loading