On 16 Apr 2026, at 14:16, Eli Britstein wrote:
> 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. ACK on having the additional documentation in v4. >>> + } >>> +} >>> + >>> +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. ACK. >> >>> +} >>> + >>> +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. My bad, I missed the '!rfm->value_format' above. >> >>> + 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. Thanks for taking the format. >>> + 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. I agree it should not happen in real life, but if it does for some reason it would be nice to have a log about 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. Let’s keep it as is; we can revisit if real use cases expose issues. >>> + * >>> + * 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
