On Fri, Sep 5, 2025 at 5:14 PM Xavier Simonart <[email protected]> wrote:
> Hi Ales > > Thanks for the patch. > Is hash_finish not also needed in route_table_notify_hash_watch > from controller/route-table-notify.c ? > > Thanks > Xavier > Hi Xavier, thank you for the review, you are right I have missed that one. It will be fixed in v2. Thanks, Ales > > On Mon, Aug 25, 2025 at 3:09 PM Ales Musil via dev < > [email protected]> wrote: > >> Call hash_finish properly in hashing sequeces that didn't do that. >> This should prevent suboptimal hashing results. >> >> Reported-by: Ilya Maximets <[email protected]> >> Signed-off-by: Ales Musil <[email protected]> >> --- >> controller/ecmp-next-hop-monitor.c | 20 +++++++++++------- >> controller/pinctrl.c | 33 +++++++++++++++--------------- >> northd/en-ecmp-nexthop.c | 16 +++++++++++---- >> 3 files changed, 42 insertions(+), 27 deletions(-) >> >> diff --git a/controller/ecmp-next-hop-monitor.c >> b/controller/ecmp-next-hop-monitor.c >> index 4dd5aa542..39ea46a20 100644 >> --- a/controller/ecmp-next-hop-monitor.c >> +++ b/controller/ecmp-next-hop-monitor.c >> @@ -62,6 +62,17 @@ void ecmp_nexthop_destroy(void) >> ecmp_nexthop_destroy_map(&ecmp_nexthop); >> } >> >> +static uint32_t >> +ecmp_nexthop_hash(const char *nexthop, const char *mac, >> + const uint16_t zone_id) >> +{ >> + uint32_t hash = hash_string(nexthop, 0); >> + hash = hash_add(hash, hash_string(mac, 0)); >> + hash = hash_add(hash, zone_id); >> + >> + return hash_finish(hash, 64); >> +} >> + >> static struct ecmp_nexthop_data * >> ecmp_nexthop_alloc_entry(const char *nexthop, const char *mac, >> const uint16_t zone_id, struct hmap *map) >> @@ -71,10 +82,7 @@ ecmp_nexthop_alloc_entry(const char *nexthop, const >> char *mac, >> e->mac = xstrdup(mac); >> e->zone_id = zone_id; >> >> - uint32_t hash = hash_string(nexthop, 0); >> - hash = hash_add(hash, hash_string(mac, 0)); >> - hash = hash_add(hash, zone_id); >> - hmap_insert(map, &e->hmap_node, hash); >> + hmap_insert(map, &e->hmap_node, ecmp_nexthop_hash(nexthop, mac, >> zone_id)); >> >> return e; >> } >> @@ -83,9 +91,7 @@ static struct ecmp_nexthop_data * >> ecmp_nexthop_find_entry(const char *nexthop, const char *mac, >> const uint16_t zone_id, const struct hmap *map) >> { >> - uint32_t hash = hash_string(nexthop, 0); >> - hash = hash_add(hash, hash_string(mac, 0)); >> - hash = hash_add(hash, zone_id); >> + uint32_t hash = ecmp_nexthop_hash(nexthop, mac, zone_id); >> >> struct ecmp_nexthop_data *e; >> HMAP_FOR_EACH_WITH_HASH (e, hmap_node, hash, map) { >> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >> index 400cb59e4..684a8ae5c 100644 >> --- a/controller/pinctrl.c >> +++ b/controller/pinctrl.c >> @@ -4910,15 +4910,24 @@ destroy_send_arps_nds(void) >> hmap_destroy(&send_arp_nd_data); >> } >> >> +static uint32_t >> +arp_nd_data_get_hash(const struct in6_addr *dst_ip, const uint32_t >> dp_key, >> + const uint32_t port_key) >> +{ >> + uint32_t hash = 0; >> + hash = hash_add_in6_addr(hash, dst_ip); >> + hash = hash_add(hash, dp_key); >> + hash = hash_add(hash, port_key); >> + >> + return hash_finish(hash, 24); >> +} >> + >> static struct arp_nd_data * >> arp_nd_find_data(const struct sbrec_port_binding *pb, >> const struct in6_addr *nexthop) >> { >> - uint32_t hash = 0; >> - >> - hash = hash_add_in6_addr(hash, nexthop); >> - hash = hash_add(hash, pb->datapath->tunnel_key); >> - hash = hash_add(hash, pb->tunnel_key); >> + uint32_t hash = arp_nd_data_get_hash(nexthop, >> pb->datapath->tunnel_key, >> + pb->tunnel_key); >> >> struct arp_nd_data *e; >> HMAP_FOR_EACH_WITH_HASH (e, hmap_node, hash, &send_arp_nd_data) { >> @@ -4931,15 +4940,6 @@ arp_nd_find_data(const struct sbrec_port_binding >> *pb, >> return NULL; >> } >> >> -static uint32_t >> -arp_nd_data_get_hash(struct arp_nd_data *e) >> -{ >> - uint32_t hash = 0; >> - hash = hash_add_in6_addr(hash, &e->dst_ip); >> - hash = hash_add(hash, e->dp_key); >> - return hash_add(hash, e->port_key); >> -} >> - >> static struct arp_nd_data * >> arp_nd_alloc_data(const struct eth_addr ea, >> struct in6_addr src_ip, struct in6_addr dst_ip, >> @@ -4954,7 +4954,7 @@ arp_nd_alloc_data(const struct eth_addr ea, >> e->dp_key = pb->datapath->tunnel_key; >> e->port_key = pb->tunnel_key; >> >> - uint32_t hash = arp_nd_data_get_hash(e); >> + uint32_t hash = arp_nd_data_get_hash(&e->dst_ip, e->dp_key, >> e->port_key); >> hmap_insert(&send_arp_nd_data, &e->hmap_node, hash); >> notify_pinctrl_handler(); >> >> @@ -4977,7 +4977,8 @@ arp_nd_sync_data(const struct >> sbrec_ecmp_nexthop_table *ecmp_nh_table) >> &nexthop); >> if (e) { >> hmap_remove(&send_arp_nd_data, &e->hmap_node); >> - uint32_t hash = arp_nd_data_get_hash(e); >> + uint32_t hash = arp_nd_data_get_hash(&e->dst_ip, e->dp_key, >> + e->port_key); >> hmap_insert(&arp_nd_active, &e->hmap_node, hash); >> } >> } >> diff --git a/northd/en-ecmp-nexthop.c b/northd/en-ecmp-nexthop.c >> index 7e9ed4f34..673e4aa4a 100644 >> --- a/northd/en-ecmp-nexthop.c >> +++ b/northd/en-ecmp-nexthop.c >> @@ -39,6 +39,15 @@ struct ecmp_nexthop_data { >> const struct sbrec_ecmp_nexthop *sb_ecmp_nh; >> }; >> >> +static uint32_t >> +ecmp_nexthop_hash(const char *nexthop, const uint32_t port_key) >> +{ >> + uint32_t hash = hash_string(nexthop, 0); >> + hash = hash_add(hash, port_key); >> + >> + return hash_finish(hash, 64); >> +} >> + >> static struct ecmp_nexthop_data * >> ecmp_nexthop_insert_entry(const struct sbrec_ecmp_nexthop *sb_ecmp_nh, >> struct hmap *map) >> @@ -46,8 +55,8 @@ ecmp_nexthop_insert_entry(const struct >> sbrec_ecmp_nexthop *sb_ecmp_nh, >> struct ecmp_nexthop_data *e = xmalloc(sizeof *e); >> e->sb_ecmp_nh = sb_ecmp_nh; >> >> - uint32_t hash = hash_string(sb_ecmp_nh->nexthop, 0); >> - hash = hash_add(hash, hash_int(sb_ecmp_nh->port->tunnel_key, 0)); >> + uint32_t hash = ecmp_nexthop_hash(sb_ecmp_nh->nexthop, >> + sb_ecmp_nh->port->tunnel_key); >> hmap_insert(map, &e->hmap_node, hash); >> >> return e; >> @@ -58,8 +67,7 @@ ecmp_nexthop_find_entry(const char *nexthop, >> const struct sbrec_port_binding *port, >> struct hmap *map) >> { >> - uint32_t hash = hash_string(nexthop, 0); >> - hash = hash_add(hash, hash_int(port->tunnel_key, 0)); >> + uint32_t hash = ecmp_nexthop_hash(nexthop, port->tunnel_key); >> >> struct ecmp_nexthop_data *e; >> HMAP_FOR_EACH_WITH_HASH (e, hmap_node, hash, map) { >> -- >> 2.50.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
