Hi Ales LGTM, thanks! Acked-by: Xavier Simonart <[email protected]>
Thanks Xavier On Fri, Sep 5, 2025 at 7:08 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]> > --- > v2: Fix missing hash_finish() in route_table_notify_hash_watch(). > --- > controller/ecmp-next-hop-monitor.c | 20 +++++++++++------- > controller/pinctrl.c | 33 +++++++++++++++--------------- > controller/route-table-notify.c | 2 +- > northd/en-ecmp-nexthop.c | 16 +++++++++++---- > 4 files changed, 43 insertions(+), 28 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 a722da59e..c83837f1b 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/controller/route-table-notify.c > b/controller/route-table-notify.c > index a052a7e8d..1fa182ea5 100644 > --- a/controller/route-table-notify.c > +++ b/controller/route-table-notify.c > @@ -96,7 +96,7 @@ route_table_deregister_notifiers(void) > static uint32_t > route_table_notify_hash_watch(uint32_t table_id) > { > - return hash_add(0, table_id); > + return hash_int(table_id, 0); > } > > void > 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.51.0 > > _______________________________________________ > 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
