[ovs-dev] [PATCH] odp-util: Avoid branch in odp_actions_add().

[Justin Pettit]

I feel like there's a race going on between the rate at which you can write code and the rate at which I can review it--and I'm losing badly.  Other than depressing me, the code looks good.

--Justin


On Aug 12, 2010, at 2:28 PM, Ben Pfaff wrote:

> I have no idea why, but the test and branch in odp_actions_add() has always
> bugged me.  This commit eliminates it.
> ---
> lib/odp-util.c    |   11 ++++-------
> lib/odp-util.h    |    4 ++++
> ofproto/ofproto.c |    1 +
> 3 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index ccf05c6..442c939 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -30,13 +30,10 @@ union odp_action *
> odp_actions_add(struct odp_actions *actions, uint16_t type)
> {
>     union odp_action *a;
> -    if (actions->n_actions < MAX_ODP_ACTIONS) {
> -        a = &actions->actions[actions->n_actions++];
> -    } else {
> -        COVERAGE_INC(odp_overflow);
> -        actions->n_actions = MAX_ODP_ACTIONS + 1;
> -        a = &actions->actions[MAX_ODP_ACTIONS - 1];
> -    }
> +    size_t idx;
> +
> +    idx = actions->n_actions++ & (MAX_ODP_ACTIONS - 1);
> +    a = &actions->actions[idx];
>     memset(a, 0, sizeof *a);
>     a->type = type;
>     return a;
> diff --git a/lib/odp-util.h b/lib/odp-util.h
> index dc9a43d..420bde5 100644
> --- a/lib/odp-util.h
> +++ b/lib/odp-util.h
> @@ -22,6 +22,7 @@
> #include <stdint.h>
> #include "openflow/openflow.h"
> #include "openvswitch/datapath-protocol.h"
> +#include "util.h"
> 
> struct ds;
> 
> @@ -29,6 +30,9 @@ struct ds;
>  * memory, so there is no point in allocating more than that.  */
> enum { MAX_ODP_ACTIONS = 4096 / sizeof(union odp_action) };
> 
> +/* odp_actions_add() assumes that MAX_ODP_ACTIONS is a power of 2. */
> +BUILD_ASSERT_DECL(IS_POW2(MAX_ODP_ACTIONS));
> +
> struct odp_actions {
>     size_t n_actions;
>     union odp_action actions[MAX_ODP_ACTIONS];
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index a466c9c..130bc98 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2776,6 +2776,7 @@ xlate_actions(const union ofp_action *in, size_t n_in,
>         *nf_output_iface = ctx.nf_output_iface;
>     }
>     if (odp_actions_overflow(out)) {
> +        COVERAGE_INC(odp_overflow);
>         odp_actions_init(out);
>         return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_TOO_MANY);
>     }
> -- 
> 1.7.1
> 
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org