[riot-notifications] [RIOT-OS/RIOT] gnrc_tcp: experimental feature "dynamic msl" (#16764)

Jean Pierre Dudey notifications at github.com
Tue Aug 31 10:40:57 CEST 2021


@jeandudey commented on this pull request.



> +#if CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_EN
+#ifndef CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_RTO_MUL
+#define CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_RTO_MUL (4U)
+#endif
+#endif

```suggestion
#ifndef CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_RTO_MUL
#define CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_RTO_MUL (4U)
#endif
```

> @@ -185,6 +185,26 @@ extern "C" {
 #ifndef CONFIG_GNRC_TCP_EVENTLOOP_MSG_QUEUE_SIZE_EXP
 #define CONFIG_GNRC_TCP_EVENTLOOP_MSG_QUEUE_SIZE_EXP (3U)
 #endif
+
+/**
+ * @brief Enable experimental feature "dynamic msl". Disabled by default.
+ * @note This features calculates the MSL based by multiplying the latest
+ *       retransmission timeout value with
+ *       CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_RTO_MUL. This leads to much
+ *       faster return times on gnrc_tcp_close.
+ */
+#ifndef CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_EN
+#define CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_EN (0U)

```suggestion
#define CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_EN 0
```

> +    size_t val = CONFIG_GNRC_TCP_MSL_MS << 1;
+
+    /* Experimental feature "dynamic msl", calculate timewait timer value
+     * based on the current retransmission timeout.
+     */
+#if CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_EN
+    size_t dyn_msl = CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_RTO_MUL * tcb->rto;
+    if (dyn_msl < val) {
+        val = dyn_msl;
+    }
+#endif

```suggestion
    size_t val = CONFIG_GNRC_TCP_MSL_MS << 1;
    /* Experimental feature "dynamic msl", calculate timewait timer value
     * based on the current retransmission timeout.
     */
    if (IS_ACTIVE(CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_EN)) {
        size_t dyn_msl = CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_RTO_MUL * tcb->rto;
        if (dyn_msl < val) {
            val = dyn_msl;
        }
    }
```

So the CI checks for this code, and will be removed anyways by the optimizer.

> +#if CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_EN
+#ifndef CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_RTO_MUL
+#define CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_RTO_MUL (4U)
+#endif
+#endif

See other review suggestion for the reason for this change

> @@ -185,6 +185,26 @@ extern "C" {
 #ifndef CONFIG_GNRC_TCP_EVENTLOOP_MSG_QUEUE_SIZE_EXP
 #define CONFIG_GNRC_TCP_EVENTLOOP_MSG_QUEUE_SIZE_EXP (3U)
 #endif
+
+/**
+ * @brief Enable experimental feature "dynamic msl". Disabled by default.
+ * @note This features calculates the MSL based by multiplying the latest
+ *       retransmission timeout value with
+ *       CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_RTO_MUL. This leads to much
+ *       faster return times on gnrc_tcp_close.
+ */
+#ifndef CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_EN
+#define CONFIG_GNRC_TCP_EXPERIMENTAL_DYN_MSL_EN (0U)

So `IS_ACTIVE` macro works

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16764#pullrequestreview-742436310
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210831/354c1852/attachment-0001.htm>


More information about the notifications mailing list