ISSUE-1619: Implement session flow control using new Proton window API #1647

kgiusti · 2024-10-23T12:58:27Z

No description provided.

ganeshmurthy · 2024-10-23T14:18:25Z

src/adaptors/amqp/container.c

+// amount of outgoing data that can be buffered on a session before control is returned to Proton. This is necessary to
+// improve latency and allow capacity sharing among all links on the session.
+//
+const size_t   qd_session_max_outgoing_bytes = 1048576;   // max buffered bytes on a session


Suggested change

const size_t qd_session_max_outgoing_bytes = 1048576; // max buffered bytes on a session

const size_t qd_session_max_outgoing_bytes = 1048576; // 1 MB is the maximum bytes that can be written to an outgoing session before backing off.

ganeshmurthy · 2024-10-23T14:32:47Z

src/adaptors/amqp/container.c

+// improve latency and allow capacity sharing among all links on the session.
+//
+const size_t   qd_session_max_outgoing_bytes = 1048576;   // max buffered bytes on a session
+const size_t   qd_session_low_outgoing_bytes = 524288;    // low watermark for max buffered bytes


Suggested change

const size_t qd_session_low_outgoing_bytes = 524288; // low watermark for max buffered bytes

const size_t qd_session_low_outgoing_bytes = 524288; // Start writing outgoing bytes to a session only if the session outgoing capacity is greater than 0.5 MB. low watermark for max buffered bytes

ganeshmurthy · 2024-10-23T14:35:34Z

src/adaptors/amqp/container.c

-    size_t  out_window_limit;
-    size_t  out_window_low_watermark;
-};
+const size_t qd_session_max_in_win_user    = (size_t) 8388608;    // AMQP application in window max bytes 8MB


AMQP application

It is not clear to me what AMQP application means

@ganeshmurthy "user connection"? Basically any connection via a listener with the role "normal". These would include connections from the control plane to do management stuff. All non router-2-router connections.

ganeshmurthy · 2024-10-23T14:42:29Z

src/adaptors/amqp/container.c

-    size_t  out_window_low_watermark;
-};
+const size_t qd_session_max_in_win_user    = (size_t) 8388608;    // AMQP application in window max bytes 8MB
+const size_t qd_session_max_in_win_trunk   = (size_t) 134217728;  // inter-router in window max bytes 128MB


Suggested change

const size_t qd_session_max_in_win_trunk = (size_t) 134217728; // inter-router in window max bytes 128MB

const size_t qd_session_max_in_win_trunk = (size_t) 134217728; // per inter-router connection window max byte limit 128MB

@ganeshmurthy but the maximum is not per connection, it is per session, right? Hence the variable prefix "qd_session_..."

ganeshmurthy · 2024-10-23T15:03:48Z

src/policy.c

+    // Assume defaults will be used
+    *in_window = cf->session_max_in_window;
+
+    if (qd_conn->policy_settings) {
+        const qd_policy_spec_t *spec = &qd_conn->policy_settings->spec;
+        if (!spec->outgoingConnection && spec->maxSessionWindow) {
+            // Policy configures the window *in bytes* but Proton uses *frames*.  Convert to frames
+            uint32_t max_frame = spec->maxFrameSize ? spec->maxFrameSize : cf->max_frame_size;
+            *in_window = spec->maxSessionWindow / max_frame;
+            if (*in_window < 2)
+                *in_window = 2;
+        }
    }


Suggested change

// Assume defaults will be used

*in_window = cf->session_max_in_window;

if (qd_conn->policy_settings) {

const qd_policy_spec_t *spec = &qd_conn->policy_settings->spec;

if (!spec->outgoingConnection && spec->maxSessionWindow) {

// Policy configures the window *in bytes* but Proton uses *frames*. Convert to frames

uint32_t max_frame = spec->maxFrameSize ? spec->maxFrameSize : cf->max_frame_size;

*in_window = spec->maxSessionWindow / max_frame;

if (*in_window < 2)

*in_window = 2;

}

}

if (qd_conn->policy_settings) {

const qd_policy_spec_t *spec = &qd_conn->policy_settings->spec;

if (!spec->outgoingConnection && spec->maxSessionWindow) {

// Policy configures the window *in bytes* but Proton uses *frames*. Convert to frames

uint32_t max_frame = spec->maxFrameSize ? spec->maxFrameSize : cf->max_frame_size;

*in_window = spec->maxSessionWindow / max_frame;

if (*in_window < 2)

*in_window = 2;

} else {

*in_window = 2;

}

} else {

// Assume defaults will be used

*in_window = cf->session_max_in_window;

}

@ganeshmurthy I must be missing something but this proposal seems wrong - why would we unconditionally set the in_window to 2 if the connection is incoming or no session max was set? In these cases we definitely want to use the cf->session_max_in_window

ganeshmurthy · 2024-10-23T15:05:45Z

include/qpid/dispatch/amqp_adaptor.h


+// Session windowing limits
+extern const size_t qd_session_max_in_win_user;   // incoming window byte limit for user connections


Suggested change

extern const size_t qd_session_max_in_win_user; // incoming window byte limit for user connections

extern const size_t qd_session_max_incoming_win_user; // incoming window byte limit for user connections

ganeshmurthy · 2024-10-23T15:05:59Z

include/qpid/dispatch/amqp_adaptor.h


+// Session windowing limits
+extern const size_t qd_session_max_in_win_user;   // incoming window byte limit for user connections
+extern const size_t qd_session_max_in_win_trunk;  // incoming window byte limit for inter-router connections


Suggested change

extern const size_t qd_session_max_in_win_trunk; // incoming window byte limit for inter-router connections

extern const size_t qd_session_max_incoming_conn_win; // incoming window byte limit for inter-router connections

ganeshmurthy · 2024-10-23T15:09:01Z

src/adaptors/amqp/container.c

@@ -69,15 +69,34 @@ struct qd_session_t {
    sys_atomic_t    ref_count;
    pn_session_t   *pn_session;
    qd_link_list_t  q3_blocked_links;  ///< Q3 blocked if !empty
+    uint32_t remote_max_frame;


Suggested change

uint32_t remote_max_frame;

uint32_t remote_max_frame_size;

ganeshmurthy · 2024-10-23T15:53:19Z

src/adaptors/amqp/server_config.c

+    // Proton does not support maxSessions > 32768
+    int64_t value = (int64_t) qd_entity_get_long(entity, "maxSessions"); CHECK();
+    if (value == 0) {
+        value = 32768;  // default


Suggested change

value = 32768; // default

value = QD_PN_MAX_SESSIONS; // default

define QD_PN_MAX_SESSIONS in amqp.h ?

ganeshmurthy · 2024-10-23T15:55:30Z

src/adaptors/amqp/server_config.c

+    // and it does not exceed the proton APIs max of INT32_MAX
+    value = (int64_t) qd_entity_get_long(entity, "maxFrameSize"); CHECK();
+    if (value == 0) {
+        value = 16384; // default


Suggested change

value = 16384; // default

value = QD_DEFAULT_MAX_FRAME_SIZE; // default

ganeshmurthy · 2024-10-23T15:56:51Z

src/adaptors/amqp/server_config.h

     */
-    size_t incoming_capacity;
+    uint32_t session_max_in_window;


Suggested change

uint32_t session_max_in_window;

uint32_t session_max_incoming_window;

src/adaptors/amqp/container.c

+
+    // if the remotes max window is smaller than the default outgoing bytes limit then adjust the limits down
+    // otherwise we may never resume sending on blocked links (stall) since the low limit will never be exceeded.
+    size_t window_bytes = in_window * qd_ssn->remote_max_frame;


kgiusti requested review from ted-ross and ganeshmurthy October 23, 2024 12:58

ganeshmurthy reviewed Oct 23, 2024

View reviewed changes

kgiusti force-pushed the ISSUE-1619-2 branch from 44e8532 to 5907d68 Compare November 1, 2024 16:19

kgiusti added 2 commits November 12, 2024 16:37

ISSUE-1619: Implement session flow control using new Proton window API

1ed963e

fixup: adjust limits based on peer window, review input

bb9546e

kgiusti force-pushed the ISSUE-1619-2 branch from 5907d68 to bb9546e Compare November 12, 2024 21:37

github-advanced-security bot found potential problems Nov 12, 2024

View reviewed changes

src/adaptors/amqp/container.c Fixed Show fixed Hide fixed

fixup: fix 0.39.0 compatibility, add assert() check

1c9b081

github-advanced-security bot found potential problems Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE-1619: Implement session flow control using new Proton window API #1647

ISSUE-1619: Implement session flow control using new Proton window API #1647

kgiusti commented Oct 23, 2024

ganeshmurthy Oct 23, 2024 •

edited

Loading

ganeshmurthy Oct 23, 2024 •

edited

Loading

ganeshmurthy Oct 23, 2024

kgiusti Oct 28, 2024

ganeshmurthy Oct 23, 2024

kgiusti Oct 28, 2024

ganeshmurthy Oct 23, 2024

kgiusti Oct 28, 2024 •

edited

Loading

ganeshmurthy Oct 23, 2024

ganeshmurthy Oct 23, 2024 •

edited

Loading

ganeshmurthy Oct 23, 2024

ganeshmurthy Oct 23, 2024

ganeshmurthy Oct 23, 2024

ganeshmurthy Oct 23, 2024

	const size_t qd_session_max_outgoing_bytes = 1048576; // max buffered bytes on a session
	const size_t qd_session_max_outgoing_bytes = 1048576; // 1 MB is the maximum bytes that can be written to an outgoing session before backing off.

	const size_t qd_session_low_outgoing_bytes = 524288; // low watermark for max buffered bytes
	const size_t qd_session_low_outgoing_bytes = 524288; // Start writing outgoing bytes to a session only if the session outgoing capacity is greater than 0.5 MB. low watermark for max buffered bytes

	const size_t qd_session_max_in_win_trunk = (size_t) 134217728; // inter-router in window max bytes 128MB
	const size_t qd_session_max_in_win_trunk = (size_t) 134217728; // per inter-router connection window max byte limit 128MB


		// Session windowing limits
		extern const size_t qd_session_max_in_win_user; // incoming window byte limit for user connections

	extern const size_t qd_session_max_in_win_user; // incoming window byte limit for user connections
	extern const size_t qd_session_max_incoming_win_user; // incoming window byte limit for user connections

	value = 32768; // default
	value = QD_PN_MAX_SESSIONS; // default

	value = 16384; // default
	value = QD_DEFAULT_MAX_FRAME_SIZE; // default

	uint32_t session_max_in_window;
	uint32_t session_max_incoming_window;

ISSUE-1619: Implement session flow control using new Proton window API #1647

Are you sure you want to change the base?

ISSUE-1619: Implement session flow control using new Proton window API #1647

Conversation

kgiusti commented Oct 23, 2024

ganeshmurthy Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

ganeshmurthy Oct 23, 2024 • edited Loading

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

kgiusti Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganeshmurthy Oct 23, 2024 • edited Loading

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

ganeshmurthy Oct 23, 2024 •

edited

Loading

ganeshmurthy Oct 23, 2024 •

edited

Loading

kgiusti Oct 28, 2024 •

edited

Loading

ganeshmurthy Oct 23, 2024 •

edited

Loading