| Message ID | 20181030220318.13998-2-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2,1/3] ovn-northd: Use structure assignment instead of memcpy(). | expand |
On Wed, Oct 31, 2018 at 3:35 AM Ben Pfaff <blp@ovn.org> wrote: > The key for a "struct ovn_chassis_qdisc_queues" is a Chassis UUID and a > queue_id, but only the UUID was being hashed, so if there was more than one > per chassis then they'd all end up in the same hash bucket, which is > needlessly inefficient. (And if there's only one per chassis then why do > we bother allocating them at all?) > > Found by inspection. > > Signed-off-by: Ben Pfaff <blp@ovn.org> > Acked-by: Numan Siddique <nusiddiq@redhat.com> > --- > ovn/northd/ovn-northd.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index bc07a40bd0f8..6cb0b6cc410a 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -323,6 +323,12 @@ struct ovn_chassis_qdisc_queues { > struct uuid chassis_uuid; > }; > > +static uint32_t > +hash_chassis_queue(const struct uuid *chassis_uuid, uint32_t queue_id) > +{ > + return hash_2words(uuid_hash(chassis_uuid), queue_id); > +} > + > static void > destroy_chassis_queues(struct hmap *set) > { > @@ -340,7 +346,8 @@ add_chassis_queue(struct hmap *set, struct uuid > *chassis_uuid, > struct ovn_chassis_qdisc_queues *node = xmalloc(sizeof *node); > node->queue_id = queue_id; > node->chassis_uuid = *chassis_uuid; > - hmap_insert(set, &node->key_node, uuid_hash(chassis_uuid)); > + hmap_insert(set, &node->key_node, > + hash_chassis_queue(chassis_uuid, queue_id)); > } > > static bool > @@ -348,7 +355,8 @@ chassis_queueid_in_use(const struct hmap *set, struct > uuid *chassis_uuid, > uint32_t queue_id) > { > const struct ovn_chassis_qdisc_queues *node; > - HMAP_FOR_EACH_WITH_HASH (node, key_node, uuid_hash(chassis_uuid), > set) { > + HMAP_FOR_EACH_WITH_HASH (node, key_node, > + hash_chassis_queue(chassis_uuid, queue_id), > set) { > if (uuid_equals(chassis_uuid, &node->chassis_uuid) > && node->queue_id == queue_id) { > return true; > @@ -378,11 +386,11 @@ static void > free_chassis_queueid(struct hmap *set, struct sbrec_chassis *chassis, > uint32_t queue_id) > { > + const struct uuid *chassis_uuid = &chassis->header_.uuid; > struct ovn_chassis_qdisc_queues *node; > HMAP_FOR_EACH_WITH_HASH (node, key_node, > - uuid_hash(&chassis->header_.uuid), > - set) { > - if (uuid_equals(&chassis->header_.uuid, &node->chassis_uuid) > + hash_chassis_queue(chassis_uuid, queue_id), > set) { > + if (uuid_equals(chassis_uuid, &node->chassis_uuid) > && node->queue_id == queue_id) { > hmap_remove(set, &node->key_node); > break; > -- > 2.16.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index bc07a40bd0f8..6cb0b6cc410a 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -323,6 +323,12 @@ struct ovn_chassis_qdisc_queues { struct uuid chassis_uuid; }; +static uint32_t +hash_chassis_queue(const struct uuid *chassis_uuid, uint32_t queue_id) +{ + return hash_2words(uuid_hash(chassis_uuid), queue_id); +} + static void destroy_chassis_queues(struct hmap *set) { @@ -340,7 +346,8 @@ add_chassis_queue(struct hmap *set, struct uuid *chassis_uuid, struct ovn_chassis_qdisc_queues *node = xmalloc(sizeof *node); node->queue_id = queue_id; node->chassis_uuid = *chassis_uuid; - hmap_insert(set, &node->key_node, uuid_hash(chassis_uuid)); + hmap_insert(set, &node->key_node, + hash_chassis_queue(chassis_uuid, queue_id)); } static bool @@ -348,7 +355,8 @@ chassis_queueid_in_use(const struct hmap *set, struct uuid *chassis_uuid, uint32_t queue_id) { const struct ovn_chassis_qdisc_queues *node; - HMAP_FOR_EACH_WITH_HASH (node, key_node, uuid_hash(chassis_uuid), set) { + HMAP_FOR_EACH_WITH_HASH (node, key_node, + hash_chassis_queue(chassis_uuid, queue_id), set) { if (uuid_equals(chassis_uuid, &node->chassis_uuid) && node->queue_id == queue_id) { return true; @@ -378,11 +386,11 @@ static void free_chassis_queueid(struct hmap *set, struct sbrec_chassis *chassis, uint32_t queue_id) { + const struct uuid *chassis_uuid = &chassis->header_.uuid; struct ovn_chassis_qdisc_queues *node; HMAP_FOR_EACH_WITH_HASH (node, key_node, - uuid_hash(&chassis->header_.uuid), - set) { - if (uuid_equals(&chassis->header_.uuid, &node->chassis_uuid) + hash_chassis_queue(chassis_uuid, queue_id), set) { + if (uuid_equals(chassis_uuid, &node->chassis_uuid) && node->queue_id == queue_id) { hmap_remove(set, &node->key_node); break;
The key for a "struct ovn_chassis_qdisc_queues" is a Chassis UUID and a queue_id, but only the UUID was being hashed, so if there was more than one per chassis then they'd all end up in the same hash bucket, which is needlessly inefficient. (And if there's only one per chassis then why do we bother allocating them at all?) Found by inspection. Signed-off-by: Ben Pfaff <blp@ovn.org> --- ovn/northd/ovn-northd.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)