Message ID | 20240525101814.1952081-1-wushaohua@chinatelecom.cn |
---|---|
State | Changes Requested |
Headers | show |
Series | [ovs-dev,v3] ofproto-dpif-rid: Fix duplicate entries. | expand |
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 |
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 --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; }