Thanks for the review Jacob.
On Thu, Oct 15, 2026 at 12:29 PM Jacob Tanenbaum <[email protected]> wrote:
>
>
>
> On Tue, Oct 6, 2026 at 1:50 PM Mark Michelson via dev
> <[email protected]> wrote:
>>
>> ECMP group IDs were assigned while building ECMP policy logical flows.
>> This commit moves the assignment into en-route-policies.
>>
>> This will assist with the upcoming commits that will add incremental
>> processing for routing policies.
>>
>> Signed-off-by: Mark Michelson <[email protected]>
>> ---
>> northd/en-route-policies.c | 6 ++++++
>> northd/en-route-policies.h | 4 ++++
>> northd/northd.c | 18 ++++++------------
>> 3 files changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c
>> index 0cdfb511d..dc33edb53 100644
>> --- a/northd/en-route-policies.c
>> +++ b/northd/en-route-policies.c
>> @@ -195,6 +195,7 @@ build_route_policies(struct ovn_datapath *od,
>> }
>>
>> size_t hash = uuid_hash(&od->key);
>> + uint16_t ecmp_group_id = 1;
>> for (int i = 0; i < od->nbr->n_policies; i++) {
>> const struct nbrec_logical_router_policy *rule =
>> od->nbr->policies[i];
>>
>> @@ -323,12 +324,17 @@ build_route_policies(struct ovn_datapath *od,
>> }
>> }
>>
>> + uint16_t group = 0;
>> + if (vector_len(&valid_nexthops) > 1) {
>
>
> missing upper bound check
I got very confused by this comment because I thought you were talking
about the vector_len() call. But I think you're actually referring to
the ecmp_group_id increment. I've added a check to ensure that it
cannot exceed UINT16_MAX. This gets essentially undone in a later
commit, but it's still good to have it here for correctness's sake.
>
>>
>> + group = ecmp_group_id++;
>> + }
>> struct route_policy *new_rp = xmalloc(sizeof *new_rp);
>> *new_rp = (struct route_policy) {
>> .rule = rule,
>> .valid_nexthops = vector_steal(&valid_nexthops),
>> .chain_id = chain_id,
>> .jump_chain_id = jump_chain_id,
>> + .ecmp_group_id = group,
>> };
>> hmap_insert(route_policies, &new_rp->key_node, hash);
>> }
>> diff --git a/northd/en-route-policies.h b/northd/en-route-policies.h
>> index 490ba09e7..dba0fc2b6 100644
>> --- a/northd/en-route-policies.h
>> +++ b/northd/en-route-policies.h
>> @@ -43,6 +43,10 @@ struct route_policy {
>> struct vector valid_nexthops; /* struct route_policy_nexthop */
>> uint32_t chain_id;
>> uint32_t jump_chain_id;
>> + /* If the policy is ECMP, then this is the group ID for the policy.
>> + * If the policy is not ECMP, then this is 0.
>> + */
>> + uint32_t ecmp_group_id;
>
>
> The field here is uint32_t but the counter is assigned from is uint16_t, I
> think this is supposed to correspond to REG_ECMP_GROUP_ID
> which is a 16 bit register. Could you change it all to uin16_t?
You're correct that the maximum value for this is UINT16_MAX. A later
commit updates the ECMP group assignment to use the same algorithm as
datapath and port tunnel IDs, which operate on uint32_t. This, plus
alignment concerns, is why I oversized this field. What I'll do is
change the rest of the code to operate on uint32_t as well, but cap
the value at UINT16_MAX.
>
>>
>> };
>>
>> /* Global route policy data exported by en-route-policies. */
>> diff --git a/northd/northd.c b/northd/northd.c
>> index d3f80f898..15921d525 100644
>> --- a/northd/northd.c
>> +++ b/northd/northd.c
>> @@ -12272,7 +12272,6 @@ static void
>> build_ecmp_routing_policy_flows(struct lflow_table *lflows,
>> struct ovn_datapath *od,
>> struct route_policy *rp,
>> - uint16_t ecmp_group_id,
>> struct lflow_ref *lflow_ref)
>> {
>> const struct nbrec_logical_router_policy *rule = rp->rule;
>> @@ -12317,9 +12316,9 @@ build_ecmp_routing_policy_flows(struct lflow_table
>> *lflows,
>> is_ipv4);
>>
>> ds_clear(&match);
>> - ds_put_format(&match, REG_ECMP_GROUP_ID" == %"PRIu16" && "
>> + ds_put_format(&match, REG_ECMP_GROUP_ID" == %"PRIu32" && "
>> REG_ECMP_MEMBER_ID" == %"PRIuSIZE,
>> - ecmp_group_id, i + 1);
>> + rp->ecmp_group_id, i + 1);
>> ovn_lflow_add(lflows, od, S_ROUTER_IN_POLICY_ECMP, 100,
>> ds_cstr(&match), ds_cstr(&actions), lflow_ref,
>> WITH_HINT(&rule->header_));
>> @@ -12327,8 +12326,8 @@ build_ecmp_routing_policy_flows(struct lflow_table
>> *lflows,
>> }
>>
>> ds_clear(&actions);
>> - ds_put_format(&actions, "%s = %"PRIu16
>> - "; %s = select(", REG_ECMP_GROUP_ID, ecmp_group_id,
>> + ds_put_format(&actions, "%s = %"PRIu32
>> + "; %s = select(", REG_ECMP_GROUP_ID, rp->ecmp_group_id,
>> REG_ECMP_MEMBER_ID);
>>
>> i = 0;
>> @@ -15686,19 +15685,14 @@ build_ingress_policy_flows_for_lrouter(
>> lflow_ref);
>>
>> /* Convert routing policies to flows. */
>> - uint16_t ecmp_group_id = 1;
>> struct route_policy *rp;
>> HMAP_FOR_EACH_WITH_HASH (rp, key_node, uuid_hash(&od->key),
>> route_policies) {
>> const struct nbrec_logical_router_policy *rule = rp->rule;
>> - bool is_ecmp_reroute =
>> - (!strcmp(rule->action, "reroute") &&
>> - vector_len(&rp->valid_nexthops) > 1);
>> + bool is_ecmp_reroute = rp->ecmp_group_id != 0;
>>
>> if (is_ecmp_reroute) {
>> - build_ecmp_routing_policy_flows(lflows, od, rp, ecmp_group_id,
>> - lflow_ref);
>> - ecmp_group_id++;
>> + build_ecmp_routing_policy_flows(lflows, od, rp, lflow_ref);
>> } else {
>> build_routing_policy_flow(lflows, od, rp, &rule->header_,
>> lflow_ref);
>> --
>> 2.55.0
>>
>> _______________________________________________
>> dev mailing list
>> [email protected]
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
> Jacob
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
