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

[Ben Pfaff]

Thanks, I pushed this out.

On Thu, Aug 12, 2010 at 02:52:30PM -0700, Justin Pettit wrote:
> 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
>