[ovs-dev] [optimize 22/26] ofproto-dpif: Avoid malloc() in common case for "execute" operations.

Ethan Jackson ethan at nicira.com
Wed Apr 18 18:30:29 UTC 2012


The addition of the ofpbuf stubs is a really useful optimization, but
it makes the memory management a bit more difficult to work out
conceptually, so please excuse me if my following comment is
misguided:

It looks to me like handle_flow_miss() can populate elements of
'ops->dpif_op.u.execute' with odp_actions.data which could be
allocated on the stack.  This data is then used by the caller of
handle_flow_miss() in its call to dpif_operate().  Also, it seems to
me that more than one packet could have its dpif_execute actions
populated from the same stack allocated odp_actions buffer.  Again, I
may be reading the code wrong, so if you think it's correct feel free
to ignore.

Ethan



On Mon, Apr 16, 2012 at 17:19, Ben Pfaff <blp at nicira.com> wrote:
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  ofproto/ofproto-dpif.c |   68 ++++++++++++++++++++++++++---------------------
>  1 files changed, 38 insertions(+), 30 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 8eeefd3..b74331e 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -334,7 +334,8 @@ 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 subfacet *,
> -                                  const struct ofpbuf *packet);
> +                                  const struct ofpbuf *packet,
> +                                  struct ofpbuf *odp_actions);
>  static int subfacet_install(struct subfacet *,
>                             const struct nlattr *actions, size_t actions_len,
>                             struct dpif_flow_stats *);
> @@ -2481,7 +2482,9 @@ struct flow_miss {
>
>  struct flow_miss_op {
>     struct dpif_op dpif_op;
> -    struct subfacet *subfacet;
> +    struct subfacet *subfacet;  /* Subfacet  */
> +    void *garbage;              /* Pointer to pass to free(), NULL if none. */
> +    uint64_t stub[1024 / 8];    /* Temporary buffer. */
>  };
>
>  /* Sends an OFPT_PACKET_IN message for 'packet' of type OFPR_NO_MATCH to each
> @@ -2598,9 +2601,12 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
>                                miss->initial_tci);
>
>     LIST_FOR_EACH (packet, list_node, &miss->packets) {
> +        struct flow_miss_op *op = &ops[*n_ops];
> +        struct dpif_execute *execute = &op->dpif_op.u.execute;
>         struct dpif_flow_stats stats;
> -        struct flow_miss_op *op;
> -        struct dpif_execute *execute;
> +
> +        uint64_t odp_actions_stub[1024 / 8];
> +        struct ofpbuf odp_actions;
>
>         ofproto->n_matches++;
>
> @@ -2618,8 +2624,10 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
>             send_packet_in_miss(ofproto, packet, flow);
>         }
>
> +        ofpbuf_use_stub(&odp_actions,
> +                        odp_actions_stub, sizeof odp_actions_stub);
>         if (!facet->may_install || !subfacet->actions) {
> -            subfacet_make_actions(subfacet, packet);
> +            subfacet_make_actions(subfacet, packet, &odp_actions);
>         }
>
>         dpif_flow_stats_extract(&facet->flow, packet, &stats);
> @@ -2639,18 +2647,22 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
>             eth_pop_vlan(packet);
>         }
>
> -        op = &ops[(*n_ops)++];
> -        execute = &op->dpif_op.u.execute;
> -        op->subfacet = subfacet;
> +        /* Set up operation. */
>         op->dpif_op.type = DPIF_OP_EXECUTE;
>         execute->key = miss->key;
>         execute->key_len = miss->key_len;
> -        execute->actions = (facet->may_install
> -                            ? subfacet->actions
> -                            : xmemdup(subfacet->actions,
> -                                      subfacet->actions_len));
> -        execute->actions_len = subfacet->actions_len;
> +        if (facet->may_install) {
> +            execute->actions = subfacet->actions;
> +            execute->actions_len = subfacet->actions_len;
> +            op->garbage = NULL;
> +        } else {
> +            execute->actions = odp_actions.data;
> +            execute->actions_len = odp_actions.size;
> +            op->garbage = ofpbuf_get_uninit_pointer(&odp_actions);
> +        }
>         execute->packet = packet;
> +
> +        (*n_ops)++;
>     }
>
>     if (facet->may_install && subfacet->key_fitness != ODP_FIT_TOO_LITTLE) {
> @@ -2658,6 +2670,7 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
>         struct dpif_flow_put *put = &op->dpif_op.u.flow_put;
>
>         op->subfacet = subfacet;
> +        op->garbage = NULL;
>         op->dpif_op.type = DPIF_OP_FLOW_PUT;
>         put->flags = DPIF_FP_CREATE | DPIF_FP_MODIFY;
>         put->key = miss->key;
> @@ -2815,14 +2828,9 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct dpif_upcall *upcalls,
>     /* Free memory and update facets. */
>     for (i = 0; i < n_ops; i++) {
>         struct flow_miss_op *op = &flow_miss_ops[i];
> -        struct dpif_execute *execute;
>
>         switch (op->dpif_op.type) {
>         case DPIF_OP_EXECUTE:
> -            execute = &op->dpif_op.u.execute;
> -            if (op->subfacet->actions != execute->actions) {
> -                free((struct nlattr *) execute->actions);
> -            }
>             break;
>
>         case DPIF_OP_FLOW_PUT:
> @@ -2834,6 +2842,8 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto, struct dpif_upcall *upcalls,
>         case DPIF_OP_FLOW_DEL:
>             NOT_REACHED();
>         }
> +
> +        free(op->garbage);
>     }
>     hmap_destroy(&todo);
>  }
> @@ -3951,22 +3961,22 @@ subfacet_get_key(struct subfacet *subfacet, struct odputil_keybuf *keybuf,
>     }
>  }
>
> -/* Composes the datapath actions for 'subfacet' based on its rule's actions. */
> +/* Composes the datapath actions for 'subfacet' based on its rule's actions.
> + * Translates the actions into 'odp_actions', which the caller must have
> + * initialized and is responsible for uninitializing. */
>  static void
> -subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet)
> +subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet,
> +                      struct ofpbuf *odp_actions)
>  {
>     struct facet *facet = subfacet->facet;
>     struct rule_dpif *rule = facet->rule;
>     struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
>
>     struct action_xlate_ctx ctx;
> -    uint64_t odp_actions_stub[1024 / 8];
> -    struct ofpbuf odp_actions;
>
> -    ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
>     action_xlate_ctx_init(&ctx, ofproto, &facet->flow, subfacet->initial_tci,
>                           rule, 0, packet);
> -    xlate_actions(&ctx, rule->up.actions, rule->up.n_actions, &odp_actions);
> +    xlate_actions(&ctx, rule->up.actions, rule->up.n_actions, odp_actions);
>     facet->tags = ctx.tags;
>     facet->may_install = ctx.may_set_up_flow;
>     facet->has_learn = ctx.has_learn;
> @@ -3975,14 +3985,12 @@ subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet)
>     facet->nf_flow.output_iface = ctx.nf_output_iface;
>     facet->mirrors = ctx.mirrors;
>
> -    if (subfacet->actions_len != odp_actions.size
> -        || memcmp(subfacet->actions, odp_actions.data, odp_actions.size)) {
> +    if (subfacet->actions_len != odp_actions->size
> +        || memcmp(subfacet->actions, odp_actions->data, odp_actions->size)) {
>         free(subfacet->actions);
> -        subfacet->actions_len = odp_actions.size;
> -        subfacet->actions = xmemdup(odp_actions.data, odp_actions.size);
> +        subfacet->actions_len = odp_actions->size;
> +        subfacet->actions = xmemdup(odp_actions->data, odp_actions->size);
>     }
> -
> -    ofpbuf_uninit(&odp_actions);
>  }
>
>  /* Updates 'subfacet''s datapath flow, setting its actions to 'actions_len'
> --
> 1.7.9
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev



More information about the dev mailing list