Hi Dumitru,
just one small comment below.
On Thu, Dec 15, 2022 at 10:29 PM Dumitru Ceara <dce...@redhat.com> 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)
>
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.
> +{
> + 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;
> + }
> +
> + 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
>
>
With that comment addressed:
Acked-by: Ales Musil <amu...@redhat.com>
Thanks,
Ales
--
Ales Musil
Senior Software Engineer - OVN Core
Red Hat EMEA <https://www.redhat.com>
amu...@redhat.com IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
