From ad946a704dc19b1a5aa51692ca7aafb5b015ba7c Mon Sep 17 00:00:00 2001 From: Christopher Faulet Date: Wed, 26 Jun 2024 14:45:24 +0200 Subject: [PATCH] MINOR: stick-table: Always decrement ref count before killing a session Guarded functions to kill a sticky session, stksess_kill() stksess_kill_if_expired(), may or may not decrement and test its reference counter before really killing it. This depends on a parameter. If it is set to non-zero value, the ref count is decremented and if it falls to zero, the session is killed. Otherwise, if this parameter is equal to zero, the session is killed, regardless the ref count value. In the code, these functions are always called with a non-zero parameter and the ref count is always decremented and tested. So, there is no reason to still have a special case. Especially because it is not really easy to say if it is supported or not. Does it mean it is possible to kill a sticky session while it is still referenced somewhere ? probably not. So, does it mean it is possible to kill a unreferenced session ? This case may be problematic because the session is accessed outside of any lock and thus may be released by another thread because it is unreferenced. Enlarging scope of the lock to avoid any issue is possible but it is a bit of shame to do so because there is no usage for now. The best is to simplify the API and remove this case. Now, stksess_kill() and stksess_kill_if_expired() functions always decrement and test the ref count before killing a sticky session. --- include/haproxy/session.h | 2 +- include/haproxy/stick_table.h | 16 ++++++++++++---- include/haproxy/stream.h | 4 ++-- src/stick_table.c | 15 +++++++++------ 4 files changed, 24 insertions(+), 13 deletions(-) diff --git a/include/haproxy/session.h b/include/haproxy/session.h index 9fd540a6d..f0731c711 100644 --- a/include/haproxy/session.h +++ b/include/haproxy/session.h @@ -78,7 +78,7 @@ static inline void session_store_counters(struct session *sess) } stkctr_set_entry(stkctr, NULL); - stksess_kill_if_expired(stkctr->table, ts, 1); + stksess_kill_if_expired(stkctr->table, ts); } } diff --git a/include/haproxy/stick_table.h b/include/haproxy/stick_table.h index 3e10db107..4132af129 100644 --- a/include/haproxy/stick_table.h +++ b/include/haproxy/stick_table.h @@ -45,7 +45,7 @@ struct stktable *stktable_find_by_name(const char *name); struct stksess *stksess_new(struct stktable *t, struct stktable_key *key); void stksess_setkey(struct stktable *t, struct stksess *ts, struct stktable_key *key); void stksess_free(struct stktable *t, struct stksess *ts); -int stksess_kill(struct stktable *t, struct stksess *ts, int decrefcount); +int stksess_kill(struct stktable *t, struct stksess *ts); int stktable_get_key_shard(struct stktable *t, const void *key, size_t len); int stktable_init(struct stktable *t, char **err_msg); @@ -215,7 +215,15 @@ static inline int __stksess_kill_if_expired(struct stktable *t, struct stksess * return 0; } -static inline void stksess_kill_if_expired(struct stktable *t, struct stksess *ts, int decrefcnt) +/* + * Decrease the refcount of a stksess and relase it if the refcount falls to 0 + * _AND_ if the session expired. Note,, the refcount is always decremented. + * + * This function locks the corresponding table shard to proceed. When this + * function is called, the caller must be sure it owns a reference on the + * stksess (refcount >= 1). + */ +static inline void stksess_kill_if_expired(struct stktable *t, struct stksess *ts) { uint shard; size_t len; @@ -232,11 +240,11 @@ static inline void stksess_kill_if_expired(struct stktable *t, struct stksess *t ALREADY_CHECKED(shard); HA_RWLOCK_WRLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock); - if (!decrefcnt || !HA_ATOMIC_SUB_FETCH(&ts->ref_cnt, 1)) + if (!HA_ATOMIC_SUB_FETCH(&ts->ref_cnt, 1)) __stksess_kill_if_expired(t, ts); HA_RWLOCK_WRUNLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock); } - else if (decrefcnt) + else HA_ATOMIC_SUB_FETCH(&ts->ref_cnt, 1); } diff --git a/include/haproxy/stream.h b/include/haproxy/stream.h index 12c58b891..6781249e8 100644 --- a/include/haproxy/stream.h +++ b/include/haproxy/stream.h @@ -140,7 +140,7 @@ static inline void stream_store_counters(struct stream *s) stktable_touch_local(s->stkctr[i].table, ts, 0); } stkctr_set_entry(&s->stkctr[i], NULL); - stksess_kill_if_expired(s->stkctr[i].table, ts, 1); + stksess_kill_if_expired(s->stkctr[i].table, ts); } } @@ -182,7 +182,7 @@ static inline void stream_stop_content_counters(struct stream *s) stktable_touch_local(s->stkctr[i].table, ts, 0); } stkctr_set_entry(&s->stkctr[i], NULL); - stksess_kill_if_expired(s->stkctr[i].table, ts, 1); + stksess_kill_if_expired(s->stkctr[i].table, ts); } } diff --git a/src/stick_table.c b/src/stick_table.c index 2b5eddd7b..f562a62a2 100644 --- a/src/stick_table.c +++ b/src/stick_table.c @@ -162,11 +162,14 @@ int __stksess_kill(struct stktable *t, struct stksess *ts) } /* - * Decrease the refcount if decrefcnt is not 0, and try to kill the stksess. + * Decrease the refcount of a stksess and relase it if the refcount falls to 0. * Returns non-zero if deleted, zero otherwise. - * This function locks the table + * + * This function locks the corresponding table shard to proceed. When this + * function is called, the caller must be sure it owns a reference on the + * stksess (refcount >= 1). */ -int stksess_kill(struct stktable *t, struct stksess *ts, int decrefcnt) +int stksess_kill(struct stktable *t, struct stksess *ts) { uint shard; size_t len; @@ -183,7 +186,7 @@ int stksess_kill(struct stktable *t, struct stksess *ts, int decrefcnt) ALREADY_CHECKED(shard); HA_RWLOCK_WRLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock); - if (!decrefcnt || !HA_ATOMIC_SUB_FETCH(&ts->ref_cnt, 1)) + if (!HA_ATOMIC_SUB_FETCH(&ts->ref_cnt, 1)) ret = __stksess_kill(t, ts); HA_RWLOCK_WRUNLOCK(STK_TABLE_LOCK, &t->shards[shard].sh_lock); @@ -5268,7 +5271,7 @@ static int table_process_entry(struct appctx *appctx, struct stksess *ts, char * break; case STK_CLI_ACT_CLR: - if (!stksess_kill(t, ts, 1)) { + if (!stksess_kill(t, ts)) { /* don't delete an entry which is currently referenced */ return cli_err(appctx, "Entry currently in use, cannot remove\n"); } @@ -5685,7 +5688,7 @@ static void cli_release_show_table(struct appctx *appctx) struct show_table_ctx *ctx = appctx->svcctx; if (ctx->state == STATE_DUMP) { - stksess_kill_if_expired(ctx->t, ctx->entry, 1); + stksess_kill_if_expired(ctx->t, ctx->entry); } }