> + facet = facet_lookup_valid(ofproto, flow, miss->hmap_node.hash);
Pulling the hash out of hmap_node directly makes me a bit nervous.
Does it make sense to store the hash directly in struct flow_miss with
a comment about how it's computed? At minimum I think we should
document that handle_flow_miss() expects 'miss' to be in a hash table
which uses flow_hash() with a basis of zero as it's hash function.
Otherwise looks good, thanks.
Ethan
> if (!facet) {
> struct rule_dpif *rule;
>
> @@ -2585,7 +2587,7 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct
> flow_miss *miss,
> return;
> }
>
> - facet = facet_create(rule, flow);
> + facet = facet_create(rule, flow, miss->hmap_node.hash);
> }
>
> subfacet = subfacet_create(facet,
> @@ -3212,17 +3214,19 @@ rule_expire(struct rule_dpif *rule)
> * 'flow' exists in 'ofproto' and that 'flow' is the best match for 'rule' in
> * the ofproto's classifier table.
> *
> + * 'hash' must be the return value of flow_hash(flow, 0).
> + *
> * The facet will initially have no subfacets. The caller should create (at
> * least) one subfacet with subfacet_create(). */
> static struct facet *
> -facet_create(struct rule_dpif *rule, const struct flow *flow)
> +facet_create(struct rule_dpif *rule, const struct flow *flow, uint32_t hash)
> {
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> struct facet *facet;
>
> facet = xzalloc(sizeof *facet);
> facet->used = time_msec();
> - hmap_insert(&ofproto->facets, &facet->hmap_node, flow_hash(flow, 0));
> + hmap_insert(&ofproto->facets, &facet->hmap_node, hash);
> list_push_back(&rule->facets, &facet->list_node);
> facet->rule = rule;
> facet->flow = *flow;
> @@ -3430,15 +3434,17 @@ facet_flush_stats(struct facet *facet)
> /* Searches 'ofproto''s table of facets for one exactly equal to 'flow'.
> * Returns it if found, otherwise a null pointer.
> *
> + * 'hash' must be the return value of flow_hash(flow, 0).
> + *
> * The returned facet might need revalidation; use facet_lookup_valid()
> * instead if that is important. */
> static struct facet *
> -facet_find(struct ofproto_dpif *ofproto, const struct flow *flow)
> +facet_find(struct ofproto_dpif *ofproto,
> + const struct flow *flow, uint32_t hash)
> {
> struct facet *facet;
>
> - HMAP_FOR_EACH_WITH_HASH (facet, hmap_node, flow_hash(flow, 0),
> - &ofproto->facets) {
> + HMAP_FOR_EACH_WITH_HASH (facet, hmap_node, hash, &ofproto->facets) {
> if (flow_equal(flow, &facet->flow)) {
> return facet;
> }
> @@ -3450,11 +3456,14 @@ facet_find(struct ofproto_dpif *ofproto, const struct
> flow *flow)
> /* Searches 'ofproto''s table of facets for one exactly equal to 'flow'.
> * Returns it if found, otherwise a null pointer.
> *
> + * 'hash' must be the return value of flow_hash(flow, 0).
> + *
> * The returned facet is guaranteed to be valid. */
> static struct facet *
> -facet_lookup_valid(struct ofproto_dpif *ofproto, const struct flow *flow)
> +facet_lookup_valid(struct ofproto_dpif *ofproto, const struct flow *flow,
> + uint32_t hash)
> {
> - struct facet *facet = facet_find(ofproto, flow);
> + struct facet *facet = facet_find(ofproto, flow, hash);
>
> /* The facet we found might not be valid, since we could be in need of
> * revalidation. If it is not valid, don't return it. */
> --
> 1.7.9
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
