On 1/12/23 20:26, Mark Michelson wrote:
> Thanks for this Dumitru,
>
> Acked-by: Mark Michelson <mmichel...@redhat.com>
>
> I have one note down below, however I don't think it's critical enough
> not to ack the patch.
>
> On 12/15/22 16:25, Dumitru Ceara 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 <dce...@redhat.com>
>> ---
>> 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)
>> +{
>> + if (!mt->recently_active) {
>> + return false;
>> + }
>> +
>> + long long int now = time_msec();
>> + if (now < mt->last_active_ms || now < mt->trim_timeout_ms) {
>> + VLOG_WARN_RL(&rl, "Detected last active timestamp overflow");
>> + mt->recently_active = false;
>> + return true;
>> + }
>> +
>> + if (now < mt->trim_timeout_ms) {
>> + VLOG_WARN_RL(&rl, "Detected very large trim timeout: "
>> + "now %lld ms timeout %"PRIu32" ms",
>> + now, mt->trim_timeout_ms);
>> + return false;
>> + }
>
> I realize this was copied directly and the code is unchanged. However, I
> think it's impossible for the block above to run. The reason is that if
> now < mt->trim_timeout_ms, then the previous block (if (now <
> mt->last_active_ms || now < mt->trim_timeout_ms)) will handle it.
>
You're right, I removed this block and applied the patch to main branch,
thanks!
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
