From c1db424db507d5f5d9376e485130c58aacdcce58 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 May 2024 21:25:07 +0900 Subject: [PATCH 1/9] tree-wide: use LOG_PRI() and LOG_FAC() --- src/basic/log.c | 8 ++++---- src/journal/journald-kmsg.c | 2 +- src/journal/journald-native.c | 4 ++-- src/journal/journald-server.c | 8 +++++++- src/journal/journald-stream.c | 2 +- src/journal/journald-syslog.c | 8 ++++---- src/libsystemd/sd-journal/journal-send.c | 4 ++-- src/udev/udev-manager.c | 2 +- 8 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/basic/log.c b/src/basic/log.c index 411f2c36b1..13ad19a051 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -396,7 +396,7 @@ void log_forget_fds(void) { } void log_set_max_level(int level) { - assert(level == LOG_NULL || (level & LOG_PRIMASK) == level); + assert(level == LOG_NULL || LOG_PRI(level) == level); log_max_level = level; @@ -788,7 +788,7 @@ int log_dispatch_internal( return -ERRNO_VALUE(error); /* Patch in LOG_DAEMON facility if necessary */ - if ((level & LOG_FACMASK) == 0) + if (LOG_FAC(level) == 0) level |= log_facility; if (open_when_needed) @@ -1077,7 +1077,7 @@ int log_struct_internal( log_target == LOG_TARGET_NULL) return -ERRNO_VALUE(error); - if ((level & LOG_FACMASK) == 0) + if (LOG_FAC(level) == 0) level |= log_facility; if (IN_SET(log_target, @@ -1180,7 +1180,7 @@ int log_struct_iovec_internal( log_target == LOG_TARGET_NULL) return -ERRNO_VALUE(error); - if ((level & LOG_FACMASK) == 0) + if (LOG_FAC(level) == 0) level |= log_facility; if (IN_SET(log_target, LOG_TARGET_AUTO, diff --git a/src/journal/journald-kmsg.c b/src/journal/journald-kmsg.c index a5e1715b13..78f6e1fef3 100644 --- a/src/journal/journald-kmsg.c +++ b/src/journal/journald-kmsg.c @@ -260,7 +260,7 @@ void dev_kmsg_record(Server *s, char *p, size_t l) { iovec[n++] = IOVEC_MAKE_STRING("_TRANSPORT=kernel"); char syslog_priority[STRLEN("PRIORITY=") + DECIMAL_STR_MAX(int)]; - xsprintf(syslog_priority, "PRIORITY=%i", priority & LOG_PRIMASK); + xsprintf(syslog_priority, "PRIORITY=%i", LOG_PRI(priority)); iovec[n++] = IOVEC_MAKE_STRING(syslog_priority); char syslog_facility[STRLEN("SYSLOG_FACILITY=") + DECIMAL_STR_MAX(int)]; diff --git a/src/journal/journald-native.c b/src/journal/journald-native.c index d6bceb0e4a..0600102d32 100644 --- a/src/journal/journald-native.c +++ b/src/journal/journald-native.c @@ -53,13 +53,13 @@ static void server_process_entry_meta( else if (l == 17 && startswith(p, "SYSLOG_FACILITY=") && p[16] >= '0' && p[16] <= '9') - *priority = (*priority & LOG_PRIMASK) | ((p[16] - '0') << 3); + *priority = LOG_PRI(*priority) | ((p[16] - '0') << 3); else if (l == 18 && startswith(p, "SYSLOG_FACILITY=") && p[16] >= '0' && p[16] <= '9' && p[17] >= '0' && p[17] <= '9') - *priority = (*priority & LOG_PRIMASK) | (((p[16] - '0')*10 + (p[17] - '0')) << 3); + *priority = LOG_PRI(*priority) | (((p[16] - '0')*10 + (p[17] - '0')) << 3); else if (l >= 19 && startswith(p, "SYSLOG_IDENTIFIER=")) { diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c index ab312714b2..b2203a87ff 100644 --- a/src/journal/journald-server.c +++ b/src/journal/journald-server.c @@ -1273,7 +1273,13 @@ void server_dispatch_message( if (c && c->unit) { (void) server_determine_space(s, &available, /* limit= */ NULL); - rl = journal_ratelimit_test(s->ratelimit, c->unit, c->log_ratelimit_interval, c->log_ratelimit_burst, priority & LOG_PRIMASK, available); + rl = journal_ratelimit_test( + s->ratelimit, + c->unit, + c->log_ratelimit_interval, + c->log_ratelimit_burst, + LOG_PRI(priority), + available); if (rl == 0) return; diff --git a/src/journal/journald-stream.c b/src/journal/journald-stream.c index 81a0e68281..e8437e3af4 100644 --- a/src/journal/journald-stream.c +++ b/src/journal/journald-stream.c @@ -310,7 +310,7 @@ static int stdout_stream_log( syslog_priority[STRLEN("PRIORITY=")] = '0' + LOG_PRI(priority); iovec[n++] = IOVEC_MAKE_STRING(syslog_priority); - if (priority & LOG_FACMASK) { + if (LOG_FAC(priority) != 0) { xsprintf(syslog_facility, "SYSLOG_FACILITY=%i", LOG_FAC(priority)); iovec[n++] = IOVEC_MAKE_STRING(syslog_facility); } diff --git a/src/journal/journald-syslog.c b/src/journal/journald-syslog.c index 4539722b20..4ad73ed387 100644 --- a/src/journal/journald-syslog.c +++ b/src/journal/journald-syslog.c @@ -177,8 +177,8 @@ void server_forward_syslog(Server *s, int priority, const char *identifier, cons int syslog_fixup_facility(int priority) { - if ((priority & LOG_FACMASK) == 0) - return (priority & LOG_PRIMASK) | LOG_USER; + if (LOG_FAC(priority) == 0) + return LOG_PRI(priority) | LOG_USER; return priority; } @@ -403,10 +403,10 @@ void server_process_syslog_message( iovec[n++] = IOVEC_MAKE_STRING("_TRANSPORT=syslog"); - xsprintf(syslog_priority, "PRIORITY=%i", priority & LOG_PRIMASK); + xsprintf(syslog_priority, "PRIORITY=%i", LOG_PRI(priority)); iovec[n++] = IOVEC_MAKE_STRING(syslog_priority); - if (priority & LOG_FACMASK) { + if (LOG_FAC(priority) != 0) { xsprintf(syslog_facility, "SYSLOG_FACILITY=%i", LOG_FAC(priority)); iovec[n++] = IOVEC_MAKE_STRING(syslog_facility); } diff --git a/src/libsystemd/sd-journal/journal-send.c b/src/libsystemd/sd-journal/journal-send.c index 650581addf..7d02b57d7b 100644 --- a/src/libsystemd/sd-journal/journal-send.c +++ b/src/libsystemd/sd-journal/journal-send.c @@ -121,7 +121,7 @@ _public_ int sd_journal_printv(int priority, const char *format, va_list ap) { assert_return(priority <= 7, -EINVAL); assert_return(format, -EINVAL); - xsprintf(p, "PRIORITY=%i", priority & LOG_PRIMASK); + xsprintf(p, "PRIORITY=%i", LOG_PRI(priority)); va_copy(aq, ap); len = vsnprintf(buffer + 8, LINE_MAX, format, aq); @@ -485,7 +485,7 @@ _public_ int sd_journal_printv_with_location(int priority, const char *file, con assert_return(priority <= 7, -EINVAL); assert_return(format, -EINVAL); - xsprintf(p, "PRIORITY=%i", priority & LOG_PRIMASK); + xsprintf(p, "PRIORITY=%i", LOG_PRI(priority)); va_copy(aq, ap); len = vsnprintf(buffer + 8, LINE_MAX, format, aq); diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index 1f8d8e4950..bb94478775 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -885,7 +885,7 @@ static int on_ctrl_msg(UdevCtrl *uctrl, UdevCtrlMessageType type, const UdevCtrl switch (type) { case UDEV_CTRL_SET_LOG_LEVEL: - if ((value->intval & LOG_PRIMASK) != value->intval) { + if (LOG_PRI(value->intval) != value->intval) { log_debug("Received invalid udev control message (SET_LOG_LEVEL, %i), ignoring.", value->intval); break; } From 78da0721c84b0f63da621d6a00b8ab01cb0b979b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 12 May 2024 10:31:41 +0900 Subject: [PATCH 2/9] test: introduce test cases for journal_ratelimit_test() --- src/journal/meson.build | 6 ++++ src/journal/test-journald-rate-limit.c | 44 ++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) create mode 100644 src/journal/test-journald-rate-limit.c diff --git a/src/journal/meson.build b/src/journal/meson.build index 7d87754e0e..9f0e699501 100644 --- a/src/journal/meson.build +++ b/src/journal/meson.build @@ -122,6 +122,12 @@ executables += [ libxz_cflags, ], }, + test_template + { + 'sources' : files( + 'test-journald-rate-limit.c', + 'journald-rate-limit.c', + ), + }, journal_test_template + { 'sources' : files('test-journald-syslog.c'), 'dependencies' : [ diff --git a/src/journal/test-journald-rate-limit.c b/src/journal/test-journald-rate-limit.c new file mode 100644 index 0000000000..89d69bb950 --- /dev/null +++ b/src/journal/test-journald-rate-limit.c @@ -0,0 +1,44 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "journald-rate-limit.h" +#include "tests.h" + +TEST(journal_ratelimit_test) { + JournalRateLimit *rl; + int r; + + assert_se(rl = journal_ratelimit_new()); + + for (unsigned i = 0; i < 20; i++) { + r = journal_ratelimit_test(rl, "hoge", USEC_PER_SEC, 10, LOG_DEBUG, 0); + assert_se(r == (i < 10 ? 1 : 0)); + r = journal_ratelimit_test(rl, "foo", 10 * USEC_PER_SEC, 10, LOG_DEBUG, 0); + assert_se(r == (i < 10 ? 1 : 0)); + } + + /* Different priority group with the same ID is not ratelimited. */ + assert_se(journal_ratelimit_test(rl, "hoge", USEC_PER_SEC, 10, LOG_INFO, 0) == 1); + assert_se(journal_ratelimit_test(rl, "foo", 10 * USEC_PER_SEC, 10, LOG_INFO, 0) == 1); + /* Still LOG_DEBUG is ratelimited. */ + assert_se(journal_ratelimit_test(rl, "hoge", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 0); + assert_se(journal_ratelimit_test(rl, "foo", 10 * USEC_PER_SEC, 10, LOG_DEBUG, 0) == 0); + /* Different ID is not ratelimited. */ + assert_se(journal_ratelimit_test(rl, "quux", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 1); + + usleep_safe(USEC_PER_SEC); + + /* The ratelimit is now expired (11 trials are suppressed, so the return value should be 12). */ + assert_se(journal_ratelimit_test(rl, "hoge", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 1 + 11); + + /* foo is still ratelimited. */ + assert_se(journal_ratelimit_test(rl, "foo", 10 * USEC_PER_SEC, 10, LOG_DEBUG, 0) == 0); + + /* Still other priority and/or other IDs are not ratelimited. */ + assert_se(journal_ratelimit_test(rl, "hoge", USEC_PER_SEC, 10, LOG_INFO, 0) == 1); + assert_se(journal_ratelimit_test(rl, "foo", 10 * USEC_PER_SEC, 10, LOG_INFO, 0) == 1); + assert_se(journal_ratelimit_test(rl, "quux", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 1); + + journal_ratelimit_free(rl); +} + +DEFINE_TEST_MAIN(LOG_INFO); From 1cb1538f7897aefad17fb60c01932195300da9e9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 May 2024 18:56:57 +0900 Subject: [PATCH 3/9] journal-rate-limit: add trailing comma in table --- src/journal/journald-rate-limit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/journal/journald-rate-limit.c b/src/journal/journald-rate-limit.c index 1028e38689..ea2c27a686 100644 --- a/src/journal/journald-rate-limit.c +++ b/src/journal/journald-rate-limit.c @@ -23,7 +23,7 @@ static const int priority_map[] = { [LOG_WARNING] = 2, [LOG_NOTICE] = 3, [LOG_INFO] = 3, - [LOG_DEBUG] = 4 + [LOG_DEBUG] = 4, }; typedef struct JournalRateLimitPool JournalRateLimitPool; From bba23ba47fda689c974e48b3570fdbfde01e1be7 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 May 2024 20:28:26 +0900 Subject: [PATCH 4/9] journal-rate-limit: make journal_ratelimit_group_free() return NULL To make it follow our recent coding style. --- src/journal/journald-rate-limit.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/journal/journald-rate-limit.c b/src/journal/journald-rate-limit.c index ea2c27a686..25a5d2e92d 100644 --- a/src/journal/journald-rate-limit.c +++ b/src/journal/journald-rate-limit.c @@ -72,8 +72,9 @@ JournalRateLimit *journal_ratelimit_new(void) { return r; } -static void journal_ratelimit_group_free(JournalRateLimitGroup *g) { - assert(g); +static JournalRateLimitGroup* journal_ratelimit_group_free(JournalRateLimitGroup *g) { + if (!g) + return NULL; if (g->parent) { assert(g->parent->n_groups > 0); @@ -88,7 +89,7 @@ static void journal_ratelimit_group_free(JournalRateLimitGroup *g) { } free(g->id); - free(g); + return mfree(g); } void journal_ratelimit_free(JournalRateLimit *r) { From 1bdaad997ddd101fb6d8db8f550e48090bed5fe8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 May 2024 20:30:30 +0900 Subject: [PATCH 5/9] journal-rate-limit: use FOREACH_ELEMENT() and usec_add() --- src/journal/journald-rate-limit.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/journal/journald-rate-limit.c b/src/journal/journald-rate-limit.c index 25a5d2e92d..1ea60c22cf 100644 --- a/src/journal/journald-rate-limit.c +++ b/src/journal/journald-rate-limit.c @@ -102,12 +102,10 @@ void journal_ratelimit_free(JournalRateLimit *r) { } static bool journal_ratelimit_group_expired(JournalRateLimitGroup *g, usec_t ts) { - unsigned i; - assert(g); - for (i = 0; i < POOLS_MAX; i++) - if (g->pools[i].begin + g->interval >= ts) + FOREACH_ELEMENT(p, g->pools) + if (usec_add(p->begin, g->interval) >= ts) return false; return true; @@ -236,7 +234,7 @@ int journal_ratelimit_test(JournalRateLimit *r, const char *id, usec_t rl_interv return 1; } - if (p->begin + rl_interval < ts) { + if (usec_add(p->begin, rl_interval) < ts) { unsigned s; s = p->suppressed; From 88c434f75c99ab62a8d755b3ecc07e1ac367b45b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 May 2024 20:41:07 +0900 Subject: [PATCH 6/9] journal-rate-limit: rename variables 'r' is usually used for storing return value. --- src/journal/journald-rate-limit.c | 40 ++++++++++++++++++++----------- src/journal/journald-rate-limit.h | 8 ++++++- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/src/journal/journald-rate-limit.c b/src/journal/journald-rate-limit.c index 1ea60c22cf..69f19bdc36 100644 --- a/src/journal/journald-rate-limit.c +++ b/src/journal/journald-rate-limit.c @@ -121,10 +121,15 @@ static void journal_ratelimit_vacuum(JournalRateLimit *r, usec_t ts) { journal_ratelimit_group_free(r->lru_tail); } -static JournalRateLimitGroup* journal_ratelimit_group_new(JournalRateLimit *r, const char *id, usec_t interval, usec_t ts) { +static JournalRateLimitGroup* journal_ratelimit_group_new( + JournalRateLimit *rl, + const char *id, + usec_t interval, + usec_t ts) { + JournalRateLimitGroup *g; - assert(r); + assert(rl); assert(id); g = new0(JournalRateLimitGroup, 1); @@ -135,19 +140,19 @@ static JournalRateLimitGroup* journal_ratelimit_group_new(JournalRateLimit *r, c if (!g->id) goto fail; - g->hash = siphash24_string(g->id, r->hash_key); + g->hash = siphash24_string(g->id, rl->hash_key); g->interval = interval; - journal_ratelimit_vacuum(r, ts); + journal_ratelimit_vacuum(rl, ts); - LIST_PREPEND(bucket, r->buckets[g->hash % BUCKETS_MAX], g); - LIST_PREPEND(lru, r->lru, g); + LIST_PREPEND(bucket, rl->buckets[g->hash % BUCKETS_MAX], g); + LIST_PREPEND(lru, rl->lru, g); if (!g->lru_next) - r->lru_tail = g; - r->n_groups++; + rl->lru_tail = g; + rl->n_groups++; - g->parent = r; + g->parent = rl; return g; fail: @@ -183,7 +188,14 @@ static unsigned burst_modulate(unsigned burst, uint64_t available) { return burst; } -int journal_ratelimit_test(JournalRateLimit *r, const char *id, usec_t rl_interval, unsigned rl_burst, int priority, uint64_t available) { +int journal_ratelimit_test( + JournalRateLimit *rl, + const char *id, + usec_t rl_interval, + unsigned rl_burst, + int priority, + uint64_t available) { + JournalRateLimitGroup *g, *found = NULL; JournalRateLimitPool *p; unsigned burst; @@ -199,13 +211,13 @@ int journal_ratelimit_test(JournalRateLimit *r, const char *id, usec_t rl_interv * < 0 → error */ - if (!r) + if (!rl) return 1; ts = now(CLOCK_MONOTONIC); - h = siphash24_string(id, r->hash_key); - g = r->buckets[h % BUCKETS_MAX]; + h = siphash24_string(id, rl->hash_key); + g = rl->buckets[h % BUCKETS_MAX]; LIST_FOREACH(bucket, i, g) if (streq(i->id, id)) { @@ -214,7 +226,7 @@ int journal_ratelimit_test(JournalRateLimit *r, const char *id, usec_t rl_interv } if (!found) { - found = journal_ratelimit_group_new(r, id, rl_interval, ts); + found = journal_ratelimit_group_new(rl, id, rl_interval, ts); if (!found) return -ENOMEM; } else diff --git a/src/journal/journald-rate-limit.h b/src/journal/journald-rate-limit.h index 8def60f052..4539e174c3 100644 --- a/src/journal/journald-rate-limit.h +++ b/src/journal/journald-rate-limit.h @@ -7,4 +7,10 @@ typedef struct JournalRateLimit JournalRateLimit; JournalRateLimit *journal_ratelimit_new(void); void journal_ratelimit_free(JournalRateLimit *r); -int journal_ratelimit_test(JournalRateLimit *r, const char *id, usec_t rl_interval, unsigned rl_burst, int priority, uint64_t available); +int journal_ratelimit_test( + JournalRateLimit *rl, + const char *id, + usec_t rl_interval, + unsigned rl_burst, + int priority, + uint64_t available); From bab044b94bd8252dfd0e2c7479a7b24d0b3d43da Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 May 2024 20:44:53 +0900 Subject: [PATCH 7/9] journal-rate-limit: introduce cleanup function for JournalRateLimitGroup This also makes journal_ratelimit_group_new() use structured initializer. --- src/journal/journald-rate-limit.c | 39 +++++++++++++++++-------------- 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/src/journal/journald-rate-limit.c b/src/journal/journald-rate-limit.c index 69f19bdc36..e4e3728125 100644 --- a/src/journal/journald-rate-limit.c +++ b/src/journal/journald-rate-limit.c @@ -92,6 +92,8 @@ static JournalRateLimitGroup* journal_ratelimit_group_free(JournalRateLimitGroup return mfree(g); } +DEFINE_TRIVIAL_CLEANUP_FUNC(JournalRateLimitGroup*, journal_ratelimit_group_free); + void journal_ratelimit_free(JournalRateLimit *r) { assert(r); @@ -121,28 +123,30 @@ static void journal_ratelimit_vacuum(JournalRateLimit *r, usec_t ts) { journal_ratelimit_group_free(r->lru_tail); } -static JournalRateLimitGroup* journal_ratelimit_group_new( +static int journal_ratelimit_group_new( JournalRateLimit *rl, const char *id, usec_t interval, - usec_t ts) { + usec_t ts, + JournalRateLimitGroup **ret) { - JournalRateLimitGroup *g; + _cleanup_(journal_ratelimit_group_freep) JournalRateLimitGroup *g = NULL; assert(rl); assert(id); + assert(ret); - g = new0(JournalRateLimitGroup, 1); + g = new(JournalRateLimitGroup, 1); if (!g) - return NULL; + return -ENOMEM; - g->id = strdup(id); + *g = (JournalRateLimitGroup) { + .id = strdup(id), + .hash = siphash24_string(id, rl->hash_key), + .interval = interval, + }; if (!g->id) - goto fail; - - g->hash = siphash24_string(g->id, rl->hash_key); - - g->interval = interval; + return -ENOMEM; journal_ratelimit_vacuum(rl, ts); @@ -153,11 +157,9 @@ static JournalRateLimitGroup* journal_ratelimit_group_new( rl->n_groups++; g->parent = rl; - return g; -fail: - journal_ratelimit_group_free(g); - return NULL; + *ret = TAKE_PTR(g); + return 0; } static unsigned burst_modulate(unsigned burst, uint64_t available) { @@ -201,6 +203,7 @@ int journal_ratelimit_test( unsigned burst; uint64_t h; usec_t ts; + int r; assert(id); @@ -226,9 +229,9 @@ int journal_ratelimit_test( } if (!found) { - found = journal_ratelimit_group_new(rl, id, rl_interval, ts); - if (!found) - return -ENOMEM; + r = journal_ratelimit_group_new(rl, id, rl_interval, ts, &found); + if (r < 0) + return r; } else found->interval = rl_interval; From 8df477675d2b267a14ec946bf7fbaae65b8df40e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 May 2024 21:05:56 +0900 Subject: [PATCH 8/9] journal-rate-limit: split out journal_ratelimit_group_acquire() No functional change, just refactoring. --- src/journal/journald-rate-limit.c | 55 +++++++++++++++++++++---------- 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/src/journal/journald-rate-limit.c b/src/journal/journald-rate-limit.c index e4e3728125..afba668e0f 100644 --- a/src/journal/journald-rate-limit.c +++ b/src/journal/journald-rate-limit.c @@ -162,6 +162,38 @@ static int journal_ratelimit_group_new( return 0; } +static int journal_ratelimit_group_acquire( + JournalRateLimit *rl, + const char *id, + usec_t interval, + usec_t ts, + JournalRateLimitGroup **ret) { + + JournalRateLimitGroup *head, *g = NULL; + uint64_t h; + + assert(rl); + assert(id); + assert(ret); + + h = siphash24_string(id, rl->hash_key); + head = rl->buckets[h % BUCKETS_MAX]; + + LIST_FOREACH(bucket, i, head) + if (streq(i->id, id)) { + g = i; + break; + } + + if (!g) + return journal_ratelimit_group_new(rl, id, interval, ts, ret); + + g->interval = interval; + + *ret = g; + return 0; +} + static unsigned burst_modulate(unsigned burst, uint64_t available) { unsigned k; @@ -198,10 +230,9 @@ int journal_ratelimit_test( int priority, uint64_t available) { - JournalRateLimitGroup *g, *found = NULL; + JournalRateLimitGroup *g; JournalRateLimitPool *p; unsigned burst; - uint64_t h; usec_t ts; int r; @@ -219,28 +250,16 @@ int journal_ratelimit_test( ts = now(CLOCK_MONOTONIC); - h = siphash24_string(id, rl->hash_key); - g = rl->buckets[h % BUCKETS_MAX]; - - LIST_FOREACH(bucket, i, g) - if (streq(i->id, id)) { - found = i; - break; - } - - if (!found) { - r = journal_ratelimit_group_new(rl, id, rl_interval, ts, &found); - if (r < 0) - return r; - } else - found->interval = rl_interval; + r = journal_ratelimit_group_acquire(rl, id, rl_interval, ts, &g); + if (r < 0) + return r; if (rl_interval == 0 || rl_burst == 0) return 1; burst = burst_modulate(rl_burst, available); - p = &found->pools[priority_map[priority]]; + p = &g->pools[priority_map[priority]]; if (p->begin <= 0) { p->suppressed = 0; From 0e2e3fa35ae077ce9c23ca0086b7d2bba009abe4 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sat, 11 May 2024 21:09:44 +0900 Subject: [PATCH 9/9] journal-rate-limit: replace in-house management of JournalRateLimitGroup with OrderedHashmap No functional change, just refactoring. --- src/journal/journald-rate-limit.c | 132 ++++++++----------------- src/journal/journald-rate-limit.h | 9 +- src/journal/journald-server.c | 9 +- src/journal/journald-server.h | 3 +- src/journal/test-journald-rate-limit.c | 30 +++--- 5 files changed, 60 insertions(+), 123 deletions(-) diff --git a/src/journal/journald-rate-limit.c b/src/journal/journald-rate-limit.c index afba668e0f..a1ae17269c 100644 --- a/src/journal/journald-rate-limit.c +++ b/src/journal/journald-rate-limit.c @@ -1,18 +1,13 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ -#include - #include "alloc-util.h" #include "hashmap.h" #include "journald-rate-limit.h" -#include "list.h" #include "logarithm.h" -#include "random-util.h" #include "string-util.h" #include "time-util.h" #define POOLS_MAX 5 -#define BUCKETS_MAX 127 #define GROUPS_MAX 2047 static const int priority_map[] = { @@ -26,17 +21,14 @@ static const int priority_map[] = { [LOG_DEBUG] = 4, }; -typedef struct JournalRateLimitPool JournalRateLimitPool; -typedef struct JournalRateLimitGroup JournalRateLimitGroup; - -struct JournalRateLimitPool { +typedef struct JournalRateLimitPool { usec_t begin; unsigned num; unsigned suppressed; -}; +} JournalRateLimitPool; -struct JournalRateLimitGroup { - JournalRateLimit *parent; +typedef struct JournalRateLimitGroup { + OrderedHashmap *groups_by_id; char *id; @@ -44,49 +36,16 @@ struct JournalRateLimitGroup { usec_t interval; JournalRateLimitPool pools[POOLS_MAX]; - uint64_t hash; - - LIST_FIELDS(JournalRateLimitGroup, bucket); - LIST_FIELDS(JournalRateLimitGroup, lru); -}; - -struct JournalRateLimit { - - JournalRateLimitGroup* buckets[BUCKETS_MAX]; - JournalRateLimitGroup *lru, *lru_tail; - - unsigned n_groups; - - uint8_t hash_key[16]; -}; - -JournalRateLimit *journal_ratelimit_new(void) { - JournalRateLimit *r; - - r = new0(JournalRateLimit, 1); - if (!r) - return NULL; - - random_bytes(r->hash_key, sizeof(r->hash_key)); - - return r; -} +} JournalRateLimitGroup; static JournalRateLimitGroup* journal_ratelimit_group_free(JournalRateLimitGroup *g) { if (!g) return NULL; - if (g->parent) { - assert(g->parent->n_groups > 0); - - if (g->parent->lru_tail == g) - g->parent->lru_tail = g->lru_prev; - - LIST_REMOVE(lru, g->parent->lru, g); - LIST_REMOVE(bucket, g->parent->buckets[g->hash % BUCKETS_MAX], g); - - g->parent->n_groups--; - } + if (g->groups_by_id && g->id) + /* The group is already removed from the hashmap when this is called from the + * destructor of the hashmap. Hence, do not check the return value here. */ + ordered_hashmap_remove_value(g->groups_by_id, g->id, g); free(g->id); return mfree(g); @@ -94,14 +53,13 @@ static JournalRateLimitGroup* journal_ratelimit_group_free(JournalRateLimitGroup DEFINE_TRIVIAL_CLEANUP_FUNC(JournalRateLimitGroup*, journal_ratelimit_group_free); -void journal_ratelimit_free(JournalRateLimit *r) { - assert(r); - - while (r->lru) - journal_ratelimit_group_free(r->lru); - - free(r); -} +DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR( + journal_ratelimit_group_hash_ops, + char, + string_hash_func, + string_compare_func, + JournalRateLimitGroup, + journal_ratelimit_group_free); static bool journal_ratelimit_group_expired(JournalRateLimitGroup *g, usec_t ts) { assert(g); @@ -113,26 +71,29 @@ static bool journal_ratelimit_group_expired(JournalRateLimitGroup *g, usec_t ts) return true; } -static void journal_ratelimit_vacuum(JournalRateLimit *r, usec_t ts) { - assert(r); +static void journal_ratelimit_vacuum(OrderedHashmap *groups_by_id, usec_t ts) { /* Makes room for at least one new item, but drop all expired items too. */ - while (r->n_groups >= GROUPS_MAX || - (r->lru_tail && journal_ratelimit_group_expired(r->lru_tail, ts))) - journal_ratelimit_group_free(r->lru_tail); + while (ordered_hashmap_size(groups_by_id) >= GROUPS_MAX) + journal_ratelimit_group_free(ordered_hashmap_first(groups_by_id)); + + JournalRateLimitGroup *g; + while ((g = ordered_hashmap_first(groups_by_id)) && journal_ratelimit_group_expired(g, ts)) + journal_ratelimit_group_free(g); } static int journal_ratelimit_group_new( - JournalRateLimit *rl, + OrderedHashmap **groups_by_id, const char *id, usec_t interval, usec_t ts, JournalRateLimitGroup **ret) { _cleanup_(journal_ratelimit_group_freep) JournalRateLimitGroup *g = NULL; + int r; - assert(rl); + assert(groups_by_id); assert(id); assert(ret); @@ -142,51 +103,40 @@ static int journal_ratelimit_group_new( *g = (JournalRateLimitGroup) { .id = strdup(id), - .hash = siphash24_string(id, rl->hash_key), .interval = interval, }; if (!g->id) return -ENOMEM; - journal_ratelimit_vacuum(rl, ts); + journal_ratelimit_vacuum(*groups_by_id, ts); - LIST_PREPEND(bucket, rl->buckets[g->hash % BUCKETS_MAX], g); - LIST_PREPEND(lru, rl->lru, g); - if (!g->lru_next) - rl->lru_tail = g; - rl->n_groups++; + r = ordered_hashmap_ensure_put(groups_by_id, &journal_ratelimit_group_hash_ops, g->id, g); + if (r < 0) + return r; + assert(r > 0); - g->parent = rl; + g->groups_by_id = *groups_by_id; *ret = TAKE_PTR(g); return 0; } static int journal_ratelimit_group_acquire( - JournalRateLimit *rl, + OrderedHashmap **groups_by_id, const char *id, usec_t interval, usec_t ts, JournalRateLimitGroup **ret) { - JournalRateLimitGroup *head, *g = NULL; - uint64_t h; + JournalRateLimitGroup *g; - assert(rl); + assert(groups_by_id); assert(id); assert(ret); - h = siphash24_string(id, rl->hash_key); - head = rl->buckets[h % BUCKETS_MAX]; - - LIST_FOREACH(bucket, i, head) - if (streq(i->id, id)) { - g = i; - break; - } - + g = ordered_hashmap_get(*groups_by_id, id); if (!g) - return journal_ratelimit_group_new(rl, id, interval, ts, ret); + return journal_ratelimit_group_new(groups_by_id, id, interval, ts, ret); g->interval = interval; @@ -223,7 +173,7 @@ static unsigned burst_modulate(unsigned burst, uint64_t available) { } int journal_ratelimit_test( - JournalRateLimit *rl, + OrderedHashmap **groups_by_id, const char *id, usec_t rl_interval, unsigned rl_burst, @@ -236,6 +186,7 @@ int journal_ratelimit_test( usec_t ts; int r; + assert(groups_by_id); assert(id); /* Returns: @@ -245,12 +196,9 @@ int journal_ratelimit_test( * < 0 → error */ - if (!rl) - return 1; - ts = now(CLOCK_MONOTONIC); - r = journal_ratelimit_group_acquire(rl, id, rl_interval, ts, &g); + r = journal_ratelimit_group_acquire(groups_by_id, id, rl_interval, ts, &g); if (r < 0) return r; diff --git a/src/journal/journald-rate-limit.h b/src/journal/journald-rate-limit.h index 4539e174c3..566bba4be2 100644 --- a/src/journal/journald-rate-limit.h +++ b/src/journal/journald-rate-limit.h @@ -1,14 +1,13 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #pragma once +#include + +#include "hashmap.h" #include "time-util.h" -typedef struct JournalRateLimit JournalRateLimit; - -JournalRateLimit *journal_ratelimit_new(void); -void journal_ratelimit_free(JournalRateLimit *r); int journal_ratelimit_test( - JournalRateLimit *rl, + OrderedHashmap **groups_by_id, const char *id, usec_t rl_interval, unsigned rl_burst, diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c index b2203a87ff..1d03350d6a 100644 --- a/src/journal/journald-server.c +++ b/src/journal/journald-server.c @@ -1274,7 +1274,7 @@ void server_dispatch_message( (void) server_determine_space(s, &available, /* limit= */ NULL); rl = journal_ratelimit_test( - s->ratelimit, + &s->ratelimit_groups_by_id, c->unit, c->log_ratelimit_interval, c->log_ratelimit_burst, @@ -2809,10 +2809,6 @@ int server_init(Server *s, const char *namespace) { if (r < 0) return r; - s->ratelimit = journal_ratelimit_new(); - if (!s->ratelimit) - return log_oom(); - r = cg_get_root_path(&s->cgroup_root); if (r < 0) return log_error_errno(r, "Failed to acquire cgroup root path: %m"); @@ -2913,8 +2909,7 @@ Server* server_free(Server *s) { safe_close(s->notify_fd); safe_close(s->forward_socket_fd); - if (s->ratelimit) - journal_ratelimit_free(s->ratelimit); + ordered_hashmap_free(s->ratelimit_groups_by_id); server_unmap_seqnum_file(s->seqnum, sizeof(*s->seqnum)); server_unmap_seqnum_file(s->kernel_seqnum, sizeof(*s->kernel_seqnum)); diff --git a/src/journal/journald-server.h b/src/journal/journald-server.h index 3878061d09..5138908996 100644 --- a/src/journal/journald-server.h +++ b/src/journal/journald-server.h @@ -14,7 +14,6 @@ typedef struct Server Server; #include "hashmap.h" #include "journal-file.h" #include "journald-context.h" -#include "journald-rate-limit.h" #include "journald-stream.h" #include "list.h" #include "prioq.h" @@ -108,7 +107,7 @@ struct Server { char *buffer; - JournalRateLimit *ratelimit; + OrderedHashmap *ratelimit_groups_by_id; usec_t sync_interval_usec; usec_t ratelimit_interval; unsigned ratelimit_burst; diff --git a/src/journal/test-journald-rate-limit.c b/src/journal/test-journald-rate-limit.c index 89d69bb950..a08fabab26 100644 --- a/src/journal/test-journald-rate-limit.c +++ b/src/journal/test-journald-rate-limit.c @@ -4,41 +4,37 @@ #include "tests.h" TEST(journal_ratelimit_test) { - JournalRateLimit *rl; + _cleanup_ordered_hashmap_free_ OrderedHashmap *rl = NULL; int r; - assert_se(rl = journal_ratelimit_new()); - for (unsigned i = 0; i < 20; i++) { - r = journal_ratelimit_test(rl, "hoge", USEC_PER_SEC, 10, LOG_DEBUG, 0); + r = journal_ratelimit_test(&rl, "hoge", USEC_PER_SEC, 10, LOG_DEBUG, 0); assert_se(r == (i < 10 ? 1 : 0)); - r = journal_ratelimit_test(rl, "foo", 10 * USEC_PER_SEC, 10, LOG_DEBUG, 0); + r = journal_ratelimit_test(&rl, "foo", 10 * USEC_PER_SEC, 10, LOG_DEBUG, 0); assert_se(r == (i < 10 ? 1 : 0)); } /* Different priority group with the same ID is not ratelimited. */ - assert_se(journal_ratelimit_test(rl, "hoge", USEC_PER_SEC, 10, LOG_INFO, 0) == 1); - assert_se(journal_ratelimit_test(rl, "foo", 10 * USEC_PER_SEC, 10, LOG_INFO, 0) == 1); + assert_se(journal_ratelimit_test(&rl, "hoge", USEC_PER_SEC, 10, LOG_INFO, 0) == 1); + assert_se(journal_ratelimit_test(&rl, "foo", 10 * USEC_PER_SEC, 10, LOG_INFO, 0) == 1); /* Still LOG_DEBUG is ratelimited. */ - assert_se(journal_ratelimit_test(rl, "hoge", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 0); - assert_se(journal_ratelimit_test(rl, "foo", 10 * USEC_PER_SEC, 10, LOG_DEBUG, 0) == 0); + assert_se(journal_ratelimit_test(&rl, "hoge", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 0); + assert_se(journal_ratelimit_test(&rl, "foo", 10 * USEC_PER_SEC, 10, LOG_DEBUG, 0) == 0); /* Different ID is not ratelimited. */ - assert_se(journal_ratelimit_test(rl, "quux", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 1); + assert_se(journal_ratelimit_test(&rl, "quux", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 1); usleep_safe(USEC_PER_SEC); /* The ratelimit is now expired (11 trials are suppressed, so the return value should be 12). */ - assert_se(journal_ratelimit_test(rl, "hoge", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 1 + 11); + assert_se(journal_ratelimit_test(&rl, "hoge", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 1 + 11); /* foo is still ratelimited. */ - assert_se(journal_ratelimit_test(rl, "foo", 10 * USEC_PER_SEC, 10, LOG_DEBUG, 0) == 0); + assert_se(journal_ratelimit_test(&rl, "foo", 10 * USEC_PER_SEC, 10, LOG_DEBUG, 0) == 0); /* Still other priority and/or other IDs are not ratelimited. */ - assert_se(journal_ratelimit_test(rl, "hoge", USEC_PER_SEC, 10, LOG_INFO, 0) == 1); - assert_se(journal_ratelimit_test(rl, "foo", 10 * USEC_PER_SEC, 10, LOG_INFO, 0) == 1); - assert_se(journal_ratelimit_test(rl, "quux", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 1); - - journal_ratelimit_free(rl); + assert_se(journal_ratelimit_test(&rl, "hoge", USEC_PER_SEC, 10, LOG_INFO, 0) == 1); + assert_se(journal_ratelimit_test(&rl, "foo", 10 * USEC_PER_SEC, 10, LOG_INFO, 0) == 1); + assert_se(journal_ratelimit_test(&rl, "quux", USEC_PER_SEC, 10, LOG_DEBUG, 0) == 1); } DEFINE_TEST_MAIN(LOG_INFO);