Less tcp packet fragmentation (IDFGH-11082) #12256

0xFEEDC0DE64 · 2023-09-15T13:59:05Z

I noticed that our products waste about 40% of their tcp traffic, because we send every second a very short status update through a websocket client to our cloud and esp_websocket_client keeps sending two tcp packets, one for the websocket header, and one for the websocket payload.

I tried merging those two buffers into a single one by providing additional websocket methods that expect a unused 16 byte prefix buffer to put the websocket header into, meaning the user code has to preallocate these bytes already.

websocket_client from esp-protocols also has a patch available, which uses those new optimized websocket sending mechanisms and my wireshark recording shows great success! all websocket headers are now merged together with the websocket payload.

I expect lots of review comments and change requests, I just want to start the discussion how to integrate this properly in the esp-idf

david-cermak · 2023-10-12T08:21:34Z

components/esp-tls/esp_tls.c

@@ -123,7 +123,7 @@ static ssize_t tcp_read(esp_tls_t *tls, char *data, size_t datalen)

 static ssize_t tcp_write(esp_tls_t *tls, const char *data, size_t datalen)
 {
-    return send(tls->sockfd, data, datalen, 0);
+    return send(tls->sockfd, data, datalen, MSG_MORE);


I don't think we could hard-code this flag here. It makes sense for this specific use-case, but it's not universally the case for other usages. In fact, users usually prefer that the data are sent immediately, even for TCP, so we typically suggest that people set TCP_NODELAY to disable naggling.

I'd suggest adding this to esp_tls config options, something like:

@@ -200,6 +200,7 @@ typedef struct esp_tls_cfg { esp_tls_addr_family_t addr_family; /*!< The address family to use when connecting to a host. */ const int *ciphersuites_list; /*!< Pointer to a zero-terminated array of IANA identifiers of TLS ciphersuites. Please check the list validity by esp_tls_get_ciphersuites_list() API */ + int socket_flags; } esp_tls_cfg_t;

david-cermak · 2023-10-12T08:21:55Z

components/esp_http_server/src/httpd_txrx.c

@@ -56,7 +56,7 @@ int httpd_send(httpd_req_t *r, const char *buf, size_t buf_len)
    }

    struct httpd_req_aux *ra = r->aux;
-    int ret = ra->sd->send_fn(ra->sd->handle, ra->sd->fd, buf, buf_len, 0);
+    int ret = ra->sd->send_fn(ra->sd->handle, ra->sd->fd, buf, buf_len, MSG_MORE);


same here, suggest making this configurable

david-cermak · 2023-10-12T08:24:57Z

components/tcp_transport/include/esp_transport_ws.h

+ * @brief               Sends websocket raw message with custom opcode and payload in a optimized way
+ *
+ * This method is similar to esp_transport_ws_send_raw(), but
+ * it assumes, that the first MAX_WEBSOCKET_HEADER_SIZE bytes


Maybe add this note as a @pre (prerequisite for using this API) that it expects the buffer to hold some space for headers.

david-cermak · 2023-10-12T08:26:35Z

components/tcp_transport/include/esp_transport_ws.h

+ * @param[in]  t           Websocket transport handle
+ * @param[in]  opcode      ws operation code
+ * @param[in]  buffer      The buffer
+ * @param[in]  len         The length


It should be made clear that the len parameter refers to the entire buffer with headers, so the actual (payload) length is len-MAX_WEBSOCKET_HEADER_SIZE

david-cermak · 2023-10-12T08:30:23Z

components/tcp_transport/include/esp_transport_ws.h

+ *  - Number of bytes was written
+ *  - (-1) if there are any errors, should check errno
+ */
+int esp_transport_ws_send_raw_optimized(esp_transport_handle_t t, ws_transport_opcodes_t opcode, const char *b, int len, int timeout_ms);


It's just a matter of taste, but I much don't like API names suffixed with ..._optimized(). Could we think of a better name that would reflect the actual purpose? Maybe ..send_raw_once(), ...send_raw_ext().

david-cermak · 2023-10-12T08:31:31Z

components/tcp_transport/transport_ws.c

+    int poll_write;
+    if ((poll_write = esp_transport_poll_write(ws->parent, timeout_ms)) <= 0) {
+        ESP_LOGE(TAG, "Error transport_poll_write");
+        return poll_write;
+    }
+
+    int len2 = len - MAX_WEBSOCKET_HEADER_SIZE;
+    if (len2 <= 125) {
+        ws_header = buffer+MAX_WEBSOCKET_HEADER_SIZE-2-4;
+        ws_header[header_len++] = opcode;
+        ws_header[header_len++] = (uint8_t)(len2 | mask_flag);
+    } else if (len2 < 65536) {
+        ws_header = buffer+MAX_WEBSOCKET_HEADER_SIZE-4-4;
+        ws_header[header_len++] = opcode;
+        ws_header[header_len++] = WS_SIZE16 | mask_flag;
+        ws_header[header_len++] = (uint8_t)(len2 >> 8);
+        ws_header[header_len++] = (uint8_t)(len2 & 0xFF);
+    } else {
+        ws_header = buffer+MAX_WEBSOCKET_HEADER_SIZE-10-4;
+        ws_header[header_len++] = opcode;
+        ws_header[header_len++] = WS_SIZE64 | mask_flag;
+        /* Support maximum 4 bytes length */
+        ws_header[header_len++] = 0; //(uint8_t)((len >> 56) & 0xFF);
+        ws_header[header_len++] = 0; //(uint8_t)((len >> 48) & 0xFF);
+        ws_header[header_len++] = 0; //(uint8_t)((len >> 40) & 0xFF);
+        ws_header[header_len++] = 0; //(uint8_t)((len >> 32) & 0xFF);
+        ws_header[header_len++] = (uint8_t)((len2 >> 24) & 0xFF);
+        ws_header[header_len++] = (uint8_t)((len2 >> 16) & 0xFF);
+        ws_header[header_len++] = (uint8_t)((len2 >> 8) & 0xFF);
+        ws_header[header_len++] = (uint8_t)((len2 >> 0) & 0xFF);
+    }
+
+    if (mask_flag) {
+        ws_header[header_len++] = 0;
+        ws_header[header_len++] = 0;
+        ws_header[header_len++] = 0;
+        ws_header[header_len++] = 0;


This part could be made a static function that would be called from both variants of send_raw() functions

david-cermak

@0xFEEDC0DE64 Nice work! The changes make a lot of sense to me and this PR is a good start, but still needs some polishing.

david-cermak · 2023-10-12T08:39:04Z

I just want to start the discussion how to integrate this properly in the esp-idf

Please note that IDF uses this concept of "code owners" defined in

esp-idf/.gitlab/CODEOWNERS

Line 143 in 8fc8f3f

/components/tcp_transport/ @esp-idf-codeowners/network

and your PR touches two different areas with two owners. I'd suggest that you split your changeset into a smaller per-component PRs, with very few changes. This way it could be very easily reviewed by responsible code-owners from Espressif side.

You can use this PR for changes in tcp_transport, I've posted some comments to the code, but this LGTM overall and could be merged almost immediately after you address the comments.

david-cermak · 2023-11-28T17:13:31Z

@0xFEEDC0DE64 Any news from your side? The ideas you shared in this PR look very good to me and I think would be useful to others, we just need to formally proceed with the changes.

Could you please check if you can split the change in two PRs?

0xFEEDC0DE64 · 2023-12-19T16:06:55Z

sorry didnt have enough time, and we didnt do a rebase for a long time, maybe i find time to do this finally in the next few days, we are just doing a rebase of everything right now

david-cermak · 2024-10-01T10:21:49Z

@0xFEEDC0DE64 Will you have time to check the status of this PR and update your work per my comments?
If not, just let me know, please. I can take care of these little fixes on top of your commit and proceed with accepting it, as I still think the proposed changes make perfect sense and should be merged.

0xFEEDC0DE64 · 2024-10-18T12:09:11Z

Sorry, I cant do it at the moment, our esp-idf fork already has sooo many fixes and improvements ontop that I dont even know which commits I have to cherry pick to here to upstream them to you. And I dont have enough time at the moment to do this. Lets close this pull request

Less tcp packet fragmentation

14ca432

espressif-bot added the Status: Opened Issue is new label Sep 15, 2023

github-actions bot changed the title ~~Less tcp packet fragmentation~~ Less tcp packet fragmentation (IDFGH-11082) Sep 15, 2023

espressif-bot assigned david-cermak Sep 22, 2023

david-cermak reviewed Oct 12, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Less tcp packet fragmentation (IDFGH-11082) #12256

Less tcp packet fragmentation (IDFGH-11082) #12256

0xFEEDC0DE64 commented Sep 15, 2023

david-cermak Oct 12, 2023

david-cermak Oct 12, 2023

david-cermak Oct 12, 2023

david-cermak Oct 12, 2023

david-cermak Oct 12, 2023

david-cermak Oct 12, 2023

david-cermak left a comment

david-cermak commented Oct 12, 2023

david-cermak commented Nov 28, 2023

0xFEEDC0DE64 commented Dec 19, 2023

david-cermak commented Oct 1, 2024

0xFEEDC0DE64 commented Oct 18, 2024

Less tcp packet fragmentation (IDFGH-11082) #12256

Are you sure you want to change the base?

Less tcp packet fragmentation (IDFGH-11082) #12256

Conversation

0xFEEDC0DE64 commented Sep 15, 2023

david-cermak Oct 12, 2023

Choose a reason for hiding this comment

david-cermak Oct 12, 2023

Choose a reason for hiding this comment

david-cermak Oct 12, 2023

Choose a reason for hiding this comment

david-cermak Oct 12, 2023

Choose a reason for hiding this comment

david-cermak Oct 12, 2023

Choose a reason for hiding this comment

david-cermak Oct 12, 2023

Choose a reason for hiding this comment

david-cermak left a comment

Choose a reason for hiding this comment

david-cermak commented Oct 12, 2023

david-cermak commented Nov 28, 2023

0xFEEDC0DE64 commented Dec 19, 2023

david-cermak commented Oct 1, 2024

0xFEEDC0DE64 commented Oct 18, 2024