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.

diff --git a/include/haproxy/session.h b/include/haproxy/session.h
index 9fd540a6da2f5220d13759bfb22a47efc4648476..f0731c711c2bd2899972c55d2a83d946164684e2 100644 (file)
--- 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 3e10db107087a6b40ae352a3c218b9b409a791a4..4132af1293e43e0ec67b0ce575d0a06d6d60e650 100644 (file)
--- 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 12c58b89155e993c3428fb85c05b69c7e5ce126c..6781249e89389733610fb421b82636efc533cef3 100644 (file)
--- 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 2b5eddd7b65dbf5a1192b1983be3ec63a656589b..f562a62a25d1628679de8b0f62de012c1029e4ab 100644 (file)
--- 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);
        }
 }