Skip to content

Commit

Permalink
Do not remove idle streams immediatly.
Browse files Browse the repository at this point in the history
According to RFC 9113 section 5.1.1:
The first use of a new stream identifier implicitly
closes all streams in the "idle" state that might
have been initiated by that peer with a lower-valued
stream identifier.
But Firefox creates idle streams and uses them as a
nodes in priority tree and such streams should not be
deleted. So we don't remove and delete idle streams
if there count is less then TFW_MAX_IDLE_STREAMS (6).
Also make two fixes:
- add/remove idle streams under ctx lock.
- add streams to idle queue in reverse order to avoid
  going through all list (new stream has usually greater
  stream id).
  • Loading branch information
EvgeniiMekhanik committed Jul 22, 2024
1 parent 910f3a1 commit b20528d
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 40 deletions.
54 changes: 38 additions & 16 deletions fw/http2.c
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@
#include "http_frame.h"
#include "http_msg.h"

/*
* For https://tempesta-tech.com Firefox creates
* six idle streams, which are used as a nodes in
* priority tree and should not be deleted.
*/
#define TFW_MAX_IDLE_STREAMS 7
#define TFW_MAX_CLOSED_STREAMS 5

/**
Expand Down Expand Up @@ -274,24 +280,42 @@ tfw_h2_conn_terminate_close(TfwH2Ctx *ctx, TfwH2Err err_code, bool close,
void
tfw_h2_remove_idle_streams(TfwH2Ctx *ctx, unsigned int id)
{
TfwH2Conn *conn = container_of(ctx, TfwH2Conn, h2);
TfwStream *stream, *tmp;
TfwStream *cur;
TfwStreamQueue *idle_streams = &ctx->idle_streams;

/*
* We add and remove streams from idle queue under
* socket lock.
*/
assert_spin_locked(&((TfwConn *)conn)->sk->sk_lock.slock);
T_DBG3("%s: ctx [%p] idle streams num %lu\n", __func__, ctx,
idle_streams->num);

list_for_each_entry_safe_reverse(stream, tmp, &ctx->idle_streams.list,
hcl_node)
{
if (id <= stream->id)
while (1) {
spin_lock(&ctx->lock);

/*
* Although according to the RFC 9113 we must immediately
* close all idle streams with lower-valued stream identifier
* Firefox uses such idle streams as a nodes in priority tree.
* So we don't touch idle streams if there count is less then
* TFW_MAX_IDLE_STREAMS.
*/
if (idle_streams->num <= TFW_MAX_IDLE_STREAMS) {
spin_unlock(&ctx->lock);
break;
}

tfw_h2_stream_del_from_queue_nolock(stream);
tfw_h2_set_stream_state(stream, HTTP2_STREAM_CLOSED);
tfw_h2_stream_add_closed(ctx, stream);
BUG_ON(list_empty(&idle_streams->list));
cur = list_last_entry(&idle_streams->list, TfwStream,
hcl_node);
if (id <= cur->id) {
spin_unlock(&ctx->lock);
break;
}

tfw_h2_stream_unlink_nolock(ctx, cur);

spin_unlock(&ctx->lock);

T_DBG3("%s: ctx [%p] cur stream [%p]\n", __func__, ctx, cur);

tfw_h2_stream_clean(ctx, cur);
}
}

Expand All @@ -306,8 +330,6 @@ tfw_h2_conn_streams_cleanup(TfwH2Ctx *ctx)

T_DBG3("%s: ctx [%p] conn %p sched %p\n", __func__, ctx, conn, sched);

tfw_h2_remove_idle_streams(ctx, UINT_MAX);

rbtree_postorder_for_each_entry_safe(cur, next, &sched->streams, node) {
tfw_h2_stream_purge_all_and_free_response(cur);
tfw_h2_stream_unlink_lock(ctx, cur);
Expand Down
35 changes: 12 additions & 23 deletions fw/http_stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -167,50 +167,39 @@ tfw_h2_stream_purge_all_and_free_response(TfwStream *stream)
void
tfw_h2_stream_add_idle(TfwH2Ctx *ctx, TfwStream *idle)
{
TfwH2Conn *conn = container_of(ctx, TfwH2Conn, h2);
struct list_head *pos, *prev = &ctx->idle_streams.list;
TfwStreamQueue *idle_streams = &ctx->idle_streams;
TfwStream *cur;
bool found = false;

/*
* We add and remove streams from idle queue under the
* socket lock.
*/
assert_spin_locked(&((TfwConn *)conn)->sk->sk_lock.slock);

spin_lock(&ctx->lock);
/*
* Found first idle stream with id less than new idle
* stream, then insert new stream before this stream.
*/
list_for_each(pos, &ctx->idle_streams.list) {
TfwStream *stream = list_entry(pos, TfwStream, hcl_node);

if (idle->id > stream->id) {
list_for_each_entry_reverse(cur, &idle_streams->list, hcl_node) {
if (idle->id > cur->id) {
found = true;
break;
}
prev = &stream->hcl_node;
}

if (found) {
list_add(&idle->hcl_node, prev);
idle->queue = &ctx->idle_streams;
list_add(&idle->hcl_node, &cur->hcl_node);
idle->queue = idle_streams;
++idle->queue->num;
} else {
tfw_h2_stream_add_to_queue_nolock(&ctx->idle_streams, idle);
tfw_h2_stream_add_to_queue_nolock(idle_streams, idle);
}

spin_unlock(&ctx->lock);
}

void
tfw_h2_stream_remove_idle(TfwH2Ctx *ctx, TfwStream *stream)
{
TfwH2Conn *conn = container_of(ctx, TfwH2Conn, h2);

/*
* We add and remove streams from idle queue under the
* socket lock.
*/
assert_spin_locked(&((TfwConn *)conn)->sk->sk_lock.slock);
spin_lock(&ctx->lock);
tfw_h2_stream_del_from_queue_nolock(stream);
spin_unlock(&ctx->lock);
}

/*
Expand Down
3 changes: 2 additions & 1 deletion fw/http_stream_sched.c
Original file line number Diff line number Diff line change
Expand Up @@ -303,9 +303,10 @@ tfw_h2_find_stream_dep(TfwStreamSched *sched, unsigned int id)
static inline bool
tfw_h2_stream_sched_has_children(TfwStreamSchedEntry *entry)
{
return !eb_is_empty(&entry->active) || !eb_is_empty(&entry->blocked);
return !eb_is_empty(&entry->active) || !eb_is_empty(&entry->blocked);
}


static inline void
tfw_h2_stream_sched_move_child(TfwStreamSched *sched, TfwStream *child,
TfwStreamSchedEntry *parent, u64 deficit)
Expand Down

0 comments on commit b20528d

Please sign in to comment.