[ovs-dev] [PATCH] Add Nicira vendor extension actions NXAST_STACK_PUSH/POP

Ben Pfaff blp at nicira.com
Thu Feb 28 01:01:01 UTC 2013


On Tue, Feb 26, 2013 at 07:01:13PM -0800, Andy Zhou wrote:
> nicira-ext: Add Nicira actions NXAST_STACK_PUSH and NXAST_STACK_POP.
> 
> The Push action takes a single parameter. Any source allowed by NXAST_REG_MOVE
> is allowed to be pushed onto the stack. When the source is a bit field,  
> its value will be right shifted to bit zero before being pushed onto the 
> stack. The remaining bits will be set to zero.
> 
> The Pop action also takes a single parameter. Any destination allowed by
> NXAST_REG_MOVE can be used as the destination of the action. The value, in
> case of a bit field, will be taken from top of the stack, starting from
> bit zero.
> 
> The stack size is not limited. The initial 8KB is statically allocated to 
> efficiently handle most common use cases. When more stack space is 
> required, the stack can grow using malloc(). 
> 
> Each stack element is big enough to store the value of any meta flow field.

Thanks for working on this.  It looks pretty close to me.

"git am" says that this patch adds some trailing whitespace.  Please
remove it.

> Feature: #13861

We usually write this as "Feature #13061."

> Signed-off-by: azhou at nicira.com

The usual form includes name and email, like this: "Signed-off-by:
Andy Zhou <azhou at nicira.com>".  I think that a repository hooks
actually requires that.

>  /* Header for Nicira-defined actions. */
> @@ -560,6 +562,20 @@ struct nx_action_reg_load {
>  };
>  OFP_ASSERT(sizeof(struct nx_action_reg_load) == 24);
>  
> +/* Action structure for NXAST_STACK_PUSH.

I think this is used for NXAST_STACK_POP also.

> + * Pushes value[ofs:ofs+n_bits] onto the top of the stack.
> + */
> +struct nx_action_stack{
> +    ovs_be16 type;                  /* OFPAT_VENDOR. */
> +    ovs_be16 len;                   /* Length is 16. */
> +    ovs_be32 vendor;                /* NX_VENDOR_ID. */
> +    ovs_be16 subtype;               /* NXAST_REG_PUSH or NXAST_REG_POP. */
> +    ovs_be16 ofs_nbits;             /* (ofs << 6) | (n_bits - 1). */
> +    ovs_be32 reg;                   /* The register used for push or pop. */
> +};
> +OFP_ASSERT(sizeof(struct nx_action_stack) == 16);

The ofs_nbits form isn't sufficient to allow any field to be pushed or
popped: it only has 6 bits for n_bits so that limits the number of
bits to 64.  We use this form in a couple of places where that kind of
limitation is OK, but I'd rather have these actions be general-purpose
from the start.

I'd suggest using separate 16-bit ofs and nbits members.  That means
that we'll have to add padding to make nx_action_stack 24 bytes long,
but that's OK.

If for some reason we don't make this change then we'll have to modify
nxm_parse_stack_push() and nxm_parse_stack_pop() to enforce a 64-bit
width limit.

Regarding "struct ofpact_stack_push" and "struct ofpact_stack_pop", do
you think it is valuable to have two separate structures?  It looks to
me that it causes a lot of otherwise needless code duplication.  For
example, if these were just a single structure then most of
nxm_stack_push_from_openflow() and nxm_stack_pop_from_openflow() could
be common code, and similarly for nxm_stack_push_to_nxast() and
nxm_stack_pop_to_nxast().

> +static void
> +nx_stack_push(struct ofpbuf* stack, union mf_value * v)

Please write this as:
    nx_stack_push(struct ofpbuf *stack, union mf_value *v)
that is, the * declarator has a space before it and none after.

> +static union mf_value*
> +nx_stack_pop(struct ofpbuf* stack)
> +{
> +    union mf_value* v =  NULL;

Similarly in the above lines.

> +    if (stack->size) {
> +        stack -> size -= sizeof *v;
> +        v = (union mf_value*)((char*)stack->data + stack->size);

Also in the cast.  Also, no spaces around -> please.

(But you could use ofpbuf_tail(stack) in place of the explicit
calculation here.)

> +    }
> +
> +    return v;
> +}
> +
> +void
> +nxm_execute_stack_push(const struct ofpact_stack_push *push,
> +                       const struct flow *flow, struct ofpbuf *stack)
> +{
> +    union mf_value src_value;
> +    union mf_value dst_value;
> +
> +    if (stack == NULL) {

How would this happen?  I don't think it can.  If I'm right about
that, then it's not worth checking.

> +         char* flow_str = NULL;
> +         flow_str = flow_to_string(flow);
> +         VLOG_WARN_RL(&rl, "Push into a NULL stack. On flow \n"
> +                           " %s", flow_str);
> +         free(flow_str);
> +         return;
> +    }
> +
> +    mf_get_value(push->src.field, flow, &src_value);
> +    memset(&dst_value, 0, sizeof dst_value);
> +
> +    /* Copy into the lower order bits. */
> +    bitwise_copy(&src_value, push->src.field->n_bytes, push->src.ofs,
> +                 &dst_value, sizeof dst_value, 0, push->src.n_bits);

I think you can use mf_read_subfield() in place of the three calls
above.

> +    /* Push onto the stack */
> +    nx_stack_push(stack, &dst_value);
> +}
> +
> +void
> +nxm_execute_stack_pop(const struct ofpact_stack_pop *pop,
> +                      struct flow *flow, struct ofpbuf *stack )

Please remove the space before the ")".

> +{
> +    /* Only pop if stack is not empty.  */
> +    union mf_value* src_value;

"* " => " *".  I see some more like this, I won't point out every one.

> +    if (stack == NULL) {

Again, I don't think that 'stack' can be NULL here.

> +         char* flow_str = NULL;
> +         flow_str = flow_to_string(flow);
> +         VLOG_WARN_RL(&rl, "Pop from a NULL stack. On flow \n"
> +                           " %s", flow_str);
> +         free(flow_str);
> +         return;
> +    }
> +
> +    src_value =  nx_stack_pop(stack);

There's a doubled space on the line above.

> +    if (src_value) {
> +        union mf_value dst_value;
> +
> +         mf_get_value(pop->dst.field, flow, &dst_value);
> +         /* Each stack element is uint32_t */

I think the comment above is obsolete.

> +         bitwise_copy(src_value, sizeof *src_value, 0,
> +                 &dst_value, pop->dst.field->n_bytes, pop->dst.ofs,
> +                 pop->dst.n_bits);
> +         mf_set_flow_value(pop->dst.field, &dst_value, flow);

I think you can use mf_write_subfield_flow() in place of the three
calls above.

> +    } else {
> +         char* flow_str = NULL;
> +         flow_str = flow_to_string(flow);

It reads oddly to me to initialize to NULL then immediately assign a
value.  You can just initialize it to flow_to_string(flow).

> +         VLOG_WARN_RL(&rl, "Failed to pop from an empty stack. On flow \n"
> +                           " %s", flow_str);
> +         free(flow_str);
> +    }

The above warning is nice but it spends a bit of CPU time in the case
where the rate-limit causes the message to be dropped.  You can
instead use a construct like

        if (!VLOG_DROP_WARN(&rl)) {
            char *flow_str = flow_to_string(flow);
            VLOG_WARN(...);
            free(flow);
        }

to avoid that overhead.

> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index a4d7e59..d47e998 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -216,6 +216,19 @@ struct action_xlate_ctx {
>       * this flow when actions change header fields. */
>      struct flow flow;
>  
> +    /*
> +       The stack for the push and pop actions.
> +       Statically allocating 8KB byte stack within this structure.
> +       This size should be sufficient for normal use. However, if necessary,
> +       the stack can grow into larger size using malloc automatically.
> +
> +       Each stack element is of the type "union mf_value".
> +
> +       There are 8 bytes reserved at top of the stack for future use.
> +    */
> +    struct ofpbuf *stack;
> +    OFPBUF_STACK_BUFFER(init_stack, 1024);  /* Statically allocated stack */

It looks like we haven't really been using OFPBUF_STACK_BUFFER.  I
guess that I must have decided that explicit arrays were better.

At any rate, the argument to OFPBUF_STACK_BUFFER is a byte count, so
this is a 1 kB array not an 8 kB one.

Is there a reason to make 'stack' a pointer instead of putting it in
here "by value"?

The comment is a little more verbose than we'd normally make it.

What is the purpose of the 8-byte reservation?  I do not think that
its presence is externally observable, so I don't know why we would
want to reserve it.

> @@ -6570,12 +6594,19 @@ xlate_actions(struct action_xlate_ctx *ctx,
>      enum slow_path_reason special;
>      struct ofport_dpif *in_port;
>      struct flow orig_flow;
> +    struct ofpbuf action_stack;
> +    OFPBUF_STACK_BUFFER(init_stack, 1024);  /* Statically
> +                                               allocated stack */

Why do we have another stack buffer here?  It seems redundant with the
one in struct action_xlate_ctx, so I think we only need one or the
other.

>      COVERAGE_INC(ofproto_dpif_xlate);
>  
>      ofpbuf_clear(odp_actions);
>      ofpbuf_reserve(odp_actions, NL_A_U32_SIZE);
>  
> +    /* Set up action_stack, using the init_action_stack */
> +    ofpbuf_use_stack(&action_stack, init_stack, sizeof init_stack);

I think you want ofpbuf_use_stub() here instead.  ofpbuf_use_stack()
creates an buffer that cannot expand (you get an assertion failure
instead).

> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -45,6 +45,7 @@ in_port=12 actions=load:0x10->NXM_NX_REG0[[]],load:0x11->NXM_NX_REG1[[]],load:0x
>  in_port=13 actions=load:0x13->NXM_NX_REG3[[]],load:0x14->NXM_NX_REG4[[]],load:0x15->NXM_NX_REG5[[]]
>  in_port=14 actions=load:0x16->NXM_NX_REG6[[]],load:0x17->NXM_NX_REG7[[]]
>  in_port=15,reg0=0x10,reg1=0x11,reg2=0x12,reg3=0x13,reg4=0x14,reg5=0x15,reg6=0x16,reg7=0x17 actions=output:33
> +
>  ])

Any reason to add a blank line here?

>  AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
>  AT_CHECK([ovs-appctl ofproto/trace br0 'in_port(90),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])

Thanks for writing tests.

> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
> index 6589eff..fd3cec6 100644
> --- a/utilities/ovs-ofctl.8.in
> +++ b/utilities/ovs-ofctl.8.in
> @@ -1050,6 +1050,20 @@ in field \fIdst\fR.
>  Example: \fBload:55\->NXM_NX_REG2[0..5]\fR loads value 55 (bit pattern
>  \fB110111\fR) into bits 0 through 5, inclusive, in register 2.
>  .
> +.IP "\fBpush:\fIsrc\fB[\fIstart\fB..\fIend\fB]"
> +Pushes \fIstart\fR to \fIend\fR bits inclusive, in fields
> +on top of the stack.

The description should probably mention "src".

> +.IP
> +Example: \fBpush:\->NXM_NX_REG2[0..5]\fR push the vlaue stored register

s/vlaue/value/ here.  Maybe add "in" after "stored"?  Also I don't
think the syntax really uses "->"?

> +2 bits 0 through 5, inclusive, on to the internal stack.
> +.
> +.IP "\fBpop:\fIdst\fB[\fIstart\fB..\fIend\fB]"
> +Pops from the top of the stack, retrive the \fIstart\fR to \fIend\fR bits inclusive, from the value poped and store them into the corresponding bits in \fIdst\fR. 

s/retrive/retrieves/, s/poped/popped/, s/store/stores/, and I'd drop
the second comma.

Is the description correct?  I would have thought that this pops the
top of the stack, retrieves the *low-order bits* of what was popped,
and stores them into bits start...end of *dst*.

> +.IP
> +Example: \fBpop:\->NXM_NX_REG2[0..5]\fR pop the value from top of the stack,
> +Set register 2 bits 0 through 5, inclusive, based on bits 0 through 5 from the value just popd.

Again I don't think the "->" is part of the syntax?

Also more minor spelling and grammar errors.

Thanks,

Ben.



More information about the dev mailing list