On 1/13/23 12:02, Ales Musil wrote: > Hi Dumitru, > just one small comment below. > > On Thu, Dec 15, 2022 at 10:29 PM Dumitru Ceara <[email protected]> wrote: > >> To do this we factor the memory trimming code out into its own module, >> memory-trim. We use this now for both the lflow cache (in >> ovn-controller) and for ovn-northd. >> >> Signed-off-by: Dumitru Ceara <[email protected]> >> --- >> controller/lflow-cache.c | 68 +++++--------------------- >> lib/automake.mk | 2 + >> lib/memory-trim.c | 120 >> ++++++++++++++++++++++++++++++++++++++++++++++ >> lib/memory-trim.h | 33 +++++++++++++ >> northd/inc-proc-northd.c | 4 +- >> northd/inc-proc-northd.h | 2 - >> northd/ovn-northd.c | 33 ++++++++++++- >> ovn-nb.xml | 9 +++ >> 8 files changed, 212 insertions(+), 59 deletions(-) >> create mode 100644 lib/memory-trim.c >> create mode 100644 lib/memory-trim.h >> >> diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c >> index 9fca2d7441..9d0e9cd10b 100644 >> --- a/controller/lflow-cache.c >> +++ b/controller/lflow-cache.c >> @@ -24,10 +24,9 @@ >> #include "coverage.h" >> #include "lflow-cache.h" >> #include "lib/uuid.h" >> -#include "openvswitch/poll-loop.h" >> +#include "memory-trim.h" >> #include "openvswitch/vlog.h" >> #include "ovn/expr.h" >> -#include "timeval.h" >> >> VLOG_DEFINE_THIS_MODULE(lflow_cache); >> >> @@ -54,6 +53,7 @@ static struct vlog_rate_limit rl = >> VLOG_RATE_LIMIT_INIT(5, 1); >> >> struct lflow_cache { >> struct hmap entries[LCACHE_T_MAX]; >> + struct memory_trimmer *mt; >> uint32_t n_entries; >> uint32_t high_watermark; >> uint32_t capacity; >> @@ -61,10 +61,7 @@ struct lflow_cache { >> uint64_t max_mem_usage; >> uint32_t trim_limit; >> uint32_t trim_wmark_perc; >> - uint32_t trim_timeout_ms; >> uint64_t trim_count; >> - long long int last_active_ms; >> - bool recently_active; >> bool enabled; >> }; >> >> @@ -84,7 +81,6 @@ static struct lflow_cache_value *lflow_cache_add__( >> static void lflow_cache_delete__(struct lflow_cache *lc, >> struct lflow_cache_entry *lce); >> static void lflow_cache_trim__(struct lflow_cache *lc, bool force); >> -static void lflow_cache_record_activity__(struct lflow_cache *lc); >> >> struct lflow_cache * >> lflow_cache_create(void) >> @@ -94,6 +90,7 @@ lflow_cache_create(void) >> for (size_t i = 0; i < LCACHE_T_MAX; i++) { >> hmap_init(&lc->entries[i]); >> } >> + lc->mt = memory_trimmer_create(); >> >> return lc; >> } >> @@ -127,6 +124,7 @@ lflow_cache_destroy(struct lflow_cache *lc) >> for (size_t i = 0; i < LCACHE_T_MAX; i++) { >> hmap_destroy(&lc->entries[i]); >> } >> + memory_trimmer_destroy(lc->mt); >> free(lc); >> } >> >> @@ -146,13 +144,6 @@ lflow_cache_enable(struct lflow_cache *lc, bool >> enabled, uint32_t capacity, >> trim_wmark_perc = 100; >> } >> >> - if (trim_timeout_ms < 1000) { >> - VLOG_WARN_RL(&rl, "Invalid requested trim timeout: " >> - "requested %"PRIu32" ms, using 1000 ms instead", >> - trim_timeout_ms); >> - trim_timeout_ms = 1000; >> - } >> - >> uint64_t max_mem_usage = max_mem_usage_kb * 1024; >> bool need_flush = false; >> bool need_trim = false; >> @@ -172,13 +163,13 @@ lflow_cache_enable(struct lflow_cache *lc, bool >> enabled, uint32_t capacity, >> lc->max_mem_usage = max_mem_usage; >> lc->trim_limit = lflow_trim_limit; >> lc->trim_wmark_perc = trim_wmark_perc; >> - lc->trim_timeout_ms = trim_timeout_ms; >> + memory_trimmer_set(lc->mt, trim_timeout_ms); >> >> if (need_flush) { >> - lflow_cache_record_activity__(lc); >> + memory_trimmer_record_activity(lc->mt); >> lflow_cache_flush(lc); >> } else if (need_trim) { >> - lflow_cache_record_activity__(lc); >> + memory_trimmer_record_activity(lc->mt); >> lflow_cache_trim__(lc, false); >> } >> } >> @@ -286,7 +277,7 @@ lflow_cache_delete(struct lflow_cache *lc, const >> struct uuid *lflow_uuid) >> lflow_cache_delete__(lc, CONTAINER_OF(lcv, struct >> lflow_cache_entry, >> value)); >> lflow_cache_trim__(lc, false); >> - lflow_cache_record_activity__(lc); >> + memory_trimmer_record_activity(lc->mt); >> } >> } >> >> @@ -327,41 +318,15 @@ lflow_cache_get_memory_usage(const struct >> lflow_cache *lc, struct simap *usage) >> void >> lflow_cache_run(struct lflow_cache *lc) >> { >> - if (!lc->recently_active) { >> - return; >> - } >> - >> - long long int now = time_msec(); >> - if (now < lc->last_active_ms || now < lc->trim_timeout_ms) { >> - VLOG_WARN_RL(&rl, "Detected cache last active timestamp >> overflow"); >> - lc->recently_active = false; >> - lflow_cache_trim__(lc, true); >> - return; >> - } >> - >> - if (now < lc->trim_timeout_ms) { >> - VLOG_WARN_RL(&rl, "Detected very large trim timeout: " >> - "now %lld ms timeout %"PRIu32" ms", >> - now, lc->trim_timeout_ms); >> - return; >> - } >> - >> - if (now - lc->trim_timeout_ms >= lc->last_active_ms) { >> - VLOG_INFO_RL(&rl, "Detected cache inactivity " >> - "(last active %lld ms ago): trimming cache", >> - now - lc->last_active_ms); >> + if (memory_trimmer_run(lc->mt)) { >> lflow_cache_trim__(lc, true); >> - lc->recently_active = false; >> } >> } >> >> void >> lflow_cache_wait(struct lflow_cache *lc) >> { >> - if (!lc->recently_active) { >> - return; >> - } >> - poll_timer_wait_until(lc->last_active_ms + lc->trim_timeout_ms); >> + memory_trimmer_wait(lc->mt); >> } >> >> static struct lflow_cache_value * >> @@ -388,7 +353,7 @@ lflow_cache_add__(struct lflow_cache *lc, const struct >> uuid *lflow_uuid, >> } >> } >> >> - lflow_cache_record_activity__(lc); >> + memory_trimmer_record_activity(lc->mt); >> lc->mem_usage += size; >> >> COVERAGE_INC(lflow_cache_add); >> @@ -447,17 +412,8 @@ lflow_cache_trim__(struct lflow_cache *lc, bool force) >> hmap_shrink(&lc->entries[i]); >> } >> >> -#if HAVE_DECL_MALLOC_TRIM >> - malloc_trim(0); >> -#endif >> + memory_trimmer_trim(lc->mt); >> >> lc->high_watermark = lc->n_entries; >> lc->trim_count++; >> } >> - >> -static void >> -lflow_cache_record_activity__(struct lflow_cache *lc) >> -{ >> - lc->last_active_ms = time_msec(); >> - lc->recently_active = true; >> -} >> diff --git a/lib/automake.mk b/lib/automake.mk >> index 15d4f84bbb..b69e854b0b 100644 >> --- a/lib/automake.mk >> +++ b/lib/automake.mk >> @@ -30,6 +30,8 @@ lib_libovn_la_SOURCES = \ >> lib/mac-binding-index.h \ >> lib/mcast-group-index.c \ >> lib/mcast-group-index.h \ >> + lib/memory-trim.c \ >> + lib/memory-trim.h \ >> lib/lex.c \ >> lib/objdep.c \ >> lib/objdep.h \ >> diff --git a/lib/memory-trim.c b/lib/memory-trim.c >> new file mode 100644 >> index 0000000000..a6791073ee >> --- /dev/null >> +++ b/lib/memory-trim.c >> @@ -0,0 +1,120 @@ >> +/* >> + * Copyright (c) 2022, Red Hat, Inc. >> + * >> + * Licensed under the Apache License, Version 2.0 (the "License"); >> + * you may not use this file except in compliance with the License. >> + * You may obtain a copy of the License at: >> + * >> + * http://www.apache.org/licenses/LICENSE-2.0 >> + * >> + * Unless required by applicable law or agreed to in writing, software >> + * distributed under the License is distributed on an "AS IS" BASIS, >> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >> implied. >> + * See the License for the specific language governing permissions and >> + * limitations under the License. >> + */ >> + >> + >> +#include <config.h> >> + >> +#if HAVE_DECL_MALLOC_TRIM >> +#include <malloc.h> >> +#endif >> + >> +#include "memory-trim.h" >> + >> +#include "openvswitch/poll-loop.h" >> +#include "openvswitch/vlog.h" >> +#include "timeval.h" >> + >> +VLOG_DEFINE_THIS_MODULE(memory_trim); >> + >> +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); >> + >> +struct memory_trimmer { >> + uint32_t trim_timeout_ms; >> + long long int last_active_ms; >> + bool recently_active; >> +}; >> + >> +struct memory_trimmer * >> +memory_trimmer_create(void) >> +{ >> + struct memory_trimmer *mt = xzalloc(sizeof *mt); >> + return mt; >> +} >> + >> +void >> +memory_trimmer_destroy(struct memory_trimmer *mt) >> +{ >> + free(mt); >> +} >> + >> +void >> +memory_trimmer_set(struct memory_trimmer *mt, uint32_t trim_timeout_ms) >> +{ >> + if (trim_timeout_ms < 1000) { >> + VLOG_WARN_RL(&rl, "Invalid requested trim timeout: " >> + "requested %"PRIu32" ms, using 1000 ms instead", >> + trim_timeout_ms); >> + trim_timeout_ms = 1000; >> + } >> + mt->trim_timeout_ms = trim_timeout_ms; >> +} >> + >> +/* Returns true if trimming due to inactivity should happen. */ >> +bool >> +memory_trimmer_run(struct memory_trimmer *mt) >> > > IMO this name does not really reflect what the function does. > I would suggest changing it to > memory_trimmer_should_run/memory_trimmer_can_run. > Something along those lines so it is clear that this function does not trim > anything. >
You're right, I changed this to memory_trimmer_can_run() before applying it to the main branch. Thanks for the review! Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
