Looks good.
Ethan
On Tue, Dec 27, 2011 at 13:28, Ben Pfaff <b...@nicira.com> wrote:
> It's redundant to pass both a facet or subfacet and an ofproto_dpif,
> because the latter can be derived from the former.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
> ofproto/ofproto-dpif.c | 138
> +++++++++++++++++++++++++-----------------------
> 1 files changed, 71 insertions(+), 67 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 1a399c0..f131ebd 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -306,27 +306,25 @@ struct facet {
> };
>
> static struct facet *facet_create(struct rule_dpif *, const struct flow *);
> -static void facet_remove(struct ofproto_dpif *, struct facet *);
> +static void facet_remove(struct facet *);
> static void facet_free(struct facet *);
>
> static struct facet *facet_find(struct ofproto_dpif *, const struct flow *);
> static struct facet *facet_lookup_valid(struct ofproto_dpif *,
> const struct flow *);
> -static bool facet_revalidate(struct ofproto_dpif *, struct facet *);
> -
> +static bool facet_revalidate(struct facet *);
> static bool execute_controller_action(struct ofproto_dpif *,
> const struct flow *,
> const struct nlattr *odp_actions,
> size_t actions_len,
> struct ofpbuf *packet, bool clone);
>
> -static void facet_flush_stats(struct ofproto_dpif *, struct facet *);
> +static void facet_flush_stats(struct facet *);
>
> -static void facet_update_time(struct ofproto_dpif *, struct facet *,
> - long long int used);
> +static void facet_update_time(struct facet *, long long int used);
> static void facet_reset_counters(struct facet *);
> static void facet_push_stats(struct facet *);
> -static void facet_account(struct ofproto_dpif *, struct facet *);
> +static void facet_account(struct facet *);
>
> static bool facet_is_controller_flow(struct facet *);
>
> @@ -368,26 +366,24 @@ struct subfacet {
> ovs_be16 initial_tci; /* Initial VLAN TCI value. */
> };
>
> -static struct subfacet *subfacet_create(struct ofproto_dpif *, struct facet
> *,
> - enum odp_key_fitness,
> +static struct subfacet *subfacet_create(struct facet *, enum odp_key_fitness,
> const struct nlattr *key,
> size_t key_len, ovs_be16 initial_tci);
> static struct subfacet *subfacet_find(struct ofproto_dpif *,
> const struct nlattr *key, size_t
> key_len);
> -static void subfacet_destroy(struct ofproto_dpif *, struct subfacet *);
> -static void subfacet_destroy__(struct ofproto_dpif *, struct subfacet *);
> +static void subfacet_destroy(struct subfacet *);
> +static void subfacet_destroy__(struct subfacet *);
> static void subfacet_reset_dp_stats(struct subfacet *,
> struct dpif_flow_stats *);
> -static void subfacet_update_time(struct ofproto_dpif *, struct subfacet *,
> - long long int used);
> -static void subfacet_update_stats(struct ofproto_dpif *, struct subfacet *,
> +static void subfacet_update_time(struct subfacet *, long long int used);
> +static void subfacet_update_stats(struct subfacet *,
> const struct dpif_flow_stats *);
> -static void subfacet_make_actions(struct ofproto_dpif *, struct subfacet *,
> +static void subfacet_make_actions(struct subfacet *,
> const struct ofpbuf *packet);
> -static int subfacet_install(struct ofproto_dpif *, struct subfacet *,
> +static int subfacet_install(struct subfacet *,
> const struct nlattr *actions, size_t actions_len,
> struct dpif_flow_stats *);
> -static void subfacet_uninstall(struct ofproto_dpif *, struct subfacet *);
> +static void subfacet_uninstall(struct subfacet *);
>
> struct ofport_dpif {
> struct ofport up;
> @@ -813,7 +809,7 @@ run(struct ofproto *ofproto_)
> HMAP_FOR_EACH_SAFE (facet, next, hmap_node, &ofproto->facets) {
> if (revalidate_all
> || tag_set_intersects(&revalidate_set, facet->tags)) {
> - facet_revalidate(ofproto, facet);
> + facet_revalidate(facet);
> }
> }
> }
> @@ -878,7 +874,7 @@ flush(struct ofproto *ofproto_)
> subfacet->dp_packet_count = 0;
> subfacet->dp_byte_count = 0;
> }
> - facet_remove(ofproto, facet);
> + facet_remove(facet);
> }
> dpif_flow_flush(ofproto->dpif);
> }
> @@ -2538,7 +2534,7 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct
> flow_miss *miss,
> facet = facet_create(rule, flow);
> }
>
> - subfacet = subfacet_create(ofproto, facet,
> + subfacet = subfacet_create(facet,
> miss->key_fitness, miss->key, miss->key_len,
> miss->initial_tci);
>
> @@ -2563,13 +2559,13 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct
> flow_miss *miss,
> }
>
> if (!facet->may_install || !subfacet->actions) {
> - subfacet_make_actions(ofproto, subfacet, packet);
> + subfacet_make_actions(subfacet, packet);
> }
>
> /* Credit statistics to subfacet for this packet. We must do this now
> * because execute_controller_action() below may destroy 'packet'. */
> dpif_flow_stats_extract(&facet->flow, packet, &stats);
> - subfacet_update_stats(ofproto, subfacet, &stats);
> + subfacet_update_stats(subfacet, &stats);
>
> if (!execute_controller_action(ofproto, &facet->flow,
> subfacet->actions,
> @@ -2906,8 +2902,8 @@ update_stats(struct ofproto_dpif *p)
> subfacet->dp_packet_count = stats->n_packets;
> subfacet->dp_byte_count = stats->n_bytes;
>
> - subfacet_update_time(p, subfacet, stats->used);
> - facet_account(p, facet);
> + subfacet_update_time(subfacet, stats->used);
> + facet_account(facet);
> facet_push_stats(facet);
> } else {
> if (!VLOG_DROP_WARN(&rl)) {
> @@ -3025,7 +3021,7 @@ expire_subfacets(struct ofproto_dpif *ofproto, int
> dp_max_idle)
> HMAP_FOR_EACH_SAFE (subfacet, next_subfacet, hmap_node,
> &ofproto->subfacets) {
> if (subfacet->used < cutoff) {
> - subfacet_destroy(ofproto, subfacet);
> + subfacet_destroy(subfacet);
> }
> }
> }
> @@ -3035,7 +3031,6 @@ expire_subfacets(struct ofproto_dpif *ofproto, int
> dp_max_idle)
> static void
> rule_expire(struct rule_dpif *rule)
> {
> - struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> struct facet *facet, *next_facet;
> long long int now;
> uint8_t reason;
> @@ -3057,7 +3052,7 @@ rule_expire(struct rule_dpif *rule)
> /* Update stats. (This is a no-op if the rule expired due to an idle
> * timeout, because that only happens when the rule has no facets left.)
> */
> LIST_FOR_EACH_SAFE (facet, next_facet, list_node, &rule->facets) {
> - facet_remove(ofproto, facet);
> + facet_remove(facet);
> }
>
> /* Get rid of the rule. */
> @@ -3168,24 +3163,26 @@ execute_odp_actions(struct ofproto_dpif *ofproto,
> const struct flow *flow,
> * - Removes 'facet' from its rule and from ofproto->facets.
> */
> static void
> -facet_remove(struct ofproto_dpif *ofproto, struct facet *facet)
> +facet_remove(struct facet *facet)
> {
> + struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> struct subfacet *subfacet, *next_subfacet;
>
> LIST_FOR_EACH_SAFE (subfacet, next_subfacet, list_node,
> &facet->subfacets) {
> - subfacet_destroy__(ofproto, subfacet);
> + subfacet_destroy__(subfacet);
> }
>
> - facet_flush_stats(ofproto, facet);
> + facet_flush_stats(facet);
> hmap_remove(&ofproto->facets, &facet->hmap_node);
> list_remove(&facet->list_node);
> facet_free(facet);
> }
>
> static void
> -facet_account(struct ofproto_dpif *ofproto, struct facet *facet)
> +facet_account(struct facet *facet)
> {
> + struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> uint64_t n_bytes;
> struct subfacet *subfacet;
> const struct nlattr *a;
> @@ -3269,8 +3266,9 @@ facet_is_controller_flow(struct facet *facet)
> * 'facet''s statistics in the datapath should have been zeroed and folded
> into
> * its packet and byte counts before this function is called. */
> static void
> -facet_flush_stats(struct ofproto_dpif *ofproto, struct facet *facet)
> +facet_flush_stats(struct facet *facet)
> {
> + struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> struct subfacet *subfacet;
>
> LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
> @@ -3279,7 +3277,7 @@ facet_flush_stats(struct ofproto_dpif *ofproto, struct
> facet *facet)
> }
>
> facet_push_stats(facet);
> - facet_account(ofproto, facet);
> + facet_account(facet);
>
> if (ofproto->netflow && !facet_is_controller_flow(facet)) {
> struct ofexpired expired;
> @@ -3334,7 +3332,7 @@ facet_lookup_valid(struct ofproto_dpif *ofproto, const
> struct flow *flow)
> if (facet
> && (ofproto->need_revalidate
> || tag_set_intersects(&ofproto->revalidate_set, facet->tags))
> - && !facet_revalidate(ofproto, facet)) {
> + && !facet_revalidate(facet)) {
> COVERAGE_INC(facet_invalidated);
> return NULL;
> }
> @@ -3342,7 +3340,7 @@ facet_lookup_valid(struct ofproto_dpif *ofproto, const
> struct flow *flow)
> return facet;
> }
>
> -/* Re-searches 'ofproto''s classifier for a rule matching 'facet':
> +/* Re-searches the classifier for 'facet':
> *
> * - If the rule found is different from 'facet''s current rule, moves
> * 'facet' to the new rule and recompiles its actions.
> @@ -3354,8 +3352,9 @@ facet_lookup_valid(struct ofproto_dpif *ofproto, const
> struct flow *flow)
> *
> * Returns true if 'facet' still exists, false if it has been destroyed. */
> static bool
> -facet_revalidate(struct ofproto_dpif *ofproto, struct facet *facet)
> +facet_revalidate(struct facet *facet)
> {
> + struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> struct actions {
> struct nlattr *odp_actions;
> size_t actions_len;
> @@ -3374,7 +3373,7 @@ facet_revalidate(struct ofproto_dpif *ofproto, struct
> facet *facet)
> new_rule = rule_dpif_lookup(ofproto, &facet->flow, 0);
> if (!new_rule) {
> /* No new rule, so delete the facet. */
> - facet_remove(ofproto, facet);
> + facet_remove(facet);
> return false;
> }
>
> @@ -3407,11 +3406,11 @@ facet_revalidate(struct ofproto_dpif *ofproto, struct
> facet *facet)
> if (should_install) {
> struct dpif_flow_stats stats;
>
> - subfacet_install(ofproto, subfacet,
> + subfacet_install(subfacet,
> odp_actions->data, odp_actions->size,
> &stats);
> - subfacet_update_stats(ofproto, subfacet, &stats);
> + subfacet_update_stats(subfacet, &stats);
> } else {
> - subfacet_uninstall(ofproto, subfacet);
> + subfacet_uninstall(subfacet);
> }
>
> if (!new_actions) {
> @@ -3427,7 +3426,7 @@ facet_revalidate(struct ofproto_dpif *ofproto, struct
> facet *facet)
> i++;
> }
> if (new_actions) {
> - facet_flush_stats(ofproto, facet);
> + facet_flush_stats(facet);
> }
>
> /* Update 'facet' now that we've taken care of all the old state. */
> @@ -3464,9 +3463,9 @@ facet_revalidate(struct ofproto_dpif *ofproto, struct
> facet *facet)
> /* Updates 'facet''s used time. Caller is responsible for calling
> * facet_push_stats() to update the flows which 'facet' resubmits into. */
> static void
> -facet_update_time(struct ofproto_dpif *ofproto, struct facet *facet,
> - long long int used)
> +facet_update_time(struct facet *facet, long long int used)
> {
> + struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> if (used > facet->used) {
> facet->used = used;
> if (used > facet->rule->used) {
> @@ -3580,10 +3579,10 @@ subfacet_find__(struct ofproto_dpif *ofproto,
> * which case the caller must populate the actions with
> * subfacet_make_actions(). */
> static struct subfacet *
> -subfacet_create(struct ofproto_dpif *ofproto, struct facet *facet,
> - enum odp_key_fitness key_fitness,
> +subfacet_create(struct facet *facet, enum odp_key_fitness key_fitness,
> const struct nlattr *key, size_t key_len, ovs_be16
> initial_tci)
> {
> + struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> uint32_t key_hash = odp_flow_key_hash(key, key_len);
> struct subfacet *subfacet;
>
> @@ -3595,7 +3594,7 @@ subfacet_create(struct ofproto_dpif *ofproto, struct
> facet *facet,
>
> /* This shouldn't happen. */
> VLOG_ERR_RL(&rl, "subfacet with wrong facet");
> - subfacet_destroy(ofproto, subfacet);
> + subfacet_destroy(subfacet);
> }
>
> subfacet = xzalloc(sizeof *subfacet);
> @@ -3635,9 +3634,12 @@ subfacet_find(struct ofproto_dpif *ofproto,
> /* Uninstalls 'subfacet' from the datapath, if it is installed, removes it
> from
> * its facet within 'ofproto', and frees it. */
> static void
> -subfacet_destroy__(struct ofproto_dpif *ofproto, struct subfacet *subfacet)
> +subfacet_destroy__(struct subfacet *subfacet)
> {
> - subfacet_uninstall(ofproto, subfacet);
> + struct facet *facet = subfacet->facet;
> + struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> +
> + subfacet_uninstall(subfacet);
> hmap_remove(&ofproto->subfacets, &subfacet->hmap_node);
> list_remove(&subfacet->list_node);
> free(subfacet->key);
> @@ -3648,13 +3650,13 @@ subfacet_destroy__(struct ofproto_dpif *ofproto,
> struct subfacet *subfacet)
> /* Destroys 'subfacet', as with subfacet_destroy__(), and then if this was
> the
> * last remaining subfacet in its facet destroys the facet too. */
> static void
> -subfacet_destroy(struct ofproto_dpif *ofproto, struct subfacet *subfacet)
> +subfacet_destroy(struct subfacet *subfacet)
> {
> struct facet *facet = subfacet->facet;
>
> - subfacet_destroy__(ofproto, subfacet);
> + subfacet_destroy__(subfacet);
> if (list_is_empty(&facet->subfacets)) {
> - facet_remove(ofproto, facet);
> + facet_remove(facet);
> }
> }
>
> @@ -3675,15 +3677,15 @@ subfacet_get_key(struct subfacet *subfacet, struct
> odputil_keybuf *keybuf,
>
> /* Composes the datapath actions for 'subfacet' based on its rule's actions.
> */
> static void
> -subfacet_make_actions(struct ofproto_dpif *p, struct subfacet *subfacet,
> - const struct ofpbuf *packet)
> +subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet)
> {
> struct facet *facet = subfacet->facet;
> const struct rule_dpif *rule = facet->rule;
> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> struct ofpbuf *odp_actions;
> struct action_xlate_ctx ctx;
>
> - action_xlate_ctx_init(&ctx, p, &facet->flow, subfacet->initial_tci,
> + action_xlate_ctx_init(&ctx, ofproto, &facet->flow, subfacet->initial_tci,
> packet);
> odp_actions = xlate_actions(&ctx, rule->up.actions, rule->up.n_actions);
> facet->tags = ctx.tags;
> @@ -3710,10 +3712,12 @@ subfacet_make_actions(struct ofproto_dpif *p, struct
> subfacet *subfacet,
> *
> * Returns 0 if successful, otherwise a positive errno value. */
> static int
> -subfacet_install(struct ofproto_dpif *ofproto, struct subfacet *subfacet,
> +subfacet_install(struct subfacet *subfacet,
> const struct nlattr *actions, size_t actions_len,
> struct dpif_flow_stats *stats)
> {
> + struct facet *facet = subfacet->facet;
> + struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> struct odputil_keybuf keybuf;
> enum dpif_flow_put_flags flags;
> struct ofpbuf key;
> @@ -3737,19 +3741,21 @@ subfacet_install(struct ofproto_dpif *ofproto, struct
> subfacet *subfacet,
>
> /* If 'subfacet' is installed in the datapath, uninstalls it. */
> static void
> -subfacet_uninstall(struct ofproto_dpif *p, struct subfacet *subfacet)
> +subfacet_uninstall(struct subfacet *subfacet)
> {
> if (subfacet->installed) {
> + struct rule_dpif *rule = subfacet->facet->rule;
> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> struct odputil_keybuf keybuf;
> struct dpif_flow_stats stats;
> struct ofpbuf key;
> int error;
>
> subfacet_get_key(subfacet, &keybuf, &key);
> - error = dpif_flow_del(p->dpif, key.data, key.size, &stats);
> + error = dpif_flow_del(ofproto->dpif, key.data, key.size, &stats);
> subfacet_reset_dp_stats(subfacet, &stats);
> if (!error) {
> - subfacet_update_stats(p, subfacet, &stats);
> + subfacet_update_stats(subfacet, &stats);
> }
> subfacet->installed = false;
> } else {
> @@ -3781,12 +3787,11 @@ subfacet_reset_dp_stats(struct subfacet *subfacet,
> /* Updates 'subfacet''s used time. The caller is responsible for calling
> * facet_push_stats() to update the flows which 'subfacet' resubmits into. */
> static void
> -subfacet_update_time(struct ofproto_dpif *ofproto, struct subfacet *subfacet,
> - long long int used)
> +subfacet_update_time(struct subfacet *subfacet, long long int used)
> {
> if (used > subfacet->used) {
> subfacet->used = used;
> - facet_update_time(ofproto, subfacet->facet, used);
> + facet_update_time(subfacet->facet, used);
> }
> }
>
> @@ -3797,13 +3802,13 @@ subfacet_update_time(struct ofproto_dpif *ofproto,
> struct subfacet *subfacet,
> * represents a packet that was sent by hand or if it represents statistics
> * that have been cleared out of the datapath. */
> static void
> -subfacet_update_stats(struct ofproto_dpif *ofproto, struct subfacet
> *subfacet,
> +subfacet_update_stats(struct subfacet *subfacet,
> const struct dpif_flow_stats *stats)
> {
> if (stats->n_packets || stats->used > subfacet->used) {
> struct facet *facet = subfacet->facet;
>
> - subfacet_update_time(ofproto, subfacet, stats->used);
> + subfacet_update_time(subfacet, stats->used);
> facet->packet_count += stats->n_packets;
> facet->byte_count += stats->n_bytes;
> facet_push_stats(facet);
> @@ -3922,11 +3927,10 @@ static void
> rule_destruct(struct rule *rule_)
> {
> struct rule_dpif *rule = rule_dpif_cast(rule_);
> - struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> struct facet *facet, *next_facet;
>
> LIST_FOR_EACH_SAFE (facet, next_facet, list_node, &rule->facets) {
> - facet_revalidate(ofproto, facet);
> + facet_revalidate(facet);
> }
>
> complete_operation(rule);
> @@ -5522,9 +5526,9 @@ send_active_timeout(struct ofproto_dpif *ofproto,
> struct facet *facet)
> if (subfacet->installed) {
> struct dpif_flow_stats stats;
>
> - subfacet_install(ofproto, subfacet, subfacet->actions,
> + subfacet_install(subfacet, subfacet->actions,
> subfacet->actions_len, &stats);
> - subfacet_update_stats(ofproto, subfacet, &stats);
> + subfacet_update_stats(subfacet, &stats);
> }
> }
>
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
