From a14854bce9b14e5ad3c5827fa692663802461745 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Fri, 1 Apr 2022 15:00:05 +0200 Subject: [PATCH] detect: keyword list to hash to improve perf Since the switch to pcre2 this was much more heavily used, which would lead to measurable time spent in list handling. --- src/detect-engine.c | 106 ++++++++++++++++++++++++-------------------- src/detect-lua.c | 2 +- src/detect-pcre.c | 2 +- src/detect.h | 7 ++- 4 files changed, 64 insertions(+), 53 deletions(-) diff --git a/src/detect-engine.c b/src/detect-engine.c index 085b400ae2..000fed2112 100644 --- a/src/detect-engine.c +++ b/src/detect-engine.c @@ -78,6 +78,7 @@ #include "util-var-name.h" #include "util-profiling.h" #include "util-validate.h" +#include "util-hash-string.h" #include "tm-threads.h" #include "runmodes.h" @@ -2408,13 +2409,7 @@ DetectEngineCtx *DetectEngineCtxInitWithPrefix(const char *prefix) static void DetectEngineCtxFreeThreadKeywordData(DetectEngineCtx *de_ctx) { - DetectEngineThreadKeywordCtxItem *item = de_ctx->keyword_list; - while (item) { - DetectEngineThreadKeywordCtxItem *next = item->next; - SCFree(item); - item = next; - } - de_ctx->keyword_list = NULL; + HashListTableFree(de_ctx->keyword_hash); } static void DetectEngineCtxFreeFailedSigs(DetectEngineCtx *de_ctx) @@ -2888,15 +2883,16 @@ static int DetectEngineThreadCtxInitKeywords(DetectEngineCtx *de_ctx, DetectEngi det_ctx->keyword_ctxs_size = de_ctx->keyword_id; - DetectEngineThreadKeywordCtxItem *item = de_ctx->keyword_list; - while (item) { + HashListTableBucket *hb = HashListTableGetListHead(de_ctx->keyword_hash); + for (; hb != NULL; hb = HashListTableGetListNext(hb)) { + DetectEngineThreadKeywordCtxItem *item = HashListTableGetListData(hb); + det_ctx->keyword_ctxs_array[item->id] = item->InitFunc(item->data); if (det_ctx->keyword_ctxs_array[item->id] == NULL) { SCLogError(SC_ERR_DETECT_PREPARE, "setting up thread local detect ctx " "for keyword \"%s\" failed", item->name); return TM_ECODE_FAILED; } - item = item->next; } } return TM_ECODE_OK; @@ -2905,12 +2901,12 @@ static int DetectEngineThreadCtxInitKeywords(DetectEngineCtx *de_ctx, DetectEngi static void DetectEngineThreadCtxDeinitKeywords(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx) { if (de_ctx->keyword_id > 0) { - DetectEngineThreadKeywordCtxItem *item = de_ctx->keyword_list; - while (item) { + HashListTableBucket *hb = HashListTableGetListHead(de_ctx->keyword_hash); + for (; hb != NULL; hb = HashListTableGetListNext(hb)) { + DetectEngineThreadKeywordCtxItem *item = HashListTableGetListData(hb); + if (det_ctx->keyword_ctxs_array[item->id] != NULL) item->FreeFunc(det_ctx->keyword_ctxs_array[item->id]); - - item = item->next; } det_ctx->keyword_ctxs_size = 0; SCFree(det_ctx->keyword_ctxs_array); @@ -3376,6 +3372,29 @@ void DetectEngineThreadCtxInfo(ThreadVars *t, DetectEngineThreadCtx *det_ctx) PatternMatchThreadPrint(&det_ctx->mtcu, det_ctx->de_ctx->mpm_matcher); } +static uint32_t DetectKeywordCtxHashFunc(HashListTable *ht, void *data, uint16_t datalen) +{ + DetectEngineThreadKeywordCtxItem *ctx = data; + const char *name = ctx->name; + uint64_t hash = StringHashDjb2((const uint8_t *)name, strlen(name)) + (uint64_t)ctx->data; + hash %= ht->array_size; + return hash; +} + +static char DetectKeywordCtxCompareFunc(void *data1, uint16_t len1, void *data2, uint16_t len2) +{ + DetectEngineThreadKeywordCtxItem *ctx1 = data1; + DetectEngineThreadKeywordCtxItem *ctx2 = data2; + const char *name1 = ctx1->name; + const char *name2 = ctx2->name; + return (strcmp(name1, name2) == 0 && ctx1->data == ctx2->data); +} + +static void DetectKeywordCtxFreeFunc(void *ptr) +{ + SCFree(ptr); +} + /** \brief Register Thread keyword context Funcs * * \param de_ctx detection engine to register in @@ -3396,31 +3415,37 @@ int DetectRegisterThreadCtxFuncs(DetectEngineCtx *de_ctx, const char *name, void { BUG_ON(de_ctx == NULL || InitFunc == NULL || FreeFunc == NULL); + if (de_ctx->keyword_hash == NULL) { + de_ctx->keyword_hash = HashListTableInit(4096, // TODO + DetectKeywordCtxHashFunc, DetectKeywordCtxCompareFunc, DetectKeywordCtxFreeFunc); + BUG_ON(de_ctx->keyword_hash == NULL); + } + if (mode) { - DetectEngineThreadKeywordCtxItem *item = de_ctx->keyword_list; - while (item != NULL) { - if (strcmp(name, item->name) == 0) { - return item->id; - } + DetectEngineThreadKeywordCtxItem search = { .data = data, .name = name }; - item = item->next; - } + DetectEngineThreadKeywordCtxItem *item = + HashListTableLookup(de_ctx->keyword_hash, (void *)&search, 0); + if (item) + return item->id; + + /* fall through */ } - DetectEngineThreadKeywordCtxItem *item = SCMalloc(sizeof(DetectEngineThreadKeywordCtxItem)); + DetectEngineThreadKeywordCtxItem *item = SCCalloc(1, sizeof(DetectEngineThreadKeywordCtxItem)); if (unlikely(item == NULL)) return -1; - memset(item, 0x00, sizeof(DetectEngineThreadKeywordCtxItem)); item->InitFunc = InitFunc; item->FreeFunc = FreeFunc; item->data = data; item->name = name; - - item->next = de_ctx->keyword_list; - de_ctx->keyword_list = item; item->id = de_ctx->keyword_id++; + if (HashListTableAdd(de_ctx->keyword_hash, (void *)item, 0) < 0) { + SCFree(item); + return -1; + } return item->id; } @@ -3438,27 +3463,14 @@ int DetectRegisterThreadCtxFuncs(DetectEngineCtx *de_ctx, const char *name, void * recommended to store it in the keywords global ctx so that * it's freed when the de_ctx is freed. */ -int DetectUnregisterThreadCtxFuncs(DetectEngineCtx *de_ctx, - DetectEngineThreadCtx *det_ctx, void *data, const char *name) -{ - BUG_ON(de_ctx == NULL); - - DetectEngineThreadKeywordCtxItem *item = de_ctx->keyword_list; - DetectEngineThreadKeywordCtxItem *prev_item = NULL; - while (item != NULL) { - if (strcmp(name, item->name) == 0 && (data == item->data)) { - if (prev_item == NULL) - de_ctx->keyword_list = item->next; - else - prev_item->next = item->next; - if (det_ctx) - item->FreeFunc(det_ctx->keyword_ctxs_array[item->id]); - SCFree(item); - return 1; - } - prev_item = item; - item = item->next; - } +int DetectUnregisterThreadCtxFuncs(DetectEngineCtx *de_ctx, void *data, const char *name) +{ + /* might happen if we call this before a call to *Register* */ + if (de_ctx->keyword_hash == NULL) + return 1; + DetectEngineThreadKeywordCtxItem remove = { .data = data, .name = name }; + if (HashListTableRemove(de_ctx->keyword_hash, (void *)&remove, 0) == 0) + return 1; return 0; } /** \brief Retrieve thread local keyword ctx by id diff --git a/src/detect-lua.c b/src/detect-lua.c index 89bec7efc2..cc189d4afb 100644 --- a/src/detect-lua.c +++ b/src/detect-lua.c @@ -1137,7 +1137,7 @@ static void DetectLuaFree(DetectEngineCtx *de_ctx, void *ptr) if (lua->filename) SCFree(lua->filename); - DetectUnregisterThreadCtxFuncs(de_ctx, NULL, lua, "lua"); + DetectUnregisterThreadCtxFuncs(de_ctx, lua, "lua"); SCFree(lua); } diff --git a/src/detect-pcre.c b/src/detect-pcre.c index 0ce9cdda80..4311fcad52 100644 --- a/src/detect-pcre.c +++ b/src/detect-pcre.c @@ -956,7 +956,7 @@ static void DetectPcreFree(DetectEngineCtx *de_ctx, void *ptr) DetectPcreData *pd = (DetectPcreData *)ptr; DetectParseFreeRegex(&pd->parse_regex); - DetectUnregisterThreadCtxFuncs(de_ctx, NULL, pd, "pcre"); + DetectUnregisterThreadCtxFuncs(de_ctx, pd, "pcre"); SCFree(pd); diff --git a/src/detect.h b/src/detect.h index c55c48ddee..0c93277cec 100644 --- a/src/detect.h +++ b/src/detect.h @@ -911,8 +911,8 @@ typedef struct DetectEngineCtx_ { bool sigerror_ok; const char *sigerror; - /** list of keywords that need thread local ctxs */ - DetectEngineThreadKeywordCtxItem *keyword_list; + /** hash list of keywords that need thread local ctxs */ + HashListTable *keyword_hash; int keyword_id; struct { @@ -1543,8 +1543,7 @@ const SigGroupHead *SigMatchSignaturesGetSgh(const DetectEngineCtx *de_ctx, cons Signature *DetectGetTagSignature(void); - -int DetectUnregisterThreadCtxFuncs(DetectEngineCtx *, DetectEngineThreadCtx *,void *data, const char *name); +int DetectUnregisterThreadCtxFuncs(DetectEngineCtx *, void *data, const char *name); int DetectRegisterThreadCtxFuncs(DetectEngineCtx *, const char *name, void *(*InitFunc)(void *), void *data, void (*FreeFunc)(void *), int); void *DetectThreadCtxGetKeywordThreadCtx(DetectEngineThreadCtx *, int);