functional option MPTCP with --mptcp #2

CrapsDorian · 2024-03-21T09:26:22Z

An HTTP test server indicating whether MPTCP (Multipath TCP) is utilized is now available at the following address: http://test.multipath-tcp.org:5000/.

Attention points:

This PR replaces First version of Multipath TCP support in curl curl/curl#9713.
Sorry for functional option MPTCP with --mptcp curl/curl#13161, I accidentally opened it while the code was not ready yet, my bad.
Regarding the documentation, we didn't find a good reference (See-also). We put tcp-fastopen because we had to put something, but we are happy to change.
It is not clear for us if a test is worth it. According to your comments from First version of Multipath TCP support in curl curl/curl#9713, it might not be worth it. We successfully tested it with http://test.multipath-tcp.org:5000 .
The transport is changed to TRNSPRT_MPTCP in Curl_cf_tcp_create(), not to change the logic before ("happy eyeballs", etc.), and because this feature is an extension to TCP, and it should only modify TCP connections. We are happy to change that if there is a better way.
In the declaration of struct OperationConfig in src/tool_cfgable.h, the struct OperationConfig *next; item has this comment next to it: /* Always last in the struct */, but it is no longer the last item in the structure. Is it normal?

matttbe

Nice, looks god, just some various things

matttbe · 2024-03-21T11:42:16Z

docs/cmdline-opts/mptcp.md

+Category: connection
+Multi: boolean
+See-also:
+  - tcp-fastopen


I'm not sure what else we can give, maybe request?
We can also wait for advices from the maintainers

can you add in the PR description, something like:

Attention points: - This PR replaces #9713. - Sorry for #13161, I accidentally opened it while the code was not ready yet, my bad. - Regarding the documentation, we didn't find a good reference (`See-also`). We put `tcp-fastopen` because we had to put something, but we are happy to change. - It is not clear for us if a test is worth it. According to your comments from #9713, it might not be worth it. We successfully tested it with http://test.multipath-tcp.org:5000 .

matttbe · 2024-03-21T11:42:58Z

docs/cmdline-opts/mptcp.md

+
+## Availability
+
+The `--mptcp` option is available starting from `curl` version 8.6.1.


matttbe · 2024-03-21T11:44:16Z

docs/cmdline-opts/mptcp.md

+
+## Requirements
+
+- Your operating system must support MPTCP, and it must be enabled.


Probably best to add:

Currently, this feature is only supported on Linux.

This feature is currently only supported on Linux starting from kernel 5.6. ?

Yes, good idea!

This feature is currently only supported on Linux 5.6 or later.

matttbe · 2024-03-21T11:44:51Z

docs/cmdline-opts/mptcp.md

+## Requirements
+
+- Your operating system must support MPTCP, and it must be enabled.
+- The server you are connecting to must also support MPTCP.


Probably best to add:

If not, the connection will fallback to TCP.

matttbe · 2024-03-21T11:51:14Z

docs/libcurl/opts/CURLOPT_MPTCP.md

+to be used simultaneously by a single TCP connection, enhancing redundancy, 
+bandwidth, and potentially reducing latency. 
+It works by presenting a standard TCP interface to applications while 
+managing multiple underlying TCP connections.


Maybe best to use the same paragraph here as well

MPTCP is an extension to the standard TCP that allows multiple TCP streams over different network paths between the same source and destination. This can enhance bandwidth and improve reliability by using multiple paths simultaneously.
MPTCP is beneficial in networks where multiple paths exist between clients and servers, such as mobile networks where a device may switch between Wi-Fi and cellular data or in wired networks with multiple ISPs.

For the paragraph above and below. You can keep the note

matttbe · 2024-03-21T11:59:07Z

lib/cf-socket.c

+  }  
+#ifdef __linux__
+  if (data->set.mptcp)  {
+     transport = TRNSPRT_MPTCP;


this line is not aligned properly: should be indented by 2 spaces, not 3

(I know it looks like details, but consistency is important, maybe they use "beautifier" tools like uncrustify, and you don't want to cause unwanted changes, and you don't want such comments during the review)

matttbe · 2024-03-21T12:02:57Z

lib/cf-socket.h

@@ -28,6 +28,10 @@
 #include "nonblock.h" /* for curlx_nonblock(), formerly Curl_nonblock() */
 #include "sockaddr.h"

+#ifndef IPPROTO_MPTCP
+#define IPPROTO_MPTCP 262
+#endif


maybe best to move this to lib/cf-socket.c, at the top (? or just above where you use it?), it is only used in this file, in a single place.

matttbe · 2024-03-21T12:03:19Z

lib/setopt.c

@@ -2931,6 +2931,9 @@ CURLcode Curl_vsetopt(struct Curl_easy *data, CURLoption option, va_list param)
    result = CURLE_NOT_BUILT_IN;
 #endif
    break;
+  case  CURLOPT_MPTCP:


style: only one space after case

matttbe · 2024-03-21T12:04:51Z

lib/urldata.h

@@ -1811,6 +1812,7 @@ struct UserDefined {
 #ifdef USE_WEBSOCKETS
  BIT(ws_raw_mode);
 #endif
+  bool mptcp; /* enable MPTCP support */


best to use BIT(mptcp); like the other ones above. (you only need 1 bit)

matttbe · 2024-03-21T12:06:04Z

src/tool_cfgable.h

@@ -298,6 +298,7 @@ struct OperationConfig {
  struct State state;             /* for create_transfer() */
  bool rm_partial;                /* on error, remove partially written output
                                     files */
+  bool mptcp;


can you add /* enable MPTCP support */ (even if it is a bit obvious...)

matttbe

Thx for the update. Don't forget to update the commit message.

Feel free to also update the PR description with the additional notes we prepare, so you just have to copy-paste when the PR will be opened later

matttbe · 2024-03-21T13:39:41Z

lib/cf-socket.c

@@ -1601,7 +1605,15 @@ CURLcode Curl_cf_tcp_create(struct Curl_cfilter **pcf,
  if(!ctx) {
    result = CURLE_OUT_OF_MEMORY;
    goto out;
-  }
+  }  


unwanted change

matttbe · 2024-03-21T13:40:28Z

lib/cf-socket.c

+#endif
+
+#ifdef __linux__
+  if(data->set.mptcp) transport = TRNSPRT_MPTCP;


can you put transport = ... at the next line?

if(data->set.mptcp) transport = TRNSPRT_MPTCP;

matttbe · 2024-03-21T13:41:46Z

lib/cf-socket.c

@@ -244,6 +244,10 @@ void Curl_sock_assign_addr(struct Curl_sockaddr_ex *dest,
    dest->socktype = SOCK_STREAM;
    dest->protocol = IPPROTO_IP;
    break;
+  case TRNSPRT_MPTCP:


can you move

#ifndef IPPROTO_MPTCP #define IPPROTO_MPTCP 262 #endif

here?

(or just after the declaration of the function?)

matttbe · 2024-03-21T13:42:12Z

lib/cf-socket.h

@@ -28,6 +28,7 @@
 #include "nonblock.h" /* for curlx_nonblock(), formerly Curl_nonblock() */
 #include "sockaddr.h"

+


unwanted change

Don't forget the auto-review ;)

matttbe · 2024-03-21T13:42:43Z

src/tool_cfgable.h

@@ -298,6 +298,7 @@ struct OperationConfig {
  struct State state;             /* for create_transfer() */
  bool rm_partial;                /* on error, remove partially written output
                                     files */
+  bool mptcp; /* enable MPTCP support */


I don't get their code style here, maybe best to align the comment with the one above?

bool rm_partial; /* on error, remove partially written output files */ bool mptcp; /* enable MPTCP support */

matttbe · 2024-03-21T15:16:51Z

lib/cf-socket.c

@@ -244,6 +244,13 @@ void Curl_sock_assign_addr(struct Curl_sockaddr_ex *dest,
    dest->socktype = SOCK_STREAM;
    dest->protocol = IPPROTO_IP;
    break;
+#ifndef IPPROTO_MPTCP
+#define IPPROTO_MPTCP 262
+#endif


sorry, it was not clear: just below case TRNSPRT_MPTCP:

matttbe · 2024-03-21T15:17:40Z

lib/cf-socket.c

@@ -1602,6 +1609,11 @@ CURLcode Curl_cf_tcp_create(struct Curl_cfilter **pcf,
    result = CURLE_OUT_OF_MEMORY;
    goto out;
  }
+


detail, just to be consistent with the rest of the code: either also add a new line after #endif or no new line here.

matttbe

Just one last batch of modifications needed and then we will be good.

matttbe · 2024-04-04T07:12:18Z

.github/scripts/spellcheck.words

@@ -484,6 +484,7 @@ Monnerat
 monospace
 MorphOS
 MPE
+MPTCP


should be placed after MPL to respect the alphabetical order.

matttbe · 2024-04-04T07:14:37Z

docs/cmdline-opts/mptcp.md

+
+- This feature is currently only supported on Linux starting from kernel 5.6.
+- The server you are connecting to must also support MPTCP.
+  If not, the connection falls back to TCP..


there is an extra dot . at the end here.

While at it, maybe good to add "seamlessly" and this line could be partially moved to the previous one, as long as you keep the 80 char limit:

- The server you are connecting to must also support MPTCP. If not, the connection will seamlessly fall back to TCP.

matttbe · 2024-04-04T07:16:31Z

docs/cmdline-opts/mptcp.md

+
+## Usage
+
+To use MPTCP for your connections, add the `--mptcp` option when using `curl`:


the : at the end suggests something is missing. Maybe you just want to replace it with a .?

matttbe · 2024-04-04T07:20:54Z

.github/scripts/spellcheck.words

@@ -484,6 +484,7 @@ Monnerat
 monospace


for the commit message (not related to this here), I think you can remove:

An HTTP test server indicating whether MPTCP (Multipath TCP) is utilized is now available at the following address: http://test.multipath-tcp.org:5000/.

But place it in the PR description: the service might not be available after the PR, maybe best not to keep this in the Git history.

matttbe · 2024-04-04T07:24:16Z

docs/cmdline-opts/mptcp.md

+Category: connection
+Multi: boolean
+See-also:
+  - tcp-fastopen


can you add in the PR description, something like:

Attention points: - This PR replaces #9713. - Sorry for #13161, I accidentally opened it while the code was not ready yet, my bad. - Regarding the documentation, we didn't find a good reference (`See-also`). We put `tcp-fastopen` because we had to put something, but we are happy to change. - It is not clear for us if a test is worth it. According to your comments from #9713, it might not be worth it. We successfully tested it with http://test.multipath-tcp.org:5000 .

matttbe · 2024-04-04T07:28:05Z

docs/libcurl/opts/CURLOPT_MPTCP.md

+infrastructure. Some networks might drop unknown MPTCP, (...), and its
+effectiveness varies based on the network configuration and conditions.
+If MPTCP is not supported by the network or the end server,
+the connection falls back to TCP.


Same as for the other doc:

the connection will seamlessly fall back to TCP.

matttbe · 2024-04-04T07:34:46Z

lib/cf-socket.c

+
+#ifdef __linux__
+  if(data->set.mptcp)
+    transport = TRNSPRT_MPTCP;


For the "Attention points" of the PR (see my previous comment above), maybe good to add:

- The `transport` is changed to `TRNSPRT_MPTCP` in `Curl_cf_tcp_create()`, not to change the logic before ("happy eyeballs", etc.), and because this feature is an extension to TCP, and it should only modify TCP connections. We are happy to change that if there is a better way.

matttbe · 2024-04-04T07:39:38Z

src/tool_cfgable.c

@@ -45,6 +45,7 @@ void config_init(struct OperationConfig *config)
  config->http09_allowed = FALSE;
  config->ftp_skip_ip = TRUE;
  config->file_clobber_mode = CLOBBER_DEFAULT;
+  config->mptcp = FALSE;


this should not be needed, because of the memset(config, 0, sizeof(struct OperationConfig));

Also, see my comment in struct OperationConfig

matttbe · 2024-04-04T07:41:37Z

src/tool_cfgable.h

@@ -298,6 +298,7 @@ struct OperationConfig {
  struct State state;             /* for create_transfer() */
  bool rm_partial;                /* on error, remove partially written output
                                     files */
+  bool mptcp;                     /* enable MPTCP support */


can you move this a bit above, just between file_clobber_mode; and struct GlobalConfig *global;.

Because struct OperationConfig *next; /* Always last in the struct */ just above

matttbe · 2024-04-04T07:45:19Z

src/tool_cfgable.h

@@ -298,6 +298,7 @@ struct OperationConfig {
  struct State state;             /* for create_transfer() */


for the PR's attention point:

- In the declaration of `struct OperationConfig` in `src/tool_cfgable.h`, the `struct OperationConfig *next;` item has this comment next to it: `/* Always last in the struct */`, but it is no longer the last item in the structure. Is it normal?

matttbe · 2024-04-04T08:39:01Z

docs/libcurl/opts/CURLOPT_MPTCP.md

+infrastructure. Some networks might drop unknown MPTCP, and its
+effectiveness varies based on the network configuration and conditions.
+If MPTCP is not supported by the network or the end server,
+the connection will seamlessly fall back to TCP..


the extra . is back

matttbe

code looks OK to me, good work!

matttbe requested changes Mar 21, 2024

View reviewed changes

CrapsDorian force-pushed the mptcp branch from 9fadd6f to 6d6c28e Compare March 21, 2024 13:31

matttbe requested changes Mar 21, 2024

View reviewed changes

CrapsDorian force-pushed the mptcp branch from 6d6c28e to a643550 Compare March 21, 2024 15:12

matttbe requested changes Mar 21, 2024

View reviewed changes

CrapsDorian force-pushed the mptcp branch from a643550 to 7844ca4 Compare March 21, 2024 15:24

CrapsDorian force-pushed the mptcp branch from 7844ca4 to ad5d447 Compare April 2, 2024 08:58

github-actions bot added cmdline tool libcurl API labels Apr 2, 2024

CrapsDorian force-pushed the mptcp branch from ad5d447 to de187ab Compare April 2, 2024 09:31

CrapsDorian force-pushed the master branch from dc765f2 to 721941a Compare April 2, 2024 09:46

CrapsDorian force-pushed the mptcp branch 3 times, most recently from 8b8e5a2 to a4dc03c Compare April 3, 2024 12:43

github-actions bot added the CI label Apr 3, 2024

CrapsDorian force-pushed the mptcp branch from a4dc03c to 13f6814 Compare April 3, 2024 13:46

matttbe requested changes Apr 4, 2024

View reviewed changes

CrapsDorian force-pushed the mptcp branch from 13f6814 to 25aae39 Compare April 4, 2024 08:29

matttbe requested changes Apr 4, 2024

View reviewed changes

CrapsDorian force-pushed the mptcp branch 3 times, most recently from 7bdb349 to 9a7791f Compare April 4, 2024 09:43

matttbe approved these changes Apr 4, 2024

View reviewed changes

matttbe force-pushed the mptcp branch from 9a7791f to 3596f14 Compare April 5, 2024 16:56

github-actions bot added the tests label Apr 5, 2024

matttbe force-pushed the mptcp branch 2 times, most recently from b9e1917 to 988d7a8 Compare April 8, 2024 10:19

matttbe force-pushed the mptcp branch 3 times, most recently from f8ed5f9 to 0906bf9 Compare April 19, 2024 11:12

CrapsDorian closed this Apr 29, 2024

CrapsDorian force-pushed the mptcp branch from 9dd5667 to 721941a Compare April 29, 2024 15:08


		## Availability

		The `--mptcp` option is available starting from `curl` version 8.6.1.


		## Requirements

		- Your operating system must support MPTCP, and it must be enabled.

		@@ -28,6 +28,7 @@
		#include "nonblock.h" /* for curlx_nonblock(), formerly Curl_nonblock() */
		#include "sockaddr.h"


		## Usage

		To use MPTCP for your connections, add the `--mptcp` option when using `curl`:

		@@ -298,6 +298,7 @@ struct OperationConfig {
		struct State state; /* for create_transfer() */

functional option MPTCP with --mptcp #2

functional option MPTCP with --mptcp #2

Conversation

CrapsDorian commented Mar 21, 2024 • edited

matttbe left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matttbe left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matttbe left a 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

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

Choose a reason for hiding this comment

matttbe left a comment • edited

Choose a reason for hiding this comment

CrapsDorian commented Mar 21, 2024 •

edited

matttbe left a comment •

edited