Hi Eelco, Thank you for your feedback on the patch. I've made the requested changes:
1. Moved variable declarations to the function definition section at the beginning; 2. Replaced the separate recirc_find_equal() and ovs_refcount_try_ref_rcu() calls with recirc_ref_equal(); 3. Improved variable naming by renaming 'old_node' to 'existing_node' for better clarity; 4. Added explicit NULL initialization for variables as a good practice; The updated patch implements the same functionality but with cleaner code structure that follows the project's coding style guidelines. I will submit a new patch to update this old one. Thanks again for your review! Cheers, Sunyang -----邮件原件----- 发件人: Eelco Chaudron <[email protected]> 发送时间: 2025年11月11日 18:31 收件人: Sunyang Wu <[email protected]> 抄送: [email protected]; Bin Shen <[email protected]>; [email protected] 主题: Re: [ovs-dev] [PATCH] recirc: Optimize node reuse logic in recirc ID allocation process On 11 Nov 2025, at 7:40, Sunyang Wu via dev wrote: > In the recirc_alloc_id__ function, when attempting to allocate a > recirc ID for new flow metadata, first check if a node with the same > state already exists. If a matching node is found, safely acquire a > reference count using ovs_refcount_try_ref_rcu() to avoid potential > reference counting race conditions in concurrent environments. > > This optimization prevents duplicate creation of nodes with identical > states, improves resource utilization, and ensures correctness in > multi-threaded environments through RCU-safe reference counting operations. Hi Sunyang, Thanks for the patch! I haven’t had a chance to look closely to determine if this is a valid change, but I do have one comment for now. Cheers, Eelco PS: Also check the robot’s comments. > Signed-off-by: Bin Shen <[email protected]> > Signed-off-by: Sunyang Wu <[email protected]> > --- > ofproto/ofproto-dpif-rid.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c > index f01468025..a9b691ad7 100644 > --- a/ofproto/ofproto-dpif-rid.c > +++ b/ofproto/ofproto-dpif-rid.c > @@ -241,6 +241,13 @@ recirc_alloc_id__(const struct frozen_state *state, > uint32_t hash) > frozen_state_clone(CONST_CAST(struct frozen_state *, > &node->state), state); > > ovs_mutex_lock(&mutex); > + struct recirc_id_node *old_node; This should go to the definition part; struct recirc_id_node *node = xzalloc(sizeof *node); + struct recirc_id_node *old_node; > + old_node = recirc_find_equal(state, hash); > + if (old_node && ovs_refcount_try_ref_rcu(&old_node->refcount)) { > + xfree(node); > + ovs_mutex_unlock(&mutex); > + return old_node; > + } Could part of this code be replaced with recirc_ref_equal()? > 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 > -- > 2.19.0.rc0.windows.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
