Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTTP/2 Framing layer implementation (#309). #1233

Merged
merged 17 commits into from
May 16, 2019
Merged

HTTP/2 Framing layer implementation (#309). #1233

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
3d77573
HTTP/2 Framing layer implementation (#309).
aleksostapenko Mar 28, 2019
10be35a
Minor layout correction (#309).
aleksostapenko Mar 28, 2019
7139f53
Correction of HTTP/2 switching (#309).
aleksostapenko Mar 29, 2019
26cc780
Minor correction in unit tests (#309).
aleksostapenko Mar 29, 2019
fe57412
HTTP/2 Starting/Switching implementation (#309).
aleksostapenko Apr 10, 2019
ed79830
Remove forgotten debug code (#309).
aleksostapenko Apr 10, 2019
f6e8edc
Implementation of base functionality for HTTP/2 Stream Scheduler (#309).
aleksostapenko Apr 25, 2019
79fcd17
Corrections according review comments (#309).
aleksostapenko Apr 28, 2019
05bd194
Add processing for zero length of application frames payload (#309).
aleksostapenko Apr 29, 2019
23d53db
Corrections according review comments (#309).
aleksostapenko Apr 29, 2019
5b93ee8
Implementation of HTTP/2 Stream FSM (#309).
aleksostapenko May 7, 2019
1e8be28
Split header and payload processing for service frames (#309).
aleksostapenko May 7, 2019
1d962dd
Add support for maximum window of Flow Control (#309).
aleksostapenko May 13, 2019
b22c1f7
Corrections according review comments (#309).
aleksostapenko May 15, 2019
5515e82
Corrections according review comments (#309).
aleksostapenko May 16, 2019
ad183a6
Add description in Tempesta FW configuration for HTTP/2 (#309).
aleksostapenko May 16, 2019
dc1f523
Make HTTP/1.1 protocol default (#309).
aleksostapenko May 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,12 @@ DBG_HTTP_PARSER ?= 0
DBG_SS ?= 0
DBG_TLS ?= 0
DBG_APM ?= 0
DBG_HTTP_FRAME ?= 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I bet there will be families of debug functions for HTTP2: for frames, HPACK, and what ever else. The whole group will have the same meaning to HTTP2 as DBG_HTTP_PARSER for HTTP1.1. Should we rename it as DBG_HTTP2_FRAME?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I cannot do such separation: these sets do not intersect yet; also both HTTP/1.1 and HTTP/2 protocols belong to the same HTTP domain, so there is no contradiction here. But in the nearest future (e.g. during HTTP parser implementation changing in context of HTTP/2 processing) mentioned intersections may appear, and in this case the separation for HTTP1_* and HTTP2_* may be really needed and will be made for preprocessor variables (and possibly for filenames too, e.g. http_frame.c/h, http_stream.c/h).

DBG_HTTP_STREAM ?= 0
TFW_CFLAGS += -DDBG_CFG=$(DBG_CFG) -DDBG_HTTP_PARSER=$(DBG_HTTP_PARSER)
TFW_CFLAGS += -DDBG_SS=$(DBG_SS) -DDBG_TLS=$(DBG_TLS) -DDBG_APM=$(DBG_APM)
TFW_CFLAGS += -DDBG_HTTP_FRAME=$(DBG_HTTP_FRAME)
TFW_CFLAGS += -DDBG_HTTP_STREAM=$(DBG_HTTP_STREAM)

PROC = $(shell cat /proc/cpuinfo)
ARCH = $(shell uname -m)
Expand Down
3 changes: 3 additions & 0 deletions tempesta_fw/connection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ typedef struct {

#define TFW_CONN_TYPE(c) ((c)->proto.type)
#define TFW_CONN_PROTO(c) TFW_CONN_TYPE2IDX(TFW_CONN_TYPE(c))
#define TFW_CONN_TLS(c) (TFW_CONN_TYPE(c) & TFW_FSM_HTTPS)

/*
* Queues in client and server connections provide support for correct
Expand Down Expand Up @@ -209,9 +210,11 @@ enum {
typedef struct {
TfwCliConn cli_conn;
TlsCtx tls;
TfwHttp2Ctx *h2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structure name becomes misleading, now it's more than TLS connection, it also stores the HTTP2 context. And a new issue is introduced: HTTP2 context is not allocated for non-TLS HTTP2 connections (which are allowed by standard).
In the same time HTTP2 connections will have two contexts in the same time: HTTP2 and HTTP1. HTTP1 context is stored as TfwConn->TfwHttpParser. Probably we should introduce a union in TfwConn class to store 'protocol context' whatever http2 or http1. Seems like we never want both. Even is the HTTP1 connection is upgraded to HTTP2 we can easily re-init the field.

Copy link
Contributor Author

@aleksostapenko aleksostapenko Apr 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HTTP2 context is not allocated for non-TLS HTTP2 connections (which are allowed by standard)

In result of discussion in #309 and in chat - it has been decided not to support non-TLS HTTP/2 in TempestaFW.

Probably we should introduce a union in TfwConn class to store 'protocol context' whatever http2 or http1.

Yes, agree with that. The similar approach is proposed in #309 (comment) (in Stream Priority/Dependencies). These changes is part of future PRs in context of #309 ("Request HTTP/2 -> HTTP/1.1" or more soon "Disable @seq_queue for HTTP/2"). During implementation of Framing, Scheduler and Stream States - I temporary left that part as is - for testing/debugging purposes.

} TfwTlsConn;

#define tfw_tls_context(conn) (TlsCtx *)(&((TfwTlsConn *)conn)->tls)
#define tfw_http2_context(conn) ((TfwHttp2Ctx *)((TfwTlsConn *)conn)->h2)

/* Callbacks used by l5-l7 protocols to operate on connection level. */
typedef struct {
Expand Down
9 changes: 6 additions & 3 deletions tempesta_fw/http.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "http_limits.h"
#include "http_tbl.h"
#include "http_parser.h"
#include "http_frame.h"
#include "client.h"
#include "http_msg.h"
#include "http_sess.h"
Expand Down Expand Up @@ -3884,8 +3885,8 @@ tfw_http_resp_process(TfwConn *conn, const TfwFsmData *data)
/**
* @return status (application logic decision) of the message processing.
*/
static int
__tfw_http_msg_process(void *conn, TfwFsmData *data)
int
tfw_http_msg_process_generic(void *conn, TfwFsmData *data)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad naming. It's not generic. It's HTTP1 processing.

Copy link
Contributor Author

@aleksostapenko aleksostapenko Apr 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the nearest future PRs (in context of #309) this function is planned for usage for HTTP/2 processing too, so I have run a little forward here. In my opinion, most part of tfw_http_msg_process_generic() is general processing, whatever HTTP/1.1 or HTTP/2 protocol (except the parsing stage only), so it is worth to try adapt it for both protocols. In any case if this idea will fail, and the separate procedures will be needed - the name will be reverted accordingly.

{
TfwConn *c = (TfwConn *)conn;

Expand Down Expand Up @@ -3930,7 +3931,9 @@ tfw_http_msg_process(void *conn, TfwFsmData *data)
if (likely(r == T_OK || r == T_POSTPONE)) {
data->skb->next = data->skb->prev = NULL;
data->trail = !next ? trail : 0;
r = __tfw_http_msg_process(conn, data);
r = TFW_CONN_TLS((TfwConn *)conn) && tfw_http2_context(conn)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like TFW_CONN_TLS((TfwConn *)conn) is extra here. HTTP2 recommends using HTTP2 over TLS (h2) but it still allows HTTP2 over TCP (h2c). Personally I don't understand why the unencrypted connections are allowed by standard, but we have to follow them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been decided not to support non-TLS HTTP/2 in TempestaFW: standard allows HTTP/2 over plain TCP, but does not force to use it.

? tfw_http2_frame_process(conn, data)
: tfw_http_msg_process_generic(conn, data);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #309 we discussed that we should split HTTP parser in at least 2 pieces: headers and body and both the parsers will be called separately from H2 layer. How do you see the split in current code? Check H2 state in tfw_http_req_process()?

Frankly, now we have single-jump cost in our parser to enter any state and I believe we're able to make GCC, probably with some assembly help, to generate proper FSM code layout to make the large parser code faster (after all, even after the parser split, we'll have complex FSM due to many checks), so probably we'll be fine with current code if it simpler than possible split parser.

Copy link
Contributor Author

@aleksostapenko aleksostapenko May 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check H2 state in tfw_http_req_process()?

Yeah - this must be a frame type and, maybe, a stream state (since we have to track request's start/end, and this exactly matches with stream's states, as mentioned in penultimate paragraph in https://tools.ietf.org/html/rfc7540#section-8.1).

Сurrently I'm not very optimistic about parser division into separate parts: indeed, we have rather effective implementation with single-jump cost to enter any state; and with divided parser we will get costs of additional function calls (if not inlined) and necessity for additional branching to call separate parser parts (with associated costs). But in general, I am still thinking about this point.

} else {
kfree(data->skb);
}
Expand Down
1 change: 1 addition & 0 deletions tempesta_fw/http.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ typedef void (*tfw_http_cache_cb_t)(TfwHttpMsg *);

/* External HTTP functions. */
int tfw_http_msg_process(void *conn, TfwFsmData *data);
int tfw_http_msg_process_generic(void *conn, TfwFsmData *data);
unsigned long tfw_http_req_key_calc(TfwHttpReq *req);
void tfw_http_req_destruct(void *msg);
void tfw_http_resp_fwd(TfwHttpResp *resp);
Expand Down
Loading