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.
+
+ if (now - mt->trim_timeout_ms >= mt->last_active_ms) {
+ VLOG_INFO_RL(&rl, "Detected inactivity "
+ "(last active %lld ms ago): trimming memory",
+ now - mt->last_active_ms);
+ mt->recently_active = false;
+ return true;
+ }
+
+ return false;
+}
+
+void
+memory_trimmer_wait(struct memory_trimmer *mt)
+{
+ if (!mt->recently_active) {
+ return;
+ }
+ poll_timer_wait_until(mt->last_active_ms + mt->trim_timeout_ms);
+}
+
+void
+memory_trimmer_trim(struct memory_trimmer *mt OVS_UNUSED)
+{
+#if HAVE_DECL_MALLOC_TRIM
+ malloc_trim(0);
+#endif
+}
+
+void
+memory_trimmer_record_activity(struct memory_trimmer *mt)
+{
+ mt->last_active_ms = time_msec();
+ mt->recently_active = true;
+}
diff --git a/lib/memory-trim.h b/lib/memory-trim.h
new file mode 100644
index 0000000000..2bf30292f0
--- /dev/null
+++ b/lib/memory-trim.h
@@ -0,0 +1,33 @@
+/*
+ * 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.
+ */
+
+#ifndef MEMORY_TRIM_H
+#define MEMORY_TRIM_H 1
+
+#include <stdbool.h>
+#include <stdint.h>
+
+struct memory_trimmer;
+
+struct memory_trimmer *memory_trimmer_create(void);
+void memory_trimmer_destroy(struct memory_trimmer *);
+void memory_trimmer_set(struct memory_trimmer *, uint32_t trim_timeout_ms);
+bool memory_trimmer_run(struct memory_trimmer *);
+void memory_trimmer_wait(struct memory_trimmer *);
+void memory_trimmer_trim(struct memory_trimmer *);
+void memory_trimmer_record_activity(struct memory_trimmer *);
+
+#endif /* lib/memory-trim.h */
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 363e384bd1..d23993a551 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -308,7 +308,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
sbrec_address_set_by_name);
}
-void inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
+/* Returns true if the incremental processing ended up updating nodes. */
+bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
struct ovsdb_idl_txn *ovnsb_txn,
bool recompute) {
engine_init_run();
@@ -347,6 +348,7 @@ void inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
} else {
engine_set_force_recompute(false);
}
+ return engine_has_updated();
}
void inc_proc_northd_cleanup(void)
diff --git a/northd/inc-proc-northd.h b/northd/inc-proc-northd.h
index 1300253791..9b81c7ee03 100644
--- a/northd/inc-proc-northd.h
+++ b/northd/inc-proc-northd.h
@@ -8,7 +8,7 @@
void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
struct ovsdb_idl_loop *sb);
-void inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
+bool inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn,
struct ovsdb_idl_txn *ovnsb_txn,
bool recompute);
void inc_proc_northd_cleanup(void);
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3a575b02a5..081081f9e4 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -25,6 +25,7 @@
#include "inc-proc-northd.h"
#include "lib/ip-mcast-index.h"
#include "lib/mcast-group-index.h"
+#include "lib/memory-trim.h"
#include "memory.h"
#include "northd.h"
#include "ovs-numa.h"
@@ -727,6 +728,34 @@ run_idl_loop(struct ovsdb_idl_loop *idl_loop, const char
*name)
return txn;
}
+#define DEFAULT_NORTHD_TRIM_TO_MS 30000
+
+static void
+run_memory_trimmer(struct ovsdb_idl *ovnnb_idl, bool activity)
+{
+ static struct memory_trimmer *mt = NULL;
+
+ if (!mt) {
+ mt = memory_trimmer_create();
+ }
+
+ const struct nbrec_nb_global *nb = nbrec_nb_global_first(ovnnb_idl);
+ if (nb) {
+ memory_trimmer_set(mt, smap_get_uint(&nb->options,
+ "northd_trim_timeout",
+ DEFAULT_NORTHD_TRIM_TO_MS));
+ }
+
+ if (activity) {
+ memory_trimmer_record_activity(mt);
+ }
+
+ if (memory_trimmer_run(mt)) {
+ memory_trimmer_trim(mt);
+ }
+ memory_trimmer_wait(mt);
+}
+
int
main(int argc, char *argv[])
{
@@ -905,7 +934,8 @@ main(int argc, char *argv[])
if (ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
int64_t loop_start_time = time_wall_msec();
- inc_proc_northd_run(ovnnb_txn, ovnsb_txn, recompute);
+ bool activity = inc_proc_northd_run(ovnnb_txn, ovnsb_txn,
+ recompute);
recompute = false;
if (ovnsb_txn) {
check_and_add_supported_dhcp_opts_to_sb_db(
@@ -937,6 +967,7 @@ main(int argc, char *argv[])
"force recompute next time.");
recompute = true;
}
+ run_memory_trimmer(ovnnb_idl_loop.idl, activity);
} else {
/* Make sure we send any pending requests, e.g., lock. */
ovsdb_idl_loop_commit_and_wait(&ovnnb_idl_loop);
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 4cceef14e0..d14d83dec0 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -202,6 +202,15 @@
</p>
</column>
+ <column name="options" key="northd_trim_timeout">
+ <p>
+ When used, this configuration value specifies the time, in
+ milliseconds, since the last <code>ovn-northd</code> active operation
+ after which memory trimming is performed. By default this is set to
+ 30000 (30 seconds).
+ </p>
+ </column>
+
<column name="options" key="use_logical_dp_groups">
<p>
Note: This option is deprecated, the only behavior is to always
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev