When rte_hash_add_key_data() overwrites an existing key, the old data pointer is silently lost. With RCU-protected readers still potentially accessing the old data, the application has no safe way to free it.
When RCU is configured with a free_key_data_func callback, automatically enqueue the old data for deferred freeing via the RCU defer queue on overwrite. In SYNC mode, synchronize and call free_key_data_func directly. Cc: [email protected] Fixes: 769b2de7fb52 ("hash: implement RCU resources reclamation") Signed-off-by: Robin Jarry <[email protected]> --- app/test/test_hash.c | 134 +++++++++++++++++++++++++ doc/guides/rel_notes/release_26_03.rst | 7 ++ lib/hash/rte_cuckoo_hash.c | 38 ++++++- lib/hash/rte_hash.h | 8 +- 4 files changed, 184 insertions(+), 3 deletions(-) diff --git a/app/test/test_hash.c b/app/test/test_hash.c index 3fb3d96d0546..56a7779e09b9 100644 --- a/app/test/test_hash.c +++ b/app/test/test_hash.c @@ -2342,6 +2342,137 @@ test_hash_rcu_qsbr_dq_reclaim(void) return 0; } +static void *old_data; + +static void +test_hash_free_key_data_func(void *p __rte_unused, void *key_data) +{ + old_data = key_data; +} + +/* + * Test automatic RCU free on overwrite via rte_hash_add_key_data. + * - Create hash with RW_CONCURRENCY_LF and RCU QSBR in DQ mode + * with a free_key_data_func callback that increments a counter. + * - Register a pseudo reader thread. + * - Add key with data (void *)1. + * - Overwrite same key with data (void *)2 via rte_hash_add_key_data. + * - Report quiescent state, trigger reclamation. + * - Verify the free callback was called exactly once. + * - Delete the key, report quiescent state, reclaim again. + * - Verify the free callback was called a second time. + */ +static int +test_hash_rcu_qsbr_replace_auto_free(void) +{ + struct rte_hash_rcu_config rcu = { + .v = NULL, + .mode = RTE_HASH_QSBR_MODE_DQ, + .free_key_data_func = test_hash_free_key_data_func, + .key_data_ptr = NULL, + }; + struct rte_hash_parameters params = { + .name = "test_replace_auto_free", + .entries = 16, + .key_len = sizeof(uint32_t), + .hash_func = NULL, + .hash_func_init_val = 0, + .socket_id = SOCKET_ID_ANY, + .extra_flag = RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF, + }; + struct rte_hash *hash = NULL; + uint32_t key = 55; + int32_t status; + int ret = -1; + size_t sz; + + printf("\n# Running RCU replace auto-free test\n"); + + hash = rte_hash_create(¶ms); + if (hash == NULL) { + printf("hash creation failed\n"); + goto end; + } + + sz = rte_rcu_qsbr_get_memsize(RTE_MAX_LCORE); + rcu.v = rte_zmalloc(NULL, sz, RTE_CACHE_LINE_SIZE); + if (rcu.v == NULL) { + printf("RCU QSBR alloc failed\n"); + goto end; + } + status = rte_rcu_qsbr_init(rcu.v, RTE_MAX_LCORE); + if (status != 0) { + printf("RCU QSBR init failed\n"); + goto end; + } + + status = rte_hash_rcu_qsbr_add(hash, &rcu); + if (status != 0) { + printf("RCU QSBR add failed\n"); + goto end; + } + + /* Register pseudo reader */ + status = rte_rcu_qsbr_thread_register(rcu.v, 0); + if (status != 0) { + printf("RCU QSBR thread register failed\n"); + goto end; + } + rte_rcu_qsbr_thread_online(rcu.v, 0); + + old_data = NULL; + + /* Add key with data = (void *)1 */ + status = rte_hash_add_key_data(hash, &key, (void *)(uintptr_t)1); + if (status != 0) { + printf("failed to add key (status=%d)\n", status); + goto end; + } + + /* Overwrite same key with data = (void *)2 */ + status = rte_hash_add_key_data(hash, &key, (void *)(uintptr_t)2); + if (status != 0) { + printf("failed to overwrite key (status=%d)\n", status); + goto end; + } + + /* Reader quiescent and reclaim */ + rte_rcu_qsbr_quiescent(rcu.v, 0); + rte_hash_rcu_qsbr_dq_reclaim(hash, NULL, NULL, NULL); + + if (old_data != (void *)(uintptr_t)1) { + printf("old data should be 0x1 but is %p\n", old_data); + goto end; + } + + /* Delete the key */ + status = rte_hash_del_key(hash, &key); + if (status < 0) { + printf("failed to delete key (status=%d)\n", status); + goto end; + } + + /* Reader quiescent and reclaim again */ + rte_rcu_qsbr_quiescent(rcu.v, 0); + rte_hash_rcu_qsbr_dq_reclaim(hash, NULL, NULL, NULL); + + if (old_data != (void *)(uintptr_t)2) { + printf("old data should be 2 but is %p\n", old_data); + goto end; + } + + ret = 0; +end: + if (rcu.v != NULL) { + rte_rcu_qsbr_thread_offline(rcu.v, 0); + rte_rcu_qsbr_thread_unregister(rcu.v, 0); + } + rte_hash_free(hash); + rte_free(rcu.v); + + return ret; +} + /* * Do all unit and performance tests. */ @@ -2423,6 +2554,9 @@ test_hash(void) if (test_hash_rcu_qsbr_dq_reclaim() < 0) return -1; + if (test_hash_rcu_qsbr_replace_auto_free() < 0) + return -1; + return 0; } diff --git a/doc/guides/rel_notes/release_26_03.rst b/doc/guides/rel_notes/release_26_03.rst index b4499ec066f8..e772fd3cb9b7 100644 --- a/doc/guides/rel_notes/release_26_03.rst +++ b/doc/guides/rel_notes/release_26_03.rst @@ -106,6 +106,13 @@ New Features Added handling of the key combination Control+L to clear the screen before redisplaying the prompt. +* **Added automatic deferred free on hash data overwrite.** + + When RCU is configured with a ``free_key_data_func`` callback, + ``rte_hash_add_key_data`` now automatically defers freeing the old data + pointer on key overwrite via the RCU defer queue. + + Removed Items ------------- diff --git a/lib/hash/rte_cuckoo_hash.c b/lib/hash/rte_cuckoo_hash.c index 36b3b477a55b..69dcfdfc398a 100644 --- a/lib/hash/rte_cuckoo_hash.c +++ b/lib/hash/rte_cuckoo_hash.c @@ -75,6 +75,7 @@ EAL_REGISTER_TAILQ(rte_hash_tailq) struct __rte_hash_rcu_dq_entry { uint32_t key_idx; uint32_t ext_bkt_idx; + void *old_data; }; RTE_EXPORT_SYMBOL(rte_hash_find_existing) @@ -761,6 +762,28 @@ enqueue_slot_back(const struct rte_hash *h, sizeof(uint32_t)); } +/* + * When RCU is configured with a free function, auto-free the overwritten + * data pointer via RCU. + */ +static inline void +__rte_hash_rcu_auto_free_old_data(const struct rte_hash *h, void *d) +{ + struct __rte_hash_rcu_dq_entry rcu_dq_entry = { + .key_idx = EMPTY_SLOT, /* sentinel value for __hash_rcu_qsbr_free_resource */ + .old_data = d, + }; + + if (d == NULL || h->hash_rcu_cfg == NULL || h->hash_rcu_cfg->free_key_data_func == NULL) + return; + + if (h->dq == NULL || rte_rcu_qsbr_dq_enqueue(h->dq, &rcu_dq_entry) != 0) { + /* SYNC mode or enqueue failed in DQ mode */ + rte_rcu_qsbr_synchronize(h->hash_rcu_cfg->v, RTE_QSBR_THRID_INVALID); + h->hash_rcu_cfg->free_key_data_func(h->hash_rcu_cfg->key_data_ptr, d); + } +} + /* Search a key from bucket and update its data. * Writer holds the lock before calling this. */ @@ -770,6 +793,7 @@ search_and_update(const struct rte_hash *h, void *data, const void *key, { int i; struct rte_hash_key *k, *keys = h->key_store; + void *old_data; for (i = 0; i < RTE_HASH_BUCKET_ENTRIES; i++) { if (bkt->sig_current[i] == sig) { @@ -782,9 +806,12 @@ search_and_update(const struct rte_hash *h, void *data, const void *key, * variable. Release the application data * to the readers. */ - rte_atomic_store_explicit(&k->pdata, + old_data = rte_atomic_exchange_explicit(&k->pdata, data, rte_memory_order_release); + + __rte_hash_rcu_auto_free_old_data(h, old_data); + /* * Return index where key is stored, * subtracting the first dummy index @@ -1566,6 +1593,15 @@ __hash_rcu_qsbr_free_resource(void *p, void *e, unsigned int n) *((struct __rte_hash_rcu_dq_entry *)e); RTE_SET_USED(n); + + if (rcu_dq_entry.key_idx == EMPTY_SLOT) { + /* Overwrite case: free old data only, do not recycle slot */ + RTE_ASSERT(h->hash_rcu_cfg->free_key_data_func != NULL); + h->hash_rcu_cfg->free_key_data_func(h->hash_rcu_cfg->key_data_ptr, + rcu_dq_entry.old_data); + return; + } + keys = h->key_store; k = (struct rte_hash_key *) ((char *)keys + diff --git a/lib/hash/rte_hash.h b/lib/hash/rte_hash.h index f692e0868dcf..e33f0aea0f5e 100644 --- a/lib/hash/rte_hash.h +++ b/lib/hash/rte_hash.h @@ -226,7 +226,9 @@ rte_hash_max_key_id(const struct rte_hash *h); * Thread safety can be enabled by setting flag during * table creation. * If the key exists already in the table, this API updates its value - * with 'data' passed in this API. It is the responsibility of + * with 'data' passed in this API. If RCU is configured with a + * free_key_data_func callback, the old data is automatically + * deferred-freed via RCU. Otherwise, it is the responsibility of * the application to manage any memory associated with the old value. * The readers might still be using the old value even after this API * has returned. @@ -253,7 +255,9 @@ rte_hash_add_key_data(const struct rte_hash *h, const void *key, void *data); * Thread safety can be enabled by setting flag during * table creation. * If the key exists already in the table, this API updates its value - * with 'data' passed in this API. It is the responsibility of + * with 'data' passed in this API. If RCU is configured with a + * free_key_data_func callback, the old data is automatically + * deferred-freed via RCU. Otherwise, it is the responsibility of * the application to manage any memory associated with the old value. * The readers might still be using the old value even after this API * has returned. -- 2.53.0
