On 11/12/25 3:34 AM, 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, reuse it by leveraging the existing > recirc_ref_equal function to safely acquire a reference count. > > 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. > > Signed-off-by: Sunyang Wu <[email protected]> > Signed-off-by: Bin Shen <[email protected]> > --- > ofproto/ofproto-dpif-rid.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c > index f01468025..c67726cb6 100644 > --- a/ofproto/ofproto-dpif-rid.c > +++ b/ofproto/ofproto-dpif-rid.c > @@ -233,9 +233,17 @@ 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 = NULL; > + struct recirc_id_node *existing_node = NULL; > > - struct recirc_id_node *node = xzalloc(sizeof *node); > + /* First try to find an existing node with the same state. */ > + existing_node = recirc_ref_equal(state, hash); > + if (existing_node) { > + return existing_node; > + }
Hi, Sunyang and Bin. Thanks for the patch. I'm a little confused by the value of this change. The only code path that doesn't already lookup existing recirc nodes is a direct call to recirc_alloc_id(), and this is happening only in the bonding code. Recirculation nodes for bonding are not really dynamic, they are allocated once when the port is created. So, the room for having a duplicate state there is minimal. But also all the bonding ports on the same bridge will have the same frozen state, and so this change will likley cause all bonding ports to share the same recirculatin id, which will break the bonding logic. We must not lookup existing nodes in this function. All other allocations are happening via recirc_alloc_id_ctx() that is already looking up existing nodes before calling recirc_alloc_id__(), so it doesn't make sense to lookup again. All in all, it doesn't seem like this change is affecting anything except for bonding. And for bonding it breaks the logic. Or am I missing some other use case here? Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
