QUIC: Add support to configure UDP max payload limit. #9486

brbzull0 · 2023-03-03T15:20:32Z

This commit also makes this configuration available for the quiche quic implementation.

brbzull0 · 2023-03-03T16:15:12Z

[approve ci autest]

maskit · 2023-03-03T16:39:30Z

I just found that max_udp_payload_size is actually a Transport Parameter of QUIC. It may be more appropriate to have the setting under ts.config.quic. And if so, we should have ones with _in and _out in the names to support outgoing QUIC connection in the future.

max_udp_payload_size (0x03):
The maximum UDP payload size parameter is an integer value that limits the size of UDP payloads that the endpoint is willing to receive. UDP datagrams with payloads larger than this limit are not likely to be processed by the receiver.¶

The default for this parameter is the maximum permitted UDP payload of 65527. Values below 1200 are invalid.¶

brbzull0 · 2023-03-06T11:44:47Z

I just found that max_udp_payload_size is actually a Transport Parameter of QUIC. It may be more appropriate to have the setting under ts.config.quic. And if so, we should have ones with _in and _out in the names to support outgoing QUIC connection in the future.

max_udp_payload_size (0x03):
The maximum UDP payload size parameter is an integer value that limits the size of UDP payloads that the endpoint is willing to receive. UDP datagrams with payloads larger than this limit are not likely to be processed by the receiver.¶

The default for this parameter is the maximum permitted UDP payload of 65527. Values below 1200 are invalid.¶

Ok, I saw that on the spcecs,

So,

max_udp_payload_size_in|out should be part of the ts.quic block, but max_udp_send_payload_size is not a transport param but rather a generic UDP, so the later should be part of the ts.udp , now as we set this on quiche, do we still need the in|out for the send_payload_size?

maskit · 2023-03-06T20:21:34Z

Ah, Quiche API uses similar names for send and recv, but only the one for recv is a transport parameter.

We should probably have the both recv and send under ts.quic because I think we need in|out for the both. in|out are for inbound|outbound connections, so having it under ts.udp doesn't make sense.

We could also have ts.udp.send_payload_size without in|out, but that'd be complicated. Either ts.udp.send_payload_size or ts.quic.max_udp_send_payload_size_in|out would have to override the other.

brbzull0 · 2023-03-07T16:16:22Z

Ah, Quiche API uses similar names for send and recv, but only the one for recv is a transport parameter.

We should probably have the both recv and send under ts.quic because I think we need in|out for the both. in|out are for inbound|outbound connections, so having it under ts.udp doesn't make sense.

We could also have ts.udp.send_payload_size without in|out, but that'd be complicated. Either ts.udp.send_payload_size or ts.quic.max_udp_send_payload_size_in|out would have to override the other.

grand, will update this PR then.

brbzull0 · 2023-03-07T17:26:59Z

[approve ci autest]

ywkaras · 2023-03-09T02:18:01Z

doc/admin-guide/files/records.yaml.en.rst

+
+.. ts:cv:: CONFIG proxy.config.quic.max_recv_udp_payload_size_out INT 65527
+
+   This value will be advertised as ``max_udp_payload_size`` Transport Parameter.


Two configs with exactly the same description.

Yes, they reference to the same transport parameter. I think this eventually will contain more details, or even a link to the current rfc definition.

ywkaras · 2023-03-09T02:19:07Z

doc/admin-guide/files/records.yaml.en.rst

+
+.. ts:cv:: CONFIG proxy.config.quic.max_send_udp_payload_size_out INT 65527
+
+   Specified the maximum outgoing UDP payload size.


ywkaras · 2023-03-09T02:20:03Z

iocore/net/QUICNetProcessor_quiche.cc

-  quiche_config_set_max_recv_udp_payload_size(this->_quiche_config, 16384);
-  quiche_config_set_max_send_udp_payload_size(this->_quiche_config, 16384);
+  quiche_config_set_max_recv_udp_payload_size(this->_quiche_config, params->get_max_recv_udp_payload_size_in());
+  quiche_config_set_max_send_udp_payload_size(this->_quiche_config, params->get_max_send_udp_payload_size_in());


Seems a bit odd that the default value for these config variables is (about) double the previous hard-coded value.

It seems to be the default for this transport parameter.

The default for this parameter is the maximum permitted UDP payload of 65527. Values below 1200 are invalid.well, being hardcoded before means nothing different

The hard-coded value means nothing. It could be 12000 or 32768 if I made the commit on a different day.

And _in is correct for the both here. It's for inbound connections. The _out will be used when we support H3 to origin.

From the names of the the Quiche API functions, it doesn't look like you can set different max packet sizes when the near end is the server versus the client.

Although MTU always limits packet sizes on both direction, what these settings mean are:
recv: Tell the peer "Don't send me packets bigger than this size"
send: Think quietly "I'm going to send this size of packets at maximum if the peer allows"

Yes, that's what I thought. But I don't see how Quiche is allowing us to configure those values differently depending on whether the near end is the client or the server. If we can't, we only need two new config variables, not four.

Oh you mean different packet sizes for inbound connection and outbound connection? We'll need another quiche_config object for outbound connections. That is why we use _in settings here. We'll have similar code that use _out settings somewhere else in the future.

maskit · 2023-03-09T15:40:09Z

src/records/RecordsConfig.cc

  ,
  {RECT_CONFIG, "proxy.config.udp.enable_gso", RECD_INT, "1", RECU_NULL, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
  ,
+  {RECT_CONFIG, "proxy.config.udp.max_recv_payload_size", RECD_INT, "65527", RECU_DYNAMIC, RR_NULL, RECC_NULL, "^[0-9]+$", RECA_NULL}


We are going to have proxy.config.quic.max_recv_udp_payload_size_in|out, right?

yes, my bad, made a mess with names and all that stuff. Will fix.

brbzull0 · 2023-03-09T17:14:45Z

Made a mess with my branches, this is missing the actual records. Will fix. Sorry again guys.

Fixed

maskit

Thanks!

* commit 'c54a2e2b77151869ff014fbdc4c82cec0afcbb8c': (37 commits) Slight performance improvements before calling APIHooks::clear (apache#9480) libswoc: Update to 1.4.5 (apache#9522) CryptoContext: Clean up to avoid compiler problem. (apache#9521) Add TLSCertSwitchSupport (apache#9322) Add clang-format-tests to clang-format target (apache#9456) Adds the AR env variable to config.nice (apache#9515) Fix .asf.yaml (apache#9519) Hugepage config cleanup (apache#9479) Separate io_uring into a separate library. AIO in io_uring mode uses new io_uring lib. (apache#9462) Avoid memory allocation in CryptoHash (apache#9474) UnitParser: add unit parser support. (apache#9485) autest - Minor fix on the verifier_client test ext to allow setting only the http3 ports. (apache#9517) Remove support for port event polling (apache#9476) QUIC: Add support to configure UDP max payload limit. (apache#9486) Reduce the size of the APIHooks, eliminating enum gap (apache#9509) Add support for CMCD-Request header nor field to prefetch plugin (apache#9232) Eliminates padding from some common structs (apache#9481) Enable external file loading for sni.yaml. (apache#9501) Remove inactive include of IpMapConf.h (apache#9512) Cleanup: Remove RecModeT from the code. (apache#9487) ...

brbzull0 added Configuration QUIC UDP labels Mar 3, 2023

brbzull0 added this to the 10.0.0 milestone Mar 3, 2023

brbzull0 self-assigned this Mar 3, 2023

bryancall requested a review from ywkaras March 6, 2023 23:24

brbzull0 closed this Mar 7, 2023

brbzull0 reopened this Mar 7, 2023

brbzull0 force-pushed the quic_max_payload_size_conf branch from f9fbf4d to f300ba0 Compare March 7, 2023 16:42

zwoop requested a review from maskit March 8, 2023 23:35

ywkaras reviewed Mar 9, 2023

View reviewed changes

maskit reviewed Mar 9, 2023

View reviewed changes

QUIC: Add support to configure UDP max payload limit.

37c2788

brbzull0 force-pushed the quic_max_payload_size_conf branch from f300ba0 to 37c2788 Compare March 9, 2023 18:00

brbzull0 requested a review from maskit March 10, 2023 23:07

maskit approved these changes Mar 11, 2023

View reviewed changes

brbzull0 merged commit bdb802b into apache:master Mar 13, 2023

brbzull0 mentioned this pull request Apr 3, 2023

MAX_UDP_PAYLOAD_SIZE is not configurable #6823

Closed

cmcfarlen pushed a commit to cmcfarlen/trafficserver that referenced this pull request Jun 3, 2024

QUIC: Add support to configure UDP max payload limit. (apache#9486)

1e0e71e


		.. ts:cv:: CONFIG proxy.config.quic.max_recv_udp_payload_size_out INT 65527

		This value will be advertised as ``max_udp_payload_size`` Transport Parameter.


		.. ts:cv:: CONFIG proxy.config.quic.max_send_udp_payload_size_out INT 65527

		Specified the maximum outgoing UDP payload size.

QUIC: Add support to configure UDP max payload limit. #9486

QUIC: Add support to configure UDP max payload limit. #9486

Uh oh!

Conversation

brbzull0 commented Mar 3, 2023

Uh oh!

brbzull0 commented Mar 3, 2023

Uh oh!

maskit commented Mar 3, 2023

Uh oh!

brbzull0 commented Mar 6, 2023

Uh oh!

maskit commented Mar 6, 2023

Uh oh!

brbzull0 commented Mar 7, 2023

Uh oh!

brbzull0 commented Mar 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brbzull0 commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maskit left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brbzull0 commented Mar 9, 2023 •

edited

Loading