Master h2 rxbuf padding #3661
Master h2 rxbuf padding #3661
Conversation
|
What is the math behind not just using malloc buffers ? |
My assumption is that we use mempools for the duration of the task and in this case it's more opportunistic (it has to be h2 and there needs to be a request body). It's also a form of transient storage since this is a request body. Not sure why @mbgrydeland chose this, I only assumed. |
It is the unbounded outside influence memory usage that is the concern, and why I wanted to tie it into the stevedores. That way the stevedore's may purge content using LRU as the result of clients asking us to buffer request body data. Each H/2 stream may ask us to buffer 64k of data, so the total sum may become significant easily. Though this uses the Transient stevedore, which by default is unbounded. So out of the box there will be no limit. |
|
I can see the point in reusing the stevedore machinery. But re-purposing Transient is wrong: The limits I would want for this are very different from the limits, if any, I'd want for Transient. One solution: Invent a new "magic" stevedore for this. Other solution: Make which stevedore is used a parameter. I probably prefer the latter. |
Valid point. I will reword things accordingly. |
|
I think I'm OK with this now. |
mbgrydeland
force-pushed
the
master-h2-rxbuf-padding
branch
from
080c0cd
to
8122a79
Compare
|
Rebased, squashed and reordered for a better bisect experience |
Renamed to "h2_rxbuf_storage" |
|
I do not find time for an appropriate review at the moment and do not want to hold back this PR. |
This is an API for getting an arbitrary buffer through the stevedores. The stevedore in question may then deploy LRU nuking or other measures to control resource usage.
The H/2 session thread does have a VSL buffer already set up, but the 'wrk->vsl' pointer was not set. This caused issue for e.g. LRU_NukeOne() as it wants to log. Set the buffer for the duration that the worker is dedicated as an H/2 session thread.
This implements stream data handling using a buffer between the H/2 session thread and each stream thread. This is needed to avoid head of line blocking on the session socket when a data frame is received for a stream thread that is not yet ready to receive it. The buffer used will have to be as large as the send window the peer expects at the time the stream is opened. This will typically be 65535 unless the h2_initial_window_size parameter has been changed. Stream window updates will then be issued only once data is removed from the buffer by the request body being consumed from the request handling thread, limited in size to what space is then available in the buffer.
H2 streams waiting for request body data will timeout after timeout_idle seconds if no new data on the stream is being received. This will ensure that individual H2 streams can be reaped if there is no data received from the peer.
We have a strict min at the protocol default here. This is because we don't have the 'use settings only after peer ack' in place yet. If the value is lower than the protocol default, the very first stream could get a flow control error.
This makes it easier to not have to know exactly when and how many window updates to expect in a test case.
With the new request body data handling, Varnish changes behaviour significantly wrt to stream window updates sent to the client. Window updates will only be sent once the data is consumed by the client through the request body VFP handling. Test cases that rely on receiving a window update to sync the H/2 stream needs to be adopted.
This is the test case that fails if these changes aren't in tree. Note the commented out rxwinup commands that are necessary for the proper fail mode when run without the varnishtest window update changes.
According to the spec the padding is an 8-bit field, and fields should be treated as unsigned unless otherwise specified, which it is not for any of the padding related places. Allow varnishtest to generate padding up to 255 bytes long.
This was found lacking in our H2 implementation. Previously we would have included any padding bytes in the request body. Possibly it would have caused errors if there also was a C-L present, or more likely just corrupt request bodies. If the client sends nothing but padding bytes and ends up consuming the entire stream window with no actual request bytes buffered, the request thread side of things would not send any stream window updates. Handle this corner case by sending a window update from the session thread.
This parameter allows the user to choose which storage backend / stevedore that the H/2 receive buffers are allocated from. By default it uses Transient.
mbgrydeland
force-pushed
the
master-h2-rxbuf-padding
branch
from
8122a79
to
862a62b
Compare
|
FWIW, we could make it a regular string parameter with a simple change: diff --git i/include/tbl/params.h w/include/tbl/params.h
index 2365cc418..90e6a34ee 100644
--- i/include/tbl/params.h
+++ w/include/tbl/params.h
@@ -1551,11 +1551,23 @@ PARAM_THREAD(
* String parameters
*/
-# define PARAM_STRING(nm, pv, def, ...) \
- PARAM(, , nm, tweak_string, pv, NULL, NULL, def, NULL, __VA_ARGS__)
+# define PARAM_STRING(nm, tw, pv, def, ...) \
+ PARAM(, , nm, tw, pv, NULL, NULL, def, NULL, __VA_ARGS__)
+
+PARAM_STRING(
+ /* name */ h2_rxbuf_storage,
+ /* tweak */ tweak_h2_rxbuf_storage,
+ /* priv */ &mgt_stv_h2_rxbuf,
+ /* def */ "Transient",
+ /* descr */
+ "The name of the storage backend that HTTP/2 receive buffers"
+ " should be allocated from.",
+ /* flags */ MUST_RESTART
+)
PARAM_STRING(
/* name */ cc_command,
+ /* tweak */ tweak_string,
/* priv */ &mgt_cc_cmd,
/* def */ VCC_CC,
/* descr */
@@ -1568,6 +1580,7 @@ PARAM_STRING(
PARAM_STRING(
/* name */ vcl_path,
+ /* tweak */ tweak_string,
/* priv */ &mgt_vcl_path,
/* def */ VARNISH_VCL_DIR,
/* descr */
@@ -1582,6 +1595,7 @@ PARAM_STRING(
PARAM_STRING(
/* name */ vmod_path,
+ /* tweak */ tweak_string,
/* priv */ &mgt_vmod_path,
/* def */ VARNISH_VMOD_DIR,
/* descr */
@@ -1657,28 +1671,10 @@ PARAM_PCRE2(
" messages."
)
-/*--------------------------------------------------------------------
- * Custom parameters with separate tweak function
- */
-
-# define PARAM_CUSTOM(nm, pv, def, ...) \
- PARAM(, , nm, tweak_ ## nm, pv, NULL, NULL, def, NULL, __VA_ARGS__)
-
-PARAM_CUSTOM(
- /* name */ h2_rxbuf_storage,
- /* priv */ &mgt_stv_h2_rxbuf,
- /* def */ "Transient",
- /* descr */
- "The name of the storage backend that HTTP/2 receive buffers"
- " should be allocated from.",
- /* flags */ MUST_RESTART
-)
-
# undef PARAM_ALL
# undef PARAM_PCRE2
# undef PARAM_STRING
# undef PARAM_VCC
-# undef PARAM_CUSTOM
#endif /* defined(PARAM_ALL) */
#undef PARAM_MEMPOOL |
|
I suppose we could also call the tweak function "tweak_storage", it doesn't really have anything specific to the h2 rxbuf. |
|
I'm not sure about making it a general tweak_storage parameter. It only works for this parameter because the default is "Transient", which is exempted from being matched against an actual defined storage. If the default is anything else, it will fail during the initial set-all-parameters-to-its-default-value-routine, which happens before command line options are processed. I think it is fine to keep it as a special case tweak, and not try to generalise it. |
|
I think you would sill have the same problem if any future parameter referred to a stevedore, let's imagine for example |
And simply require string parameters to define their tweaks. Refs #3661
We could have shortlived_storage and req_body_storage parameters to stop requiring Transient for those special cases. Refs #3661
This PR solves two H/2 related bugs:
The patch set also improves on panic output for H/2.