Add support for setting TypeOfService/TrafficClass on connections #4757

anrossi · 2025-01-21T04:19:47Z

Description

Resolves #4767. Add support for applications to set the Type of Service/Traffic Class field all traffic sent from a connection.

Testing

TBD

Documentation

Updated the settings document with the new get/set param.

src/platform/datapath_winkernel.c

anrossi · 2025-01-25T22:13:50Z

src/core/connection.c

+        Status = QUIC_STATUS_SUCCESS;
+        break;
+
+     case QUIC_PARAM_CONN_DSCP:


Should GetParam return the last-set DSCP value, or the most recently received DSCP value?
Or should getting the received DSCP value be a separate, get-only param?

It's probably better to support querying the last value set by the app. So, then we would need two parameters, one for send and one (get-only) for receive. But let's save receive for a future PR?

Sure, receive can be a future PR

anrossi · 2025-01-26T01:16:38Z

src/platform/datapath_winkernel.c

+        CMsg = (PWSACMSGHDR)&CMsgBuffer[CMsgLen];
+        CMsgLen += WSA_CMSG_SPACE(sizeof(INT));
+        CMsg->cmsg_level =
+            Route->LocalAddress.si_family == QUIC_ADDRESS_FAMILY_INET ?


LocalAddress might be the wrong address to use; if trying to send to an IPv4 address using a dual-mode socket, you need to set the IPv4 IP_TOS, instead of the IPV6_TCLASS

nibanks · 2025-01-27T12:50:02Z

src/core/binding.c

@@ -803,7 +803,7 @@ QuicBindingProcessStatelessOperation(
        Binding,
        OperationType);

-    CXPLAT_SEND_CONFIG SendConfig = { RecvPacket->Route, 0, CXPLAT_ECN_NON_ECT, 0 };
+    CXPLAT_SEND_CONFIG SendConfig = { RecvPacket->Route, 0, CXPLAT_ECN_NON_ECT, 0, 0 };


Should we have a #define for zero here instead of using a magic number?

nibanks · 2025-01-27T12:50:23Z

src/core/connection.c

+    case QUIC_PARAM_CONN_DSCP:
+    {


Suggested change

case QUIC_PARAM_CONN_DSCP:

{

case QUIC_PARAM_CONN_DSCP: {

nibanks · 2025-01-27T12:51:12Z

src/core/connection.c

+        uint8_t DSCP = 0;
+        CxPlatCopyMemory(&DSCP, Buffer, BufferLength);


Simplify?

Suggested change

uint8_t DSCP = 0;

CxPlatCopyMemory(&DSCP, Buffer, BufferLength);

uint8_t DSCP = *Buffer;

Won't the simplification cause a crash if the buffer is unaligned on ARM?

nibanks · 2025-01-27T12:51:52Z

src/core/connection.c

@@ -7207,27 +7228,55 @@ QuicConnParamGet(
    }

    case QUIC_PARAM_CONN_ORIG_DEST_CID:
+


nit: don't modify code that's not relevant to this PR?

nibanks · 2025-01-27T12:54:29Z

src/inc/msquic.h

@@ -923,6 +923,7 @@ typedef struct QUIC_SCHANNEL_CREDENTIAL_ATTRIBUTE_W {
 #define QUIC_PARAM_CONN_STATISTICS_V2                   0x05000016  // QUIC_STATISTICS_V2
 #define QUIC_PARAM_CONN_STATISTICS_V2_PLAT              0x05000017  // QUIC_STATISTICS_V2
 #define QUIC_PARAM_CONN_ORIG_DEST_CID                   0x05000018  // uint8_t[]
+#define QUIC_PARAM_CONN_DSCP                            0x50000019  // uint8_t


Maybe call it _SEND_DSCP?

Suggested change

#define QUIC_PARAM_CONN_DSCP 0x50000019 // uint8_t

#define QUIC_PARAM_CONN_SEND_DSCP 0x50000019 // uint8_t

I was thinking of the same pattern that socket options use where the send one has no prefix, but the receive one has a prefix. So when we do the last-received DSCP value, it would be QUIC_PARAM_CONN_RECV_DSCP

nibanks · 2025-01-27T12:55:51Z

src/inc/quic_datapath.h

@@ -444,6 +455,7 @@ CxPlatDataPathUpdateConfig(
 #define CXPLAT_DATAPATH_FEATURE_TCP                   0x0020
 #define CXPLAT_DATAPATH_FEATURE_RAW                   0x0040
 #define CXPLAT_DATAPATH_FEATURE_TTL                   0x0080
+#define CXPLAT_DATAPATH_FEATURE_DSCP                  0x0100


Will we need separate features for send/receive support?

I think the receive support already exists in all stacks, it's just the send support that's missing on some.

anrossi · 2025-01-27T19:34:47Z

src/platform/datapath_winuser.c

@@ -1672,7 +1708,7 @@ SocketCreateUdp(
            setsockopt(
                SocketProc->Socket,
                IPPROTO_IP,
-                IP_ECN,
+                IP_RECVTOS,


I think this might be causing test failures because packets aren't getting received in some tests since adding this change.

anrossi · 2025-01-27T19:35:08Z

src/platform/datapath_winuser.c

-                } else if (CMsg->cmsg_type == IP_ECN) {
-                    ECN = *(PINT)WSA_CMSG_DATA(CMsg);
-                    CXPLAT_DBG_ASSERT(ECN < UINT8_MAX);
+                } else if (CMsg->cmsg_type == IP_TOS) {


Same here; I think this change might be causing receives to fail.

Add support for setting TypeOfService/TrafficClass on connections

02c82b2

anrossi added OS: Windows (User) OS: Ubuntu OS: Windows (Kernel) Area: API labels Jan 21, 2025

anrossi requested a review from a team as a code owner January 21, 2025 04:19

Update generated files

35949c1

nibanks reviewed Jan 21, 2025

View reviewed changes

src/platform/datapath_winkernel.c Outdated Show resolved Hide resolved

Switch to Ancillary Data

6bc1acc

anrossi commented Jan 25, 2025

View reviewed changes

anrossi commented Jan 26, 2025

View reviewed changes

anrossi added 3 commits January 25, 2025 17:39

Zero out new DSCP struct member and see if tests pass

de34f35

Add tests for Dscp send support.

fe5a6f3

Fix failing tests, and collect ToS/TClass on Windows for validation

35eff60

nibanks reviewed Jan 27, 2025

View reviewed changes

anrossi commented Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for setting TypeOfService/TrafficClass on connections #4757

Add support for setting TypeOfService/TrafficClass on connections #4757

anrossi commented Jan 21, 2025 •

edited by nibanks

Loading

anrossi Jan 25, 2025

nibanks Jan 27, 2025

anrossi Jan 30, 2025

anrossi Jan 26, 2025

nibanks Jan 27, 2025

nibanks Jan 27, 2025

nibanks Jan 27, 2025

anrossi Jan 27, 2025

nibanks Jan 27, 2025

nibanks Jan 27, 2025

anrossi Jan 27, 2025

nibanks Jan 27, 2025

anrossi Jan 27, 2025

anrossi Jan 27, 2025

anrossi Jan 27, 2025

		uint8_t DSCP = 0;
		CxPlatCopyMemory(&DSCP, Buffer, BufferLength);

	uint8_t DSCP = 0;
	CxPlatCopyMemory(&DSCP, Buffer, BufferLength);
	uint8_t DSCP = *Buffer;

		@@ -7207,27 +7228,55 @@ QuicConnParamGet(
		}

		case QUIC_PARAM_CONN_ORIG_DEST_CID:

	#define QUIC_PARAM_CONN_DSCP 0x50000019 // uint8_t
	#define QUIC_PARAM_CONN_SEND_DSCP 0x50000019 // uint8_t

Add support for setting TypeOfService/TrafficClass on connections #4757

Are you sure you want to change the base?

Add support for setting TypeOfService/TrafficClass on connections #4757

Conversation

anrossi commented Jan 21, 2025 • edited by nibanks Loading

Description

Testing

Documentation

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

Choose a reason for hiding this comment

anrossi commented Jan 21, 2025 •

edited by nibanks

Loading