On 18 May 2026, at 13:27, 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.
Hi Eli and Gaetan, Going over this series, looking at the non-DPDK changes for now, waiting for David as he is/was reviewing them. Some comments on this patch below. //Eelco > Signed-off-by: Gaetan Rivet <[email protected]> > Co-authored-by: Eli Britstein <[email protected]> > Signed-off-by: Eli Britstein <[email protected]> > --- > lib/automake.mk | 2 + > lib/refmap.c | 494 +++++++++++++++ > lib/refmap.h | 158 +++++ > tests/automake.mk | 1 + > tests/library.at | 5 + > tests/test-refmap.c | 1107 +++++++++++++++++++++++++++++++++ > utilities/checkpatch_dict.txt | 2 + > 7 files changed, 1769 insertions(+) > create mode 100644 lib/refmap.c > create mode 100644 lib/refmap.h > create mode 100644 tests/test-refmap.c [...] > diff --git a/lib/refmap.h b/lib/refmap.h > new file mode 100644 > index 000000000..4c99b763e > --- /dev/null > +++ b/lib/refmap.h > @@ -0,0 +1,158 @@ > +/* > + * Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. > + * > + * 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 REFMAP_H > +#define REFMAP_H > + > +#include <config.h> <config.h> is included in a header file. It should only be included in .c files, not in headers, per the OVS coding guidelines. > +#include <stdbool.h> > +#include <stddef.h> > +#include <stdint.h> > + > +#include "cmap.h" > + > +#include "openvswitch/dynamic-string.h" > + > +/* > + * Reference map > + * ============= > + * > + * This key-value store acts like a regular concurrent hashmap, > + * except that insertion takes a reference on the value if already > + * present. > + * The key provided must be fully initialized, including potential pad bytes. > + * > + * As the value creation is dependent on it being already present > + * within the structure and the user cannot predict that, this structure > + * requires definitions for value_init and value_uninit functions, > + * that will be called only at creation (first reference taken) and > + * destruction (last reference released). > + * > + * Example: > + * 1. struct key key; > + * 2. memset(&key, 0, sizeof key); > + * 3. refmap_create() > + * 4. value = refmap_ref(key); > + * Since it's the first reference for <key>, value_init is called. > + * 5. refmap_ref(key); > + * This is not the first reference for <key>. Only ref-count is updated. > + * 6. refmap_unref(value); > + * This is not the last reference released. Only ref-count is updated. > + * 7. refmap_unref(value); > + * This is the last reference released. value_uninit is immediately > + * called, while the value memory is freed after RCU grace period. > + * > + * Thread safety > + * ============= > + * > + * MT-unsafe: > + * * refmap_create > + * * refmap_destroy > + * > + * 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); Both callbacks are invoked while holding the internal map_lock mutex. This means value_init and value_uninit must not call back into any refmap API on the same map. Should we add a comment about this? A second undocumented ordering constraint exists between value_init and value_uninit for the same key. When a node's refcount reaches zero, refmap_lookup_protected() filters it out (refcount > 0 check at line 334) before the lock is released and value_uninit() runs. This means a concurrent refmap_ref() for the same key can allocate a new node and call value_init() on it while the dying node is still in the map and its value_uninit() has not yet run. The two calls operate on separate memory, so there is no memory corruption, but if value_init and value_uninit manage an external resource tied to the key (a hardware table entry, a file descriptor, or similar), value_init() for the new entry can complete before value_uninit() releases the old entry for the same key. Whether this causes a conflict depends entirely on the callbacks. Should we document this constraint, or should we fix it? > +/* Format a (key, value) tuple in 's'. This is an optional (can be NULL) > + * callback, used for debug log purposes. > + */ > +typedef struct ds *(*refmap_value_format)(struct ds *s, void *key, > + void *value); _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
