On 15/04/2026 12:23, Eelco Chaudron wrote:
External email: Use caution opening links or attachments
On 1 Apr 2026, at 11:13, Eli Britstein wrote:
From: Gaetan Rivet <[email protected]>
Add a reference-counted key-value map.
Duplicates take a reference on the original entry within the map,
leaving it in place. To be able to execute an entry creation after
determining whether it is already present or not in the map,
store relevant initialization and de-initialization functions.
Signed-off-by: Gaetan Rivet <[email protected]>
Co-authored-by: Eli Britstein <[email protected]>
Signed-off-by: Eli Britstein <[email protected]>
Thanks Eli/Gaetan for following up with the v3.
Find some comments below.
Cheers,
Eelco
---
lib/automake.mk | 2 +
lib/refmap.c | 485 ++++++++++++++++++
lib/refmap.h | 130 +++++
tests/automake.mk | 1 +
tests/library.at | 5 +
tests/test-refmap.c | 894 ++++++++++++++++++++++++++++++++++
utilities/checkpatch_dict.txt | 2 +
7 files changed, 1519 insertions(+)
create mode 100644 lib/refmap.c
create mode 100644 lib/refmap.h
create mode 100644 tests/test-refmap.c
diff --git a/lib/automake.mk b/lib/automake.mk
index c6e988906..cb6458b0d 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -323,6 +323,8 @@ lib_libopenvswitch_la_SOURCES = \
lib/rculist.h \
lib/reconnect.c \
lib/reconnect.h \
+ lib/refmap.c \
+ lib/refmap.h \
lib/rstp.c \
lib/rstp.h \
lib/rstp-common.h \
diff --git a/lib/refmap.c b/lib/refmap.c
new file mode 100644
index 000000000..c2c435238
--- /dev/null
+++ b/lib/refmap.c
@@ -0,0 +1,485 @@
+/*
+ * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES.
+ * All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * 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>
+
+#include "cmap.h"
+#include "hash.h"
+#include "fatal-signal.h"
Should be second include.
Ack
+#include "ovs-atomic.h"
+#include "ovs-thread.h"
+#include "refmap.h"
+#include "timeval.h"
+
+#include "openvswitch/list.h"
+#include "openvswitch/vlog.h"
+
+VLOG_DEFINE_THIS_MODULE(refmap);
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(600, 600);
+
+static struct ovs_mutex refmap_destroy_lock = OVS_MUTEX_INITIALIZER;
+static struct ovs_list refmap_destroy_list
+ OVS_GUARDED_BY(refmap_destroy_lock) =
+ OVS_LIST_INITIALIZER(&refmap_destroy_list);
+
+struct refmap {
+ struct cmap map;
+ struct ovs_mutex map_lock;
+ size_t key_size;
+ size_t value_size;
+ refmap_value_init value_init;
+ refmap_value_uninit value_uninit;
+ refmap_value_format value_format;
+ char *name;
+ struct ovs_list in_destroy_list;
+};
+
+struct refmap_node {
+ struct ovsrcu_node rcu_node;
+ /* CMAP related: */
+ struct cmap_node map_node;
+ uint32_t hash;
+ /* Content: */
+ struct ovs_refcount refcount;
+ /* Key, then Value follows. */
+};
+
+static void
+refmap_destroy__(struct refmap *rfm, bool global_destroy);
This could simple become:
+refmap_destroy__(struct refmap *, bool global_destroy);
Ack
+
+static void
+refmap_destroy_unregister_protected(struct refmap *rfm)
+ OVS_REQUIRES(refmap_destroy_lock)
...
+ if (global_destroy && leaks_detected) {
+ ovs_abort(-1, "Refmap values leak detected.");
Looke like none of the ovs_abort() messages end with a dot.
ovs_abort(-1, "Refmap values leak detected");
From the earlier discussion;
EC> Do we need to call value_uninit?
EB> Those nodes were leaks in the refmap, at this point they are destroyed as
OVS is stopping, it is not possible to call value_uninit without crashing.
EC> But is this not only true for 'global_destroy == true'. What about the
genuine refmap_destroy() call? If this remains the case, we should clearly
add this to the API documentation.
You chose not to call value_uninit() in the non-global_destroy case.
Should we document this more explicitly, or should we call ovs_assert()
to prevent this bad programming behavior?
* WARNING: The caller MUST ensure the map is empty before calling
* refmap_destroy(). If the map contains remaining elements, their
* values will NOT have value_uninit() called, which will leak any
* resources managed by those values (file descriptors, allocated
* memory, etc.). The node memory will be freed but resource cleanup
* will not occur.
The documentation I put is the same as appears in cmap. I'll take the
last paragraph to v4. As no assert in cmap, I prefer the same here.
+ }
+}
+
+void
+refmap_destroy(struct refmap *rfm)
+{
+ if (!rfm) {
+ return;
+ }
+
+ refmap_destroy_unregister(rfm);
+ refmap_destroy__(rfm, false);
We had an earlier discussion, where I suggested this should be done
under RCU. However you mentioned this is by design, and it's MT-unsafe.
We need to clearly document this with an example in the documentation
on how this should be synchronized. Something along the lines of:
refmap_destroy() MUST NOT be called while any other thread
might be accessing the refmap, even through MT-safe operations like
refmap_for_each() or refmap_try_ref().
Or make it RCU safe and avoid any headaches in the future :)
I enhanced the doc about it, with the last paragraph. An example seems
like an overkill.
I find it as a needless complication. I think it's clear enough as-is.
+}
+
+static size_t
+refmap_aligned_key_size(struct refmap *rfm)
+{
+ return ROUND_UP(rfm->key_size, 8);
+}
+
+static void *
+refmap_node_key(struct refmap_node *node)
+{
+ if (!node) {
+ return NULL;
+ }
+
+ return node + 1;
+}
+
+static void *
+refmap_node_value(struct refmap *rfm, struct refmap_node *node)
+{
+ if (!node) {
+ return NULL;
+ }
+
+ return ((char *) refmap_node_key(node)) + refmap_aligned_key_size(rfm);
+}
+
+static size_t
+refmap_node_total_size(struct refmap *rfm)
+{
+ return sizeof(struct refmap_node) +
+ refmap_aligned_key_size(rfm) + rfm->value_size;
+}
+
+static struct refmap_node *
+refmap_node_from_value(struct refmap *rfm, void *value)
+{
+ size_t offset = sizeof(struct refmap_node) + refmap_aligned_key_size(rfm);
+
+ if ((uintptr_t) value < offset) {
+ return NULL;
+ }
Add a new line, similar to refmap_node_value()
Ack
+ return (void *) (((char *) value) - offset);
+}
+
+static void
+log_node(struct refmap *rfm, const char *prefix, struct refmap_node *node)
+{
+ void *key, *value;
+ struct ds s;
+
+ if (OVS_LIKELY(VLOG_DROP_DBG(&rl) || !rfm->value_format)) {
+ return;
+ }
+
+ key = refmap_node_key(node);
+ value = refmap_node_value(rfm, node);
+
+ ds_init(&s);
+ ds_put_cstr(&s, ", '");
+ rfm->value_format(&s, key, value);
Isn't the format option callback optional? If not, the ovs_assert in
refmap_create() needs updating.
It is. there is no assert there.
+ ds_put_cstr(&s, "'");
+ VLOG_DBG("%s: %p %s, refcnt=%d%s", rfm->name, value, prefix,
The refcnt is unsigned, so "%u"
Why not use "%s: value=%p %s, refcnt=~%u, '%s'" and avoid the ds_put_cstr()
calls?
Note the value=%p as else it's just a random pointer, or maybe a bit cleaner,
like:
myrefmap[unref]: value=0xffffffere, refcnt=~1, 'k,v=eelco,redhat'
"%s[%s]: value=%p, refcnt=~%u, '%s'"
+ ovs_refcount_read(&node->refcount), ds_cstr(&s));
Should we include the refcount? As it might cause false security? The value
could be different than the value that is actually used in the function.
Maybe print it like "refcnt=~%d," so when people use the message for
debugging they notice the ~ which might trigger some additional thinking.
I took your format. the refcount value is important to debug. when
debugging, let's say a single thread, we want to be able to see all refs
are covered with unrefs and it behaves correctly.
+ ds_destroy(&s);
+}
+
+/* Increments 'refcount', but only if it is over one.
+ *
+ * Returns false if the refcount was zero or one.
+ * In refmap, the last reference is only used to synchronize between
+ * value init and uninit in case of contention. In such state, the
+ * object is not valid anymore for external readers, until the
+ * value_{init,uninit} critical section is completed.
+ */
+static inline bool
+refmap_refcount_try_ref_one(struct ovs_refcount *refcount)
I assume this is the attempt to avoid the add/delete raise callback.
As you mentioned this is not fixed, I'm not reviewing the 'complex'
reference count attempt. Not sure but could this maybe be fixed simpler
doing something like this (not tested or well thought trought):
This is the old code with some annotations::
static inline bool
ovs_refcount_unref_if_not_last(struct ovs_refcount *refcount)
{
unsigned int count;
atomic_read_explicit(&refcount->count, &count,
memory_order_acquire);
ovs_assert(count > 0);
while (count > 1) {
if (atomic_compare_exchange_weak_explicit(
&refcount->count, &count, count - 1,
memory_order_release, memory_order_relaxed)) {
return true;
}
}
return false;
}
bool
refmap_unref(struct refmap *rfm, void *value)
{
struct refmap_node *node;
unsigned int old_refcount;
if (!value) {
return false;
}
node = refmap_node_from_value(rfm, value);
if (!node) {
return false;
}
if (ovs_refcount_unref_if_not_last(&node->refcount)) {
return false;
}
ovs_mutex_lock(&rfm->map_lock);
old_refcount = ovs_refcount_unref(&node->refcount);
if (old_refcount == 1) {
/* We transitioned 1→0 under lock. Safe to cleanup. */
rfm->value_uninit(refmap_node_value(rfm, node));
cmap_remove(&rfm->map, &node->map_node, node->hash);
ovs_mutex_unlock(&rfm->map_lock);
ovsrcu_postpone_inline(free, node, rcu_node);
return true;
}
ovs_mutex_unlock(&rfm->map_lock);
return false;
}
I took it. It works (added coverage in test-refmap). Thanks!
+{
+ unsigned int count;
+
+ atomic_read_explicit(&refcount->count, &count, memory_order_relaxed);
+ do {
+ if (count <= 1) {
+ return false;
+ }
+ } while (!atomic_compare_exchange_weak_explicit(&refcount->count, &count,
+ count + 1,
+ memory_order_relaxed,
+ memory_order_relaxed));
+ return true;
+}
+
+void
+refmap_for_each(struct refmap *rfm,
+ void (*cb)(void *value, void *key, void *arg), void *arg)
From our earlier discussion:
EC> A macro for this would be more in line with existing implementations and
EC> avoids the requirement for a callback function for simple operations, i.e.,
EC> something like REFMAP_FOR_EACH().
EB> We want to call the cb only under a reference count that is internal in
EB> the refmap.
EB> We can ask the user to explicitly call ref/unref before/after the "cb"
EB> code, but I think it's better to have this as part of the
EB> infrastructure, with the downside of a "cb" code that is not inline with
EB> the loop.
EB> Do you have a suggestion how to do it in a macro?
I did some quick/dirty coding, and this is a potential diff (might need
some cleanup bit it looks nicer with the code inline):
diff --git a/lib/refmap.c b/lib/refmap.c
index c2c435238..86b5850e1 100644
--- a/lib/refmap.c
+++ b/lib/refmap.c
@@ -291,6 +291,38 @@ refmap_refcount_try_ref_one(struct ovs_refcount *refcount)
return true;
}
+/* Helper function for REFMAP_FOR_EACH macro.
+ * Advances to the next refmap entry, handling ref/unref automatically.
+ * Returns true if a value was found, false when iteration is complete. */
+bool
+refmap_iter_next(struct refmap_iter *iter, void **value, void **key)
+{
+ struct refmap_node *node;
+
+ /* Unref previous value if any. */
+ if (iter->prev_value) {
+ refmap_unref(iter->rfm, iter->prev_value);
+ iter->prev_value = NULL;
+ }
+
+ /* Initialize on first call (cursor.impl is NULL from zero-init). */
+ if (!iter->cursor.impl) {
+ iter->cursor = cmap_cursor_start(&iter->rfm->map);
+ }
+
+ /* Find next refable node. */
+ CMAP_CURSOR_FOR_EACH_CONTINUE (node, map_node, &iter->cursor) {
+ if (refmap_refcount_try_ref_one(&node->refcount)) {
+ *key = refmap_node_key(node);
+ *value = refmap_node_value(iter->rfm, node);
+ iter->prev_value = *value;
+ return true;
+ }
+ }
+
+ return false;
+}
+
void
refmap_for_each(struct refmap *rfm,
void (*cb)(void *value, void *key, void *arg), void *arg)
diff --git a/lib/refmap.h b/lib/refmap.h
index e41f1d888..2d7b6a265 100644
--- a/lib/refmap.h
+++ b/lib/refmap.h
@@ -23,7 +23,9 @@
#include <stddef.h>
#include <stdint.h>
+#include <stdbool.h>
+#include "cmap.h"
+
#include "openvswitch/dynamic-string.h"
/*
@@ -86,6 +88,31 @@ typedef void (*refmap_value_uninit)(void *value);
typedef struct ds *(*refmap_value_format)(struct ds *s, void *key,
void *value);
+/* Iterator context for REFMAP_FOR_EACH macro. */
+struct refmap_iter {
+ struct refmap *rfm;
+ struct cmap_cursor cursor;
+ void *prev_value;
+};
+
+/* Helper function for REFMAP_FOR_EACH macro.
+ * Advances to the next refmap entry, handling ref/unref automatically.
+ * Returns true if a value was found, false when iteration is complete. */
+bool refmap_iter_next(struct refmap_iter *, void **value, void **key);
+
+/* Iterates through all key-value pairs in REFMAP. Each iteration assigns
+ * VALUE (void *) and KEY (void *) to the current entry's value and key.
+ *
+ * The iterator holds a reference during each iteration, automatically
+ * calling refmap_unref() after the loop body completes.
+ *
+ * If you break out of the loop, then you need to manually call
+ * refmap_unref(REFMAP, VALUE) to release the reference. */
+#define REFMAP_FOR_EACH(VALUE, KEY, REFMAP) \
+ for (struct refmap_iter refmap_iter__ = { (REFMAP), {0}, NULL }; \
+ refmap_iter_next(&refmap_iter__, &(VALUE), &(KEY)); \
+ )
+
/* Allocate and return a map handle.
*
* The user must ensure the 'key' is fully initialized, including potential
diff --git a/tests/test-refmap.c b/tests/test-refmap.c
index 4639088bc..533df5407 100644
--- a/tests/test-refmap.c
+++ b/tests/test-refmap.c
@@ -126,20 +126,6 @@ struct check_refmap_ctx {
int count;
};
-static void
-check_refmap_cb(void *value_, void *key_, void *arg_)
-{
- struct check_refmap_ctx *ctx = arg_;
- struct key *key = key_;
-
- ovs_assert(key->idx < N);
- if (ctx->values) {
- ovs_assert(ctx->values[key->idx] == value_);
- }
-
- ctx->count++;
-}
-
static void
check_refmap(struct refmap *rfm, struct value **values, int n_expected)
{
@@ -148,8 +134,19 @@ check_refmap(struct refmap *rfm, struct value **values,
int n_expected)
.values = values,
.count = 0,
};
+ void *value, *key;
+
+ REFMAP_FOR_EACH (value, key, rfm) {
+ struct key *k = key;
+
+ ovs_assert(k->idx < N);
+ if (ctx.values) {
+ ovs_assert(ctx.values[k->idx] == value);
+ }
+
+ ctx.count++;
+ }
- refmap_for_each(rfm, check_refmap_cb, &ctx);
ovs_assert(ctx.count == n_expected);
}
@@ -161,25 +158,6 @@ struct iter_modify_ctx {
int unref_count;
};
-static void
-iter_ref_cb(void *value_, void *key_, void *arg_)
-{
- struct iter_modify_ctx *ctx = arg_;
- struct key *key = key_;
-
- ovs_assert(refmap_try_ref_value(ctx->rfm, value_));
- ctx->extra_refs[key->idx] = value_;
- ctx->ref_count++;
-}
-
-static void
-iter_unref_cb(void *value_, void *key_ OVS_UNUSED, void *arg_)
-{
- struct iter_modify_ctx *ctx = arg_;
-
- refmap_unref(ctx->rfm, value_);
- ctx->unref_count++;
-}
struct try_ref_race_ctx {
struct refmap *rfm;
@@ -187,14 +165,6 @@ struct try_ref_race_ctx {
atomic_bool stop;
};
-static void
-race_for_each_cb(void *value_, void *key_ OVS_UNUSED, void *arg_ OVS_UNUSED)
-{
- struct value *value = value_;
-
- ovs_assert(value->hdl);
-}
-
static void *
try_ref_racer(void *arg)
{
@@ -216,7 +186,13 @@ try_ref_racer(void *arg)
refmap_unref(ctx->rfm, value);
}
- refmap_for_each(ctx->rfm, race_for_each_cb, NULL);
+ void *iter_value, *iter_key;
+
+ REFMAP_FOR_EACH (iter_value, iter_key, ctx->rfm) {
+ struct value *v = iter_value;
+
+ ovs_assert(v->hdl);
+ }
}
return NULL;
@@ -269,6 +245,7 @@ run_check(struct ovs_cmdl_context *ctx OVS_UNUSED)
struct key keys[N];
struct refmap *rfm;
uint32_t args[N];
+ void *value, *key;
rfm = refmap_create("check-rfm", sizeof(struct key), sizeof(struct value),
value_init, value_uninit, value_format);
@@ -280,7 +257,6 @@ run_check(struct ovs_cmdl_context *ctx OVS_UNUSED)
struct arg arg = {
.ptr = &args[i],
};
- struct value *value;
keys[i].idx = i;
args[i] = i;
@@ -333,7 +309,15 @@ run_check(struct ovs_cmdl_context *ctx OVS_UNUSED)
im_ctx.keys = keys;
im_ctx.extra_refs = (struct value **) extra_refs;
memset(extra_refs, 0, sizeof extra_refs);
- refmap_for_each(rfm, iter_ref_cb, &im_ctx);
+
+ REFMAP_FOR_EACH (value, key, rfm) {
+ struct key *k = key;
+
+ ovs_assert(refmap_try_ref_value(im_ctx.rfm, value));
+ im_ctx.extra_refs[k->idx] = value;
+ im_ctx.ref_count++;
+ }
+
ovs_assert(im_ctx.ref_count == N);
for (int i = 0; i < N; i++) {
ovs_assert(extra_refs[i] == values[i]);
@@ -345,7 +329,12 @@ run_check(struct ovs_cmdl_context *ctx OVS_UNUSED)
/* Drop extra refs from within refmap_for_each callback. */
memset(&im_ctx, 0, sizeof im_ctx);
im_ctx.rfm = rfm;
- refmap_for_each(rfm, iter_unref_cb, &im_ctx);
+
+ REFMAP_FOR_EACH (value, key, rfm) {
+ refmap_unref(im_ctx.rfm, value);
+ im_ctx.unref_count++;
+ }
+
ovs_assert(im_ctx.unref_count == N);
for (int i = 0; i < N; i++) {
ovs_assert(refmap_value_refcount_read(rfm, values[i]) == 1);
I took it. It works. Thanks!
+ struct refmap_node *node;
+
+ CMAP_FOR_EACH (node, map_node, &rfm->map) {
+ void *value;
+
+ if (!refmap_refcount_try_ref_one(&node->refcount)) {
+ continue;
+ }
+ log_node(rfm, "foreach", node);
+ value = refmap_node_value(rfm, node);
+ cb(value, refmap_node_key(node), arg);
+ refmap_unref(rfm, value);
+ }
+}
+
+static uint32_t
+refmap_key_hash(const struct refmap *rfm, const void *key)
+{
+ return hash_bytes(key, rfm->key_size, 0);
+}
+
+static void *
+refmap_lookup_protected(struct refmap *rfm, void *key, uint32_t hash)
This should return the actual node struct, i.e. 'static struct refmap_node *'
Ack
+{
+ struct refmap_node *node;
+
+ CMAP_FOR_EACH_WITH_HASH_PROTECTED (node, map_node, hash, &rfm->map) {
+ if (!memcmp(key, refmap_node_key(node), rfm->key_size) &&
+ ovs_refcount_read(&node->refcount) > 1) {
+ return node;
+ }
+ }
+
+ return NULL;
+}
+
+static void *
+refmap_lookup(struct refmap *rfm, void *key, uint32_t hash)
This should return the actual node struct, i.e. 'static struct refmap_node *'
Ack
+{
+ struct refmap_node *node;
+
+ CMAP_FOR_EACH_WITH_HASH (node, map_node, hash, &rfm->map) {
+ if (!memcmp(key, refmap_node_key(node), rfm->key_size) &&
...
+
+bool
+refmap_try_ref_value(struct refmap *rfm, void *value)
+{
+ struct refmap_node *node;
+
+ if (!value) {
+ return false;
+ }
+
+ node = refmap_node_from_value(rfm, value);
+ if (!node || !refmap_refcount_try_ref_one(&node->refcount)) {
Should we log_node() the refmap_refcount_() failure here?
I don't see much value, but I can add it.
+ return false;
+ }
+
+ log_node(rfm, "try_ref_value", node);
+ return true;
+}
+
+bool
+refmap_unref(struct refmap *rfm, void *value)
+{
+ struct refmap_node *node;
...
+ * 7. refmap_unref(value);
+ * This is the last reference released. value_uninit is immediatelly
immediatelly -> immediately
Ack
+ * called, while the value memory is freed after RCU grace period.
+ *
+ * Thread safety
+ * =============
+ *
+ * MT-unsafe:
+ * * refmap_create
+ * * refmap_destroy
See earlier comment about refmap_destroy(). I would prefer to make this MT-safe,
maybe with something like this (not tested or thought trough):
static void refmap_destroy_rcu__(void *rfm_)
{
struct refmap *rfm = rfm_;
refmap_destroy__(rfm, false);
}
struct refmap {
atomic_bool destroyed; // Add this field
...
};
void refmap_destroy(struct refmap *rfm)
{
if (!rfm) {
return;
}
atomic_store(&rfm->destroyed, true); // Mark as destroyed
refmap_destroy_unregister(rfm);
ovsrcu_postpone(refmap_destroy_rcu__, rfm);
}
void *refmap_ref(struct refmap *rfm, const void *key, void *arg)
{
if (atomic_load(&rfm->destroyed)) {
return NULL;
}
// ... rest of existing code
}
Also need similar checks in refmap_try_ref() and possibly refmap_for_each().
Same as above. I think it over-complicates something that should be
clear enough.
+ *
+ * MT-safe:
+ * * refmap_for_each
+ * * refmap_ref
+ * * refmap_try_ref
+ * * refmap_try_ref_value
+ * * refmap_unref
+ *
+ */
+
+struct refmap;
+
+/* Called once on a newly created 'value', i.e. when the first
+ * reference is taken. */
+typedef int (*refmap_value_init)(void *value, void *arg);
+
+/* Called once on the last dereference to value. */
+typedef void (*refmap_value_uninit)(void *value);
+
+/* Format a (key, value, arg) tuple in 's'. This is an optional (can be NULL)
arg value is no longer present.
Ack
+ * callback, used for debug log purposes.
+ */
+typedef struct ds *(*refmap_value_format)(struct ds *s, void *key,
+ void *value);
+
+/* Allocate and return a map handle.
+ *
+ * The user must ensure the 'key' is fully initialized, including potential
+ * pad bytes.
+ */
+struct refmap *refmap_create(const char *name,
+ size_t key_size,
+ size_t value_size,
+ refmap_value_init value_init,
+ refmap_value_uninit value_uninit,
+ refmap_value_format value_format);
refmap_value_init,
refmap_value_uninit,
refmap_value_format);
No need to add variable names if it's clear what the argument is.
Ack
+
+/* Frees the map memory.
+ *
+ * The client is responsible for unreferencing any data previously held in
+ * the map. */
See earlier comment on adding a more explicit comment.
Ack
+void refmap_destroy(struct refmap *rfm);
Same as earlier, refmap_destroy(struct refmap *), same for all *rfm's below.
+
+/* refmap_try_ref takes a reference for the found value upon success.
+ * It's the user's responsibility to unref it. */
This help might need to be more general as it applies to all three functions
below.
+void *refmap_try_ref(struct refmap *rfm, void *key);
+void *refmap_ref(struct refmap *rfm, void *key, void *arg);
+bool refmap_try_ref_value(struct refmap *rfm, void *value);
New line here.
Ack
+void refmap_for_each(struct refmap *rfm,
+ void (*cb)(void *value, void *key, void *arg),
+ void *arg);
+/* The refmap_value_refcount_read() API requires the caller to hold a
+ * reference, so a returned value of 1 only indicates you were the sole owner
+ * at the moment of the read, but may no longer be by the time you receive the
+ * value. This makes it unsuitable for logic decisions and only useful for
+ * debug logging.
+ */
This help text should move down to the function definition.
Ack
+void *refmap_key_from_value(struct refmap *rfm, void *value);
+
+/* Return 'true' if it was the last 'value' dereference and
+ * 'value_uninit' has been called. */
+bool refmap_unref(struct refmap *rfm, void *value);
+
+unsigned int
+refmap_value_refcount_read(struct refmap *rfm, void *value);
+
+#endif /* REFMAP_H */
diff --git a/tests/automake.mk b/tests/automake.mk
index a9d972a86..a74e56454 100644
--- a/tests/automake.mk
+++ b/tests/automake.mk
@@ -502,6 +502,7 @@ tests_ovstest_SOURCES = \
tests/test-rcu.c \
tests/test-rculist.c \
tests/test-reconnect.c \
+ tests/test-refmap.c \
tests/test-rstp.c \
tests/test-sflow.c \
tests/test-sha1.c \
diff --git a/tests/library.at b/tests/library.at
index 449f15fd5..3662d488e 100644
--- a/tests/library.at
+++ b/tests/library.at
@@ -44,6 +44,11 @@ AT_CHECK([ovstest test-ccmap check 1], [0], [...
])
AT_CLEANUP
+AT_SETUP([refmap])
+AT_KEYWORDS([refmap])
+AT_CHECK([ovstest test-refmap check], [0], [])
+AT_CLEANUP
+
AT_SETUP([atomic operations])
AT_CHECK([ovstest test-atomic])
AT_CLEANUP
diff --git a/tests/test-refmap.c b/tests/test-refmap.c
new file mode 100644
index 000000000..4639088bc
--- /dev/null
+++ b/tests/test-refmap.c
I did not look at the test cases in details, but it looks like most
case are covered. However it might be nice to add a test case were
the value_init() function is failing. Also a test for NULL value_format()
would show the existing bug.
Removed the value_format in test-refmap. It is not used anyway. No bug
with it.
Added coverage for value_init failure.
@@ -0,0 +1,894 @@
+/*
+ * SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES.
+ * All rights reserved.
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * 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
...
+ aux->rfm = refmap_create("benchmark-rfm", sizeof(struct key),
+ sizeof(struct value), value_init, value_uninit,
+ value_format);
ovs_assert(!aux->rfm);
Ack (without the !)
Done for all refmap_create occurrences.
+
+ print_test_header(&aux->test);
+ ovs_barrier_block(&barrier_inner);
+ /* Init is done, worker can start preparing to work. */
...
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
