diff mbox series

[ovs-dev,v3] ofproto-dpif-rid: Fix duplicate entries.

Message ID 20240525101814.1952081-1-wushaohua@chinatelecom.cn
State Changes Requested
Headers show
Series [ovs-dev,v3] ofproto-dpif-rid: Fix duplicate entries. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

wushaohua@chinatelecom.cn May 25, 2024, 10:18 a.m. UTC 
From: Shaohua Wu <wushaohua@chinatelecom.cn>

In scenarios with multiple PMDs, there may be
simultaneous requests for recirc_id from multiple
PMD threads.In recirc_alloc_id_ctx, we first check
if there is a duplicate entry in the metadata_map
for the same frozen_state field. If successful,
we directly retrieve the recirc_id. If unsuccessful,
we create a new recirc_node and insert it into
id_map and metadata_map. There is no locking mechanism
to prevent the possibility of two threads with the same
state simultaneously inserting, meaning their IDs are
different, but their frozen_states are the same.

trace log:
static struct recirc_id_node *
recirc_alloc_id__(const struct frozen_state *state, uint32_t hash)
{
    ovs_assert(state->action_set_len <= state->ofpacts_len);

    struct recirc_id_node *node = xzalloc(sizeof *node);

    node->hash = hash;
    ovs_refcount_init(&node->refcount);
    node->is_hotupgrade = false;
    frozen_state_clone(CONST_CAST(struct frozen_state *, &node->state), state);
    struct recirc_id_node *hash_recirc_node = recirc_find_equal(&node->state, state);
    if (hash_recirc_node) {
        VLOG_INFO("wsh:hash equal:hash_recirc_node %p id:%u, hash_recirc_node hash:%u,node %p hash:%u\n",
                   hash_recirc_node， hash_recirc_node->id, hash_recirc_node->hash, node, node->hash);
    }
    ovs_mutex_lock(&mutex);
    ...
}

Log recording:
2024-04-27T12:28:47.973Z|00006|ofproto_dpif_rid(pmd-c08/id:13)|INFO|wsh:hash equal:
hash_recirc_node 0x7fb29c0276a0 id:27,hash_recirc_node hash:3224122528,node 0x7fb2900194d0 hash:3224122528
2024-04-27T12:28:47.973Z|00009|ofproto_dpif_rid(pmd-c02/id:15)|INFO|wsh:hash equal:
hash_recirc_node 0x7fb29c0276a0 id:27,hash_recirc_node hash:3224122528,node 0x7fb288025270 hash:3224122528
2024-04-27T12:28:47.973Z|00006|ofproto_dpif_rid(pmd-c03/id:14)|INFO|wsh:hash equal:
hash_recirc_node 0x7fb29c0276a0 id:27,hash_recirc_node hash:3224122528,node 0x7fb29401d4e0 hash:3224122528
2024-04-27T12:28:47.973Z|00004|ofproto_dpif_rid(pmd-c09/id:12)|INFO|node->id:28,hash:4019648042,table_id:75
2024-04-27T12:28:47.973Z|00007|ofproto_dpif_rid(pmd-c08/id:13)|INFO|wsh:hash equal:
hash_recirc_node 0x7fb29c028d40 id:28,hash_recirc_node hash:4019648042,node 0x7fb29001ac30 hash:4019648042
2024-04-27T12:28:48.065Z|00005|ofproto_dpif_rid(pmd-c09/id:12)|INFO|node->id:29,hash:3800776147,table_id:30
2024-04-27T12:28:48.101Z|00007|ofproto_dpif_rid(pmd-c03/id:14)|INFO|node->id:30,hash:1580334976,table_id:75

Signed-off-by: Shaohua Wu <wushaohua@chinatelecom.cn>

---
v1->v2:modify log recording , add trace code.
v2->v3:Optimize recirc_alloc_id_ctx. The recirc_alloc_id
       interface is used for bond configuration and must
       obtain a unique recirc_id.

Signed-off-by: wushaohua <wushaohua@chinatelecom.cn>
---
 ofproto/ofproto-dpif-rid.c | 37 ++++++++++++++++++++++++++++++++++---
 1 file changed, 34 insertions(+), 3 deletions(-)

Comments

Ilya Maximets May 28, 2024, 5:23 p.m. UTC | #1 
On 5/25/24 12:18, wushaohua@chinatelecom.cn wrote:
> From: Shaohua Wu <wushaohua@chinatelecom.cn>
> 
> In scenarios with multiple PMDs, there may be
> simultaneous requests for recirc_id from multiple
> PMD threads.In recirc_alloc_id_ctx, we first check
> if there is a duplicate entry in the metadata_map
> for the same frozen_state field. If successful,
> we directly retrieve the recirc_id. If unsuccessful,
> we create a new recirc_node and insert it into
> id_map and metadata_map. There is no locking mechanism
> to prevent the possibility of two threads with the same
> state simultaneously inserting, meaning their IDs are
> different, but their frozen_states are the same.

Hi, Shaohua Wu.  Could you, please, explain why having multiple IDs
allocated for the same state is a problem?  This may create a few
more datapath flows, but should not cause any correctness issues, as
Simon pointed out previously.  The change you're proposing may have
some performance impact as we'll be holding the shared mutex for
longer while allocating IDs, so I'd like to understand why it is a
problem before making the change.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index f01468025..eeee81f05 100644
--- a/ofproto/ofproto-dpif-rid.c
+++ b/ofproto/ofproto-dpif-rid.c
@@ -277,11 +277,42 @@  recirc_find_id(const struct frozen_state *target)
 uint32_t
 recirc_alloc_id_ctx(const struct frozen_state *state)
 {
+    ovs_assert(state->action_set_len <= state->ofpacts_len);
+    struct recirc_id_node *node = NULL;
+    struct recirc_id_node *find_node = NULL;
     uint32_t hash = frozen_state_hash(state);
-    struct recirc_id_node *node = recirc_ref_equal(state, hash);
-    if (!node) {
-        node = recirc_alloc_id__(state, hash);
+
+    node = xzalloc(sizeof *node);
+    node->hash = hash;
+    ovs_refcount_init(&node->refcount);
+    frozen_state_clone(CONST_CAST(struct frozen_state *, &node->state), state);
+
+    ovs_mutex_lock(&mutex);
+    find_node = recirc_ref_equal(state, hash);
+    if (find_node) {
+        ovs_mutex_unlock(&mutex);
+        recirc_id_node_free(node);
+        return find_node->id;
     }
+
+    for (;;) {
+        /* Claim the next ID.  The ID space should be sparse enough for the
+           allocation to succeed at the first try.  We do skip the first
+           RECIRC_POOL_STATIC_IDS IDs on the later rounds, though, as some of
+           the initial allocations may be for long term uses (like bonds). */
+        node->id = next_id++;
+        if (OVS_UNLIKELY(!node->id)) {
+            next_id = RECIRC_POOL_STATIC_IDS + 1;
+            node->id = next_id++;
+        }
+        /* Find if the id is free. */
+        if (OVS_LIKELY(!recirc_find__(node->id))) {
+            break;
+        }
+    }
+    cmap_insert(&id_map, &node->id_node, node->id);
+    cmap_insert(&metadata_map, &node->metadata_node, node->hash);
+    ovs_mutex_unlock(&mutex);
     return node->id;
 }