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

[Ilya Maximets]

On 5/25/24 12:18, wushao...@chinatelecom.cn wrote:
> From: Shaohua Wu 
> 
> 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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

[wushaohua]

From: Shaohua Wu 

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(>refcount);
node->is_hotupgrade = false;
frozen_state_clone(CONST_CAST(struct frozen_state *, >state), state);
struct recirc_id_node *hash_recirc_node = recirc_find_equal(>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();
...
}

Log recording:
2024-04-27T12:28:47.973Z|6|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|9|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|6|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|4|ofproto_dpif_rid(pmd-c09/id:12)|INFO|node->id:28,hash:4019648042,table_id:75
2024-04-27T12:28:47.973Z|7|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|5|ofproto_dpif_rid(pmd-c09/id:12)|INFO|node->id:29,hash:3800776147,table_id:30
2024-04-27T12:28:48.101Z|7|ofproto_dpif_rid(pmd-c03/id:14)|INFO|node->id:30,hash:1580334976,table_id:75

Signed-off-by: Shaohua Wu 

---
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 
---
 ofproto/ofproto-dpif-rid.c | 37 ++---
 1 file changed, 34 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
index f01468025..81f05 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(>refcount);
+frozen_state_clone(CONST_CAST(struct frozen_state *, >state), state);
+
+ovs_mutex_lock();
+find_node = recirc_ref_equal(state, hash);
+if (find_node) {
+ovs_mutex_unlock();
+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(_map, >id_node, node->id);
+cmap_insert(_map, >metadata_node, node->hash);
+ovs_mutex_unlock();
 return node->id;
 }
 
-- 
2.30.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev