From 114e83d875859f36883cc10ffa089eb534aecaff Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 30 Mar 2022 13:51:18 -0700 Subject: [PATCH] HTTP2: Initiate write for acknowledging SETTINGS frame (#29218) * HTTP2: Initiate write for acknowledging SETTINGS frame * Codegen * yapf code * Fix tests --- .../ext/transport/chttp2/transport/chttp2_transport.cc | 5 +++++ .../ext/transport/chttp2/transport/frame_settings.cc | 2 ++ src/core/ext/transport/chttp2/transport/internal.h | 1 + src/core/lib/debug/stats_data.cc | 2 ++ src/core/lib/debug/stats_data.h | 5 +++++ src/core/lib/debug/stats_data.yaml | 2 ++ src/core/lib/debug/stats_data_bq_schema.sql | 1 + test/core/bad_client/bad_client.cc | 5 ++++- tools/run_tests/performance/massage_qps_stats.py | 3 +++ .../run_tests/performance/scenario_result_schema.json | 10 ++++++++++ 10 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 268fd8c784e..df180d30dc9 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -852,6 +852,9 @@ static void inc_initiate_write_reason( case GRPC_CHTTP2_INITIATE_WRITE_SEND_SETTINGS: GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_SEND_SETTINGS(); break; + case GRPC_CHTTP2_INITIATE_WRITE_SETTINGS_ACK: + GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_SETTINGS_ACK(); + break; case GRPC_CHTTP2_INITIATE_WRITE_FLOW_CONTROL_UNSTALLED_BY_SETTING: GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_FLOW_CONTROL_UNSTALLED_BY_SETTING(); break; @@ -3300,6 +3303,8 @@ const char* grpc_chttp2_initiate_write_reason_string( return "TRANSPORT_FLOW_CONTROL"; case GRPC_CHTTP2_INITIATE_WRITE_SEND_SETTINGS: return "SEND_SETTINGS"; + case GRPC_CHTTP2_INITIATE_WRITE_SETTINGS_ACK: + return "SETTINGS_ACK"; case GRPC_CHTTP2_INITIATE_WRITE_FLOW_CONTROL_UNSTALLED_BY_SETTING: return "FLOW_CONTROL_UNSTALLED_BY_SETTING"; case GRPC_CHTTP2_INITIATE_WRITE_FLOW_CONTROL_UNSTALLED_BY_UPDATE: diff --git a/src/core/ext/transport/chttp2/transport/frame_settings.cc b/src/core/ext/transport/chttp2/transport/frame_settings.cc index c488f5aba5d..eefd6d29c8f 100644 --- a/src/core/ext/transport/chttp2/transport/frame_settings.cc +++ b/src/core/ext/transport/chttp2/transport/frame_settings.cc @@ -151,6 +151,8 @@ grpc_error_handle grpc_chttp2_settings_parser_parse(void* p, GRPC_CHTTP2_NUM_SETTINGS * sizeof(uint32_t)); t->num_pending_induced_frames++; grpc_slice_buffer_add(&t->qbuf, grpc_chttp2_settings_ack_create()); + grpc_chttp2_initiate_write(t, + GRPC_CHTTP2_INITIATE_WRITE_SETTINGS_ACK); if (t->notify_on_receive_settings != nullptr) { grpc_core::ExecCtx::Run(DEBUG_LOCATION, t->notify_on_receive_settings, diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 48c62ef1d77..fff63d635cd 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -98,6 +98,7 @@ typedef enum { GRPC_CHTTP2_INITIATE_WRITE_STREAM_FLOW_CONTROL, GRPC_CHTTP2_INITIATE_WRITE_TRANSPORT_FLOW_CONTROL, GRPC_CHTTP2_INITIATE_WRITE_SEND_SETTINGS, + GRPC_CHTTP2_INITIATE_WRITE_SETTINGS_ACK, GRPC_CHTTP2_INITIATE_WRITE_FLOW_CONTROL_UNSTALLED_BY_SETTING, GRPC_CHTTP2_INITIATE_WRITE_FLOW_CONTROL_UNSTALLED_BY_UPDATE, GRPC_CHTTP2_INITIATE_WRITE_APPLICATION_PING, diff --git a/src/core/lib/debug/stats_data.cc b/src/core/lib/debug/stats_data.cc index 1ffe8a83f13..73963c4cd01 100644 --- a/src/core/lib/debug/stats_data.cc +++ b/src/core/lib/debug/stats_data.cc @@ -73,6 +73,7 @@ const char* grpc_stats_counter_name[GRPC_STATS_COUNTER_COUNT] = { "http2_initiate_write_due_to_stream_flow_control", "http2_initiate_write_due_to_transport_flow_control", "http2_initiate_write_due_to_send_settings", + "http2_initiate_write_due_to_settings_ack", "http2_initiate_write_due_to_bdp_estimator_ping", "http2_initiate_write_due_to_flow_control_unstalled_by_setting", "http2_initiate_write_due_to_flow_control_unstalled_by_update", @@ -181,6 +182,7 @@ const char* grpc_stats_counter_doc[GRPC_STATS_COUNTER_COUNT] = { "Number of HTTP2 writes initiated due to 'stream_flow_control'", "Number of HTTP2 writes initiated due to 'transport_flow_control'", "Number of HTTP2 writes initiated due to 'send_settings'", + "Number of HTTP2 writes initiated due to 'settings_ack'", "Number of HTTP2 writes initiated due to 'bdp_estimator_ping'", "Number of HTTP2 writes initiated due to " "'flow_control_unstalled_by_setting'", diff --git a/src/core/lib/debug/stats_data.h b/src/core/lib/debug/stats_data.h index 41eb8efb695..46084198fb2 100644 --- a/src/core/lib/debug/stats_data.h +++ b/src/core/lib/debug/stats_data.h @@ -74,6 +74,7 @@ typedef enum { GRPC_STATS_COUNTER_HTTP2_INITIATE_WRITE_DUE_TO_STREAM_FLOW_CONTROL, GRPC_STATS_COUNTER_HTTP2_INITIATE_WRITE_DUE_TO_TRANSPORT_FLOW_CONTROL, GRPC_STATS_COUNTER_HTTP2_INITIATE_WRITE_DUE_TO_SEND_SETTINGS, + GRPC_STATS_COUNTER_HTTP2_INITIATE_WRITE_DUE_TO_SETTINGS_ACK, GRPC_STATS_COUNTER_HTTP2_INITIATE_WRITE_DUE_TO_BDP_ESTIMATOR_PING, GRPC_STATS_COUNTER_HTTP2_INITIATE_WRITE_DUE_TO_FLOW_CONTROL_UNSTALLED_BY_SETTING, GRPC_STATS_COUNTER_HTTP2_INITIATE_WRITE_DUE_TO_FLOW_CONTROL_UNSTALLED_BY_UPDATE, @@ -281,6 +282,9 @@ typedef enum { #define GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_SEND_SETTINGS() \ GRPC_STATS_INC_COUNTER( \ GRPC_STATS_COUNTER_HTTP2_INITIATE_WRITE_DUE_TO_SEND_SETTINGS) +#define GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_SETTINGS_ACK() \ + GRPC_STATS_INC_COUNTER( \ + GRPC_STATS_COUNTER_HTTP2_INITIATE_WRITE_DUE_TO_SETTINGS_ACK) #define GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_BDP_ESTIMATOR_PING() \ GRPC_STATS_INC_COUNTER( \ GRPC_STATS_COUNTER_HTTP2_INITIATE_WRITE_DUE_TO_BDP_ESTIMATOR_PING) @@ -476,6 +480,7 @@ void grpc_stats_inc_server_cqs_checked(int x); #define GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_STREAM_FLOW_CONTROL() #define GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_TRANSPORT_FLOW_CONTROL() #define GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_SEND_SETTINGS() +#define GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_SETTINGS_ACK() #define GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_BDP_ESTIMATOR_PING() #define GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_FLOW_CONTROL_UNSTALLED_BY_SETTING() #define GRPC_STATS_INC_HTTP2_INITIATE_WRITE_DUE_TO_FLOW_CONTROL_UNSTALLED_BY_UPDATE() diff --git a/src/core/lib/debug/stats_data.yaml b/src/core/lib/debug/stats_data.yaml index af4553028e3..76dc7341762 100644 --- a/src/core/lib/debug/stats_data.yaml +++ b/src/core/lib/debug/stats_data.yaml @@ -173,6 +173,8 @@ doc: Number of HTTP2 writes initiated due to 'transport_flow_control' - counter: http2_initiate_write_due_to_send_settings doc: Number of HTTP2 writes initiated due to 'send_settings' +- counter: http2_initiate_write_due_to_settings_ack + doc: Number of HTTP2 writes initiated due to 'settings_ack' - counter: http2_initiate_write_due_to_bdp_estimator_ping doc: Number of HTTP2 writes initiated due to 'bdp_estimator_ping' - counter: http2_initiate_write_due_to_flow_control_unstalled_by_setting diff --git a/src/core/lib/debug/stats_data_bq_schema.sql b/src/core/lib/debug/stats_data_bq_schema.sql index 04b6d471f63..1c542c21b12 100644 --- a/src/core/lib/debug/stats_data_bq_schema.sql +++ b/src/core/lib/debug/stats_data_bq_schema.sql @@ -44,6 +44,7 @@ http2_initiate_write_due_to_close_from_api_per_iteration:FLOAT, http2_initiate_write_due_to_stream_flow_control_per_iteration:FLOAT, http2_initiate_write_due_to_transport_flow_control_per_iteration:FLOAT, http2_initiate_write_due_to_send_settings_per_iteration:FLOAT, +http2_initiate_write_due_to_settings_ack_per_iteration:FLOAT, http2_initiate_write_due_to_bdp_estimator_ping_per_iteration:FLOAT, http2_initiate_write_due_to_flow_control_unstalled_by_setting_per_iteration:FLOAT, http2_initiate_write_due_to_flow_control_unstalled_by_update_per_iteration:FLOAT, diff --git a/test/core/bad_client/bad_client.cc b/test/core/bad_client/bad_client.cc index 09a91d19cbe..5bb0153a886 100644 --- a/test/core/bad_client/bad_client.cc +++ b/test/core/bad_client/bad_client.cc @@ -282,9 +282,12 @@ grpc_bad_client_arg connection_preface_arg = { bool rst_stream_client_validator(grpc_slice_buffer* incoming, void* /*arg*/) { // Get last frame from incoming slice buffer. + constexpr int kExpectedFrameLength = 13; + if (incoming->length < kExpectedFrameLength) return false; grpc_slice_buffer last_frame_buffer; grpc_slice_buffer_init(&last_frame_buffer); - grpc_slice_buffer_trim_end(incoming, 13, &last_frame_buffer); + grpc_slice_buffer_trim_end(incoming, kExpectedFrameLength, + &last_frame_buffer); GPR_ASSERT(last_frame_buffer.count == 1); grpc_slice last_frame = last_frame_buffer.slices[0]; diff --git a/tools/run_tests/performance/massage_qps_stats.py b/tools/run_tests/performance/massage_qps_stats.py index d89e8aa18d8..77c02f62dca 100644 --- a/tools/run_tests/performance/massage_qps_stats.py +++ b/tools/run_tests/performance/massage_qps_stats.py @@ -157,6 +157,9 @@ def massage_qps_stats(scenario_result): stats[ "core_http2_initiate_write_due_to_send_settings"] = massage_qps_stats_helpers.counter( core_stats, "http2_initiate_write_due_to_send_settings") + stats[ + "core_http2_initiate_write_due_to_settings_ack"] = massage_qps_stats_helpers.counter( + core_stats, "http2_initiate_write_due_to_settings_ack") stats[ "core_http2_initiate_write_due_to_bdp_estimator_ping"] = massage_qps_stats_helpers.counter( core_stats, diff --git a/tools/run_tests/performance/scenario_result_schema.json b/tools/run_tests/performance/scenario_result_schema.json index b6f30a332dd..b3ac4794e1a 100644 --- a/tools/run_tests/performance/scenario_result_schema.json +++ b/tools/run_tests/performance/scenario_result_schema.json @@ -340,6 +340,11 @@ "name": "core_http2_initiate_write_due_to_send_settings", "type": "INTEGER" }, + { + "mode": "NULLABLE", + "name": "core_http2_initiate_write_due_to_settings_ack", + "type": "INTEGER" + }, { "mode": "NULLABLE", "name": "core_http2_initiate_write_due_to_bdp_estimator_ping", @@ -1172,6 +1177,11 @@ "name": "core_http2_initiate_write_due_to_send_settings", "type": "INTEGER" }, + { + "mode": "NULLABLE", + "name": "core_http2_initiate_write_due_to_settings_ack", + "type": "INTEGER" + }, { "mode": "NULLABLE", "name": "core_http2_initiate_write_due_to_bdp_estimator_ping",