You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/09/10 23:31:29 UTC
[GitHub] [trafficserver] bneradt opened a new pull request, #9085: Make HTTP/2 session and stream windows configurable
bneradt opened a new pull request, #9085:
URL: https://github.com/apache/trafficserver/pull/9085
Issue #8199 outlines the issue. This PR adds a new setting to allow for setting the session window separately from the stream window.
Closes #8199
---
This takes over for PR #8203 since that now has conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1245584353
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#discussion_r998625086
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1427,31 +1477,65 @@ Http2ConnectionState::restart_streams()
void
Http2ConnectionState::restart_receiving(Http2Stream *stream)
{
- uint32_t initial_rwnd = this->local_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE);
- uint32_t min_rwnd = std::min(initial_rwnd, this->local_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE));
-
// Connection level WINDOW UPDATE
- if (this->server_rwnd() < min_rwnd) {
- Http2WindowSize diff_size = initial_rwnd - this->server_rwnd();
- this->increment_server_rwnd(diff_size);
- this->_server_rwnd_is_shrinking = false;
- this->send_window_update_frame(0, diff_size);
+ uint32_t const configured_session_window = this->_get_configured_receive_session_window_size_in();
+ uint32_t const min_session_window = std::min(configured_session_window, this->local_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE));
+ if (this->server_rwnd() < min_session_window) {
+ Http2WindowSize diff_size = configured_session_window - this->server_rwnd();
+ if (diff_size > 0) {
+ this->increment_server_rwnd(diff_size);
+ this->_server_rwnd_is_shrinking = false;
+ this->send_window_update_frame(HTTP2_CONNECTION_CONTROL_STREAM, diff_size);
+ }
}
// Stream level WINDOW UPDATE
- if (stream == nullptr || stream->server_rwnd() >= min_rwnd) {
+ if (stream == nullptr || stream->server_rwnd() >= min_session_window) {
+ // There's no need to increase the stream window size if it is already big
+ // enough to hold what the stream/max frame size can receive.
return;
}
+ uint32_t initial_stream_window = this->local_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE);
+ if (this->_has_dynamic_stream_window()) {
+ // If our stream window size is dynamic, then we have to allow for this situation:
+ //
+ // 1. ATS initializes the stream window size to the configured value via a
+ // SETTINGS frame, say 10 bytes.
+ //
+ // 2. The peer sends a data frame of that size, again 10 bytes.
+ //
+ // 3. At the same time that the peer sends the data frame, we shrink the
+ // stream window size via a SETTINGS frame to a smaller value, say 5 bytes.
+ //
+ // 4. The peer receives the SETTINGS frame and calculates its send window
+ // to be -5 bytes.
+ //
+ // 5. ATS cannot simply send a WINDOW_UPDATE frame of 5 bytes to bring the
Review Comment:
We discussed this offline together, but just to record it here: the RFC states that existing stream window sizes (as well as new ones) do get adjusted per SETTINGS frames that configure SETTINGS_INITIAL_WINDOW_SIZE:
https://www.rfc-editor.org/rfc/rfc9113.html#name-initial-flow-control-window
> In addition to changing the flow-control window for streams that are not yet active, a [SETTINGS](https://www.rfc-editor.org/rfc/rfc9113.html#SETTINGS) frame can alter the initial flow-control window size for streams with active flow-control windows (that is, streams in the “open” or “half-closed (remote)” state). When the value of [SETTINGS_INITIAL_WINDOW_SIZE](https://www.rfc-editor.org/rfc/rfc9113.html#SETTINGS_INITIAL_WINDOW_SIZE) changes, a receiver MUST adjust the size of all stream flow-control windows that it maintains by the difference between the new value and the old value.[¶](https://www.rfc-editor.org/rfc/rfc9113.html#section-6.9.2-3)
None of this is a problem: the math actually works out pretty seamlessly for negative receive windows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#discussion_r998620382
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -4152,7 +4152,40 @@ HTTP/2 Configuration
:reloadable:
:units: bytes
- The initial window size for inbound connections.
+ The initial HTTP/2 stream window size for inbound connections that |TS| as a
+ receiver advertises to the peer. See IETF RFC 9113 section 5.2 for details
+ concerning HTTP/2 flow control. See
+ :ts:cv:`proxy.config.http2.flow_control.in.policy` for how HTTP/2 stream and
+ session windows are maintained over the lifetime of HTTP/2 connections.
Review Comment:
Sounds good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#discussion_r996539311
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -4152,7 +4152,40 @@ HTTP/2 Configuration
:reloadable:
:units: bytes
- The initial window size for inbound connections.
+ The initial HTTP/2 stream window size for inbound connections that |TS| as a
+ receiver advertises to the peer. See IETF RFC 9113 section 5.2 for details
+ concerning HTTP/2 flow control. See
+ :ts:cv:`proxy.config.http2.flow_control.in.policy` for how HTTP/2 stream and
+ session windows are maintained over the lifetime of HTTP/2 connections.
Review Comment:
Let's not mix terms in the spec and terms in ATS. It should be Connection-Stream, or Session-Transaction.
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1572,12 +1656,23 @@ Http2ConnectionState::release_stream()
}
void
-Http2ConnectionState::update_initial_rwnd(Http2WindowSize new_size)
+Http2ConnectionState::update_initial_client_rwnd(Http2WindowSize new_size)
Review Comment:
How about `update_initial_peer_rwnd` ? We forgot to rename `_client_rwnd` and `_server_rwnd` on #9084.
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1427,31 +1477,65 @@ Http2ConnectionState::restart_streams()
void
Http2ConnectionState::restart_receiving(Http2Stream *stream)
{
- uint32_t initial_rwnd = this->local_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE);
- uint32_t min_rwnd = std::min(initial_rwnd, this->local_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE));
-
// Connection level WINDOW UPDATE
- if (this->server_rwnd() < min_rwnd) {
- Http2WindowSize diff_size = initial_rwnd - this->server_rwnd();
- this->increment_server_rwnd(diff_size);
- this->_server_rwnd_is_shrinking = false;
- this->send_window_update_frame(0, diff_size);
+ uint32_t const configured_session_window = this->_get_configured_receive_session_window_size_in();
+ uint32_t const min_session_window = std::min(configured_session_window, this->local_settings.get(HTTP2_SETTINGS_MAX_FRAME_SIZE));
+ if (this->server_rwnd() < min_session_window) {
+ Http2WindowSize diff_size = configured_session_window - this->server_rwnd();
+ if (diff_size > 0) {
+ this->increment_server_rwnd(diff_size);
+ this->_server_rwnd_is_shrinking = false;
+ this->send_window_update_frame(HTTP2_CONNECTION_CONTROL_STREAM, diff_size);
+ }
}
// Stream level WINDOW UPDATE
- if (stream == nullptr || stream->server_rwnd() >= min_rwnd) {
+ if (stream == nullptr || stream->server_rwnd() >= min_session_window) {
+ // There's no need to increase the stream window size if it is already big
+ // enough to hold what the stream/max frame size can receive.
return;
}
+ uint32_t initial_stream_window = this->local_settings.get(HTTP2_SETTINGS_INITIAL_WINDOW_SIZE);
+ if (this->_has_dynamic_stream_window()) {
+ // If our stream window size is dynamic, then we have to allow for this situation:
+ //
+ // 1. ATS initializes the stream window size to the configured value via a
+ // SETTINGS frame, say 10 bytes.
+ //
+ // 2. The peer sends a data frame of that size, again 10 bytes.
+ //
+ // 3. At the same time that the peer sends the data frame, we shrink the
+ // stream window size via a SETTINGS frame to a smaller value, say 5 bytes.
+ //
+ // 4. The peer receives the SETTINGS frame and calculates its send window
+ // to be -5 bytes.
+ //
+ // 5. ATS cannot simply send a WINDOW_UPDATE frame of 5 bytes to bring the
Review Comment:
ATS gave 10 at the beginning and the peer already consumed 10. If ATS has already processed the 10 bytes received, we should be able to give 5. If ATS has not processed the 10 bytes, there's nothing to do here yet. SETTINGS frame doesn't affect any existing streams on the peer side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#discussion_r999597383
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -4152,7 +4152,40 @@ HTTP/2 Configuration
:reloadable:
:units: bytes
- The initial window size for inbound connections.
+ The initial HTTP/2 stream window size for inbound connections that |TS| as a
+ receiver advertises to the peer. See IETF RFC 9113 section 5.2 for details
+ concerning HTTP/2 flow control. See
+ :ts:cv:`proxy.config.http2.flow_control.policy.in` for how HTTP/2 stream and
Review Comment:
Makes sense. Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1245783898
Thank you for the ideas @maskit.
> I don't understand why adjusting proxy.config.http2.initial_window_size_in and proxy.config.http2.max_concurrent_streams_in isn't enough, if we use them to calculate the connection window size. You just need to do a math. If we had a setting for connection window size, how those three would be used?
To make sure I understand you correctly: you prefer that this patch set the window (session) size to the product of `max_concurrent_streams_in` by `initial_window_size_in` (the stream window size)? That is, you would prefer that over adding a separate configuration to set the window size? I could potentially be persuaded to start by just setting the session window size to the product of these two values for now.
I think @SolidWallOfCode pointed out that we might have issues with origin sessions with many streams eating up more bandwidth than we like. I believe his suggestion was to add a configurable multiplier. What are your thoughts on a configurable multiplier?
> Let's say we have, initial_connection_window_size=100,000, initial_stream_window_size=1,000 and max_concurrent_streams=10, what's your expectation? You never reach the connection window size. If it never reach the connection window size set, what's the point? It's the same as setting it to 1PB. It limits nothing. This is why I don't want the setting for connection window size.
>
> Similarly, initial_connection_window_size=100,000, initial_stream_window_size=10,000 and max_concurrent_streams=100, without dynamic size change, first 10 streams can dominate the connection window and nothing balances the unfair connection window use. This is why I think we need dynamic adjustment mechanism.
These are good points, but I guess I'm personally not too concerned about this. The knobs should be pretty clear concerning what they mean. We already have a configuration that sets the stream size, so it doesn't seem out of place to me to have the ability to set the session window size. Given your thoughts, though, it would be good to add documentation to highlight how these three configurations can relate to each other. Having the separate session window size configuration is still my preference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1258556432
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1260027733
The `http2_flow_control` autest will fail until I get a new release of Proxy Verifier created and merged into master/10-Dev. The new Proxy Verifier adds `WINDOW_UPDATE` and `SETTINGS` frame logging that the test depends upon.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1249800411
> But it still solves only half of my concern. How do you fully utilize a connection (or its connection window) by unknown number of streams? I'm asking this question again and again, but nobody has told me any solution. It hasn't been an issue because the both window sizes are the same. One stream could occupy the entire connection window. Now, with the new setting, session window size is bigger than stream window size. A connection that has just one stream would be unreasonably limited. You don't care about it?
This is an important concern, and I appreciate you raising it again. But I don't think that has been ignored. Maybe it feels like it has been ignored because we haven't explicitly addressed it recently, but it has been discussed at some length in #8199. As I understand the discussion there, the conclusion was that the agreed upon best behavior would be to have ATS tune the stream window sizes as the number of streams either increases or decreases. This way the streams can utilize an appropriate amount of the connection window without starving other streams. That is, one stream by itself in a session can use the whole session window, while twenty streams sharing a session can use about 1/20 of the session window each. But given the complexity of implementing that dynamic behavior, I believe our conclusion was to move forward with this separate explicit and static session and stream window configuration.
To be clear, without some change, HTTP/2 to origin will be useless because Susan observed that as things currently stand, a few greedy streams can starve the other sibling streams.
I'd be content with either of these solutions:
* For now, simply set the session window size to the product of the stream window size and the max streams size, or
* for now, add the separate static session window size configuration (i.e., what is implemented in this patch).
It seems to me that long-term we all agree that we want a session window size configuration. Am I correct in understanding that? That is, if the stream window size will be dynamic, then users will generally prefer to set the session window size to some value and leave the stream window size to be dynamic based upon the number of streams. In which case the stream window size will probably support a `-1` value (as you suggested above for the session window size) which means it will be dynamic. If that's the future, then we might as well add a session window size configuration now. If I'm wrong about this however, and we're not sure we want a session window, then we might as well choose the first option (have no session window configuration and just have it be the produce of the stream window size and the max streams configuration).
Thank you for continuing this discussion, @maskit. I want to end up committing what we are all content with.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#discussion_r998626563
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1572,12 +1656,23 @@ Http2ConnectionState::release_stream()
}
void
-Http2ConnectionState::update_initial_rwnd(Http2WindowSize new_size)
+Http2ConnectionState::update_initial_client_rwnd(Http2WindowSize new_size)
Review Comment:
Yes. I noticed this too. Let me do more renaming corrections after this PR to keep it more manageable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1244781498
> One benefit of having two district settings is if there are in fact numerous concurrent streams, it is unlikely that one will suck up all the resources before the other streams can get started.
I don't understand why adjusting `proxy.config.http2.initial_window_size_in` and ` proxy.config.http2.max_concurrent_streams_in` isn't enough, if we use them to calculate the connection window size. You just need to do a math. If we had a setting for connection window size, how those three would be used?
Let's say we have, `initial_connection_window_size=100,000`, `initial_stream_window_size=1,000` and `max_concurrent_streams=10`, what's your expectation? You never reach the connection window size. If it never reach the connection window size set, what's the point? It's the same as setting it to 1PB. It limits nothing. This is why I don't want the setting for connection window size.
Similarly, `initial_connection_window_size=100,000`, `initial_stream_window_size=10,000` and `max_concurrent_streams=100`, without dynamic size change, first 10 streams can dominate the connection window and nothing balance the unfair connection window use. This is why I think we need dynamic adjustment mechanism.
If you want to keep initial stream window size small at the beginning and make it bigger in the middle of transfer for some reasons, having a separate setting for client window size may make sense because `stream-window-size * max-concurrent-streams` would be too small. But I don't see any reasons to do that.
> I would be concerned that fiddling the setting that frequently would mess up the the peer.
It might depends on the peer implementation, but I don't think it requires a lot of tasks nor heavy tasks to the peer. If the frequent update is going to be a problem, we can use the average number of streams of the calculation.
And nobody has suggested any other solutions for connection utilization issue on a connection that has only one stream.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#discussion_r998906518
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -4152,7 +4152,40 @@ HTTP/2 Configuration
:reloadable:
:units: bytes
- The initial window size for inbound connections.
+ The initial HTTP/2 stream window size for inbound connections that |TS| as a
+ receiver advertises to the peer. See IETF RFC 9113 section 5.2 for details
+ concerning HTTP/2 flow control. See
+ :ts:cv:`proxy.config.http2.flow_control.in.policy` for how HTTP/2 stream and
Review Comment:
Good observation. Let's keep it at the end since that's what we usually do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a diff in pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on code in PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#discussion_r999826678
##########
proxy/http2/Http2ConnectionState.cc:
##########
@@ -1572,12 +1656,23 @@ Http2ConnectionState::release_stream()
}
void
-Http2ConnectionState::update_initial_rwnd(Http2WindowSize new_size)
+Http2ConnectionState::update_initial_client_rwnd(Http2WindowSize new_size)
Review Comment:
I have a branch where I did the work for this. I'll PR after this gets merged:
more_h2_client_server_renames
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] shinrich commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
shinrich commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1244716790
Hmm. I'd be open to a solution that dynamically changes the initial window size based on the number of open streams. I would be concerned that fiddling the setting that frequently would mess up the the peer.
One benefit of having two district settings is if there are in fact numerous concurrent streams, it is unlikely that one will suck up all the resources before the other streams can get started.
Without this PR or some sort of dynamically resizing stream window in setting, we do need to update our documentation to tell folks that they should set the connection window size to be number of expected concurrent active streams * the actual desired window size.
I noticed this issue primarily with session resource with H2 to origin. It probably doesn't matter as much for the client side. But on the origin side, you are more vulnerable to greedy streams.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1243098660
Like I said on #8199, this change doesn't make sense to me. Just setting a large number to connection window size is effectively removing flow control for a connection. And even if you can set a reasonable value to connection window size, setting stream window size separately by hand isn't great because you need to estimate the average number of streams on a connection before hand. Let's say connection window size is 256KB. If the average number of streams is 2, stream window size should be 256 / 2 = 128KB. This stream window size is not the best for a connection that has just one stream (one stream can't fully utilize the connection).
My suggestion is:
- Keep `proxy.config.http2.initial_window_size_in` as the only setting for H2 window size
- Set the initial connection window size to `proxy.config.http2.initial_window_size_in * proxy.config.http2.max_concurrent_streams_in` at the beginning of a connection
- Send SETTINGS frame with the value of `the-connection-window-size / #-of-open-streams` when the number of open streams changes (or alternatively, send WINDOW_UPDATE on each stream)
You can set initial connection window size by adjusting initial_window_size_in (for stream) and max_concurrent_streams_in. This should work for you since you'd have a larger connection window size while you keep your initial stream window size as is. The last item is optional but it's to fully utilize a connection by a low number of streams and I think we should do this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1258289237
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt merged pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt merged PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#discussion_r998932176
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -4152,7 +4152,40 @@ HTTP/2 Configuration
:reloadable:
:units: bytes
- The initial window size for inbound connections.
+ The initial HTTP/2 stream window size for inbound connections that |TS| as a
+ receiver advertises to the peer. See IETF RFC 9113 section 5.2 for details
+ concerning HTTP/2 flow control. See
+ :ts:cv:`proxy.config.http2.flow_control.policy.in` for how HTTP/2 stream and
Review Comment:
I personally prefer a dot for this in/out thing, but we currently use an underscore. Let's consistently use an underscore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a diff in pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
maskit commented on code in PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#discussion_r998877610
##########
doc/admin-guide/files/records.config.en.rst:
##########
@@ -4152,7 +4152,40 @@ HTTP/2 Configuration
:reloadable:
:units: bytes
- The initial window size for inbound connections.
+ The initial HTTP/2 stream window size for inbound connections that |TS| as a
+ receiver advertises to the peer. See IETF RFC 9113 section 5.2 for details
+ concerning HTTP/2 flow control. See
+ :ts:cv:`proxy.config.http2.flow_control.in.policy` for how HTTP/2 stream and
Review Comment:
Many other setting names have `_in` and `_out` at the end. Are there any reason you put "in" in the middle?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
bneradt commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1258637621
[approve ci]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1251973134
> But given the complexity of implementing that dynamic behavior, I believe our conclusion was to move forward with this separate explicit and static session and stream window configuration.
This is why I feel like the issue is not fully understood. I don't think we should go that way. I was -0 because I was tired.
> For now, simply set the session window size to the product of the stream window size and the max streams size, or
H2 to Origin would work since session window size would be larger (although this PR is one for incoming connection), but the session window utilization issue would remain until somebody implements dynamic stream window size based on the number of streams. Increasing stream window size means increasing session window size, so the window utilization issue can't be resolved with any setting values. If we do this, I'd say dynamic stream window size is essential and we need to have it first to avoid the issue.
> for now, add the separate static session window size configuration (i.e., what is implemented in this patch).
Like I explained with examples above, this allows users to set window sizes to values that don't make sense. Setting a reasonably big value to session window size make sense in general and people would do that, but without dynamic stream window size, such setting actually causes the window utilization issue and make things worse if a client don't use many streams. I don't think we should add such a trap.
> It seems to me that long-term we all agree that we want a session window size configuration. Am I correct in understanding that?
Not quite, there isn't huge difference in this context though. I'd say we all agree that we want to make session window size configurable. I can live with the setting for session window size but I don't want to have a value for session window size on my config. That's why I suggested to support -1 (auto) as a compromise if we really need to add it.
> That is, if the stream window size will be dynamic, then users will generally prefer to set the session window size to some value and leave the stream window size to be dynamic based upon the number of streams. In which case the stream window size will probably support a -1 value (as you suggested above for the session window size) which means it will be dynamic.
Hmm, we all need to be more specific if we talk about this. Note that the existing setting is INITIAL stream window size. A server may not want to give much window size to a stream until it validates a request on the stream is valid, although ATS doesn't increase the size at the moment. If we have initial_stream_window_size and also max_stream_window_size, and if you set session_window_size to a value > 0, then setting max_stream_window_size to -1 makes sense. Either max_stream_window_size or session_window_size can be -1, but not the both at the same time.
And in that sense, INITIAL session window size, which you proposed, doesn't make sense because nothing updates it.
I'm wondering if we can wait for dynamic stream window size, if we agree it's best behavior. I don't think it's too complicated. I can't work on it right now, but it seems like there are still a couple of things to do to merge H2 to Origin. Can you tell me your concerns on dynamic stream window size?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #9085: Make HTTP/2 session and stream windows configurable
Posted by GitBox <gi...@apache.org>.
maskit commented on PR #9085:
URL: https://github.com/apache/trafficserver/pull/9085#issuecomment-1246123899
> To make sure I understand you correctly: you prefer that this patch set the window (session) size to the product of max_concurrent_streams_in by initial_window_size_in (the stream window size)? That is, you would prefer that over adding a separate configuration to set the window size? I could potentially be persuaded to start by just setting the session window size to the product of these two values for now.
Yes, that's correct. If there's something we can't do without the setting for connection window size, I'm totally fine with adding it. But I don't see the necessity so far.
> What are your thoughts on a configurable multiplier?
It doesn't make much sense to me because you can achieve the same thing by adjusting the two existing settings.
A compromise would be a setting for session window size with support for auto adjustment. By default it's set to -1 and the size will be automatically calculated based on initial stream window size and max concurrent streams, but you can set a specific size if you need for some reasons.
But it still solves only half of my concern. How do you fully utilize a connection (or its connection window) by unknown number of streams? I'm asking this question again and again, but nobody has told me any solution. It hasn't been an issue because the both window sizes are the same. One stream could occupy the entire connection window. Now, with the new setting, session window size is bigger than stream window size. A connection that has just one stream would be unreasonably limited. You don't care about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org