[ovs-dev] [PATCH 34/41] ofproto-dpif-rid: Use array instead of ofpbuf for recirc_state stack.

[Jarno Rajahalme]

I see that the next patch fixed this bug,

  Jarno

> On Jan 20, 2016, at 3:49 PM, Jarno Rajahalme <jarno at ovn.org> wrote:
> 
> Nice simplification! With one bug fix below:
> 
> Acked-by: Jarno Rajahalme <jarno at ovn.org <mailto:jarno at ovn.org>>
> 
>> On Jan 18, 2016, at 11:27 PM, Ben Pfaff <blp at ovn.org> wrote:
>> 
>> In my opinion, this makes better sense for the stack, because it's not
>> a packet or a collection of bytes, it's an array of struct mf_subvalue.
>> (I left it as an ofpbuf for accumulating stack entries during
>> translation, because the automatic reallocation and especially the stub
>> support there is helpful.)
>> 
>> Signed-off-by: Ben Pfaff <blp at ovn.org>
>> ---
>> ofproto/ofproto-dpif-rid.c   | 17 ++++++++---------
>> ofproto/ofproto-dpif-rid.h   |  3 ++-
>> ofproto/ofproto-dpif-xlate.c |  6 ++++--
>> 3 files changed, 14 insertions(+), 12 deletions(-)
>> 
>> diff --git a/ofproto/ofproto-dpif-rid.c b/ofproto/ofproto-dpif-rid.c
>> index cb00301..f4dc21f 100644
>> --- a/ofproto/ofproto-dpif-rid.c
>> +++ b/ofproto/ofproto-dpif-rid.c
>> @@ -142,9 +142,9 @@ recirc_metadata_hash(const struct recirc_state *state)
>>    hash = hash_bytes64((const uint64_t *) &state->metadata.metadata,
>>                        sizeof state->metadata - sizeof state->metadata.tunnel,
>>                        hash);
>> -    if (state->stack && state->stack->size != 0) {
>> -        hash = hash_bytes64((const uint64_t *) state->stack->data,
>> -                            state->stack->size, hash);
>> +    if (state->stack && state->n_stack) {
>> +        hash = hash_bytes64((const uint64_t *) state->stack,
>> +                            state->n_stack * sizeof *state->stack, hash);
>>    }
>>    hash = hash_int(state->mirrors, hash);
>>    hash = hash_int(state->action_set_len, hash);
>> @@ -164,9 +164,8 @@ recirc_metadata_equal(const struct recirc_state *a,
>>            && flow_tnl_equal(a->metadata.tunnel, b->metadata.tunnel)
>>            && !memcmp(&a->metadata.metadata, &b->metadata.metadata,
>>                       sizeof a->metadata - sizeof a->metadata.tunnel)
>> -            && (((!a->stack || !a->stack->size) &&
>> -                 (!b->stack || !b->stack->size))
>> -                || (a->stack && b->stack && ofpbuf_equal(a->stack, b->stack)))
>> +            && a->n_stack == b->n_stack
>> +            && !memcmp(a->stack, b->stack, a->n_stack * sizeof *a->stack)
>>            && a->mirrors == b->mirrors
>>            && a->conntracked == b->conntracked
>>            && a->action_set_len == b->action_set_len
>> @@ -211,8 +210,8 @@ recirc_state_clone(struct recirc_state *new, const struct recirc_state *old,
>>    flow_tnl_copy__(tunnel, old->metadata.tunnel);
>>    new->metadata.tunnel = tunnel;
>> 
>> -    if (new->stack) {
>> -        new->stack = new->stack->size ? ofpbuf_clone(new->stack) : NULL;
>> +    if (new->n_stack) {
>> +        new->stack = xmemdup(new->stack, new->n_stack * sizeof *new->stack);
> 
> I needed to do this instead to avoid a core dump in test 860 (MPLS):
> 
> +    new->stack = new->n_stack
> +        ? xmemdup(new->stack, new->n_stack * sizeof *new->stack) : NULL;
> 
> The reason is that the input stack pointer may be non-NULL (pointing to a stubbed ofpbuf’s data), while n_stack is 0, so we need to rewrite the stack pointer with NULL in that case so that calling free() on it is safe.
> 
>>    }
>>    if (new->ofpacts) {
>>        new->ofpacts = (new->ofpacts_len
>> @@ -224,7 +223,7 @@ recirc_state_clone(struct recirc_state *new, const struct recirc_state *old,
>> static void
>> recirc_state_free(struct recirc_state *state)
>> {
>> -    ofpbuf_delete(state->stack);
>> +    free(state->stack);
>>    free(state->ofpacts);
>> }
>> 
>> diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h
>> index 4bbc7bf..101bd6e 100644
>> --- a/ofproto/ofproto-dpif-rid.h
>> +++ b/ofproto/ofproto-dpif-rid.h
>> @@ -140,7 +140,8 @@ struct recirc_state {
>>    /* Pipeline context for post-recirculation processing. */
>>    struct ofproto_dpif *ofproto; /* Post-recirculation bridge. */
>>    struct recirc_metadata metadata; /* Flow metadata. */
>> -    struct ofpbuf *stack;         /* Stack if any. */
>> +    union mf_subvalue *stack;     /* Stack if any. */
>> +    size_t n_stack;
>>    mirror_mask_t mirrors;        /* Mirrors already output. */
>>    bool conntracked;             /* Conntrack occurred prior to recirc. */
>> 
>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
>> index 2ee647b..6d67e91 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -3626,7 +3626,8 @@ compose_recirculate_action__(struct xlate_ctx *ctx, uint8_t table)
>>        .table_id = table,
>>        .ofproto = ctx->xbridge->ofproto,
>>        .metadata = md,
>> -        .stack = &ctx->stack,
>> +        .stack = ctx->stack.data,
>> +        .n_stack = ctx->stack.size / sizeof(union mf_subvalue),
>>        .mirrors = ctx->mirrors,
>>        .conntracked = ctx->conntracked,
>>        .action_set_len = ctx->recirc_action_offset,
>> @@ -5163,7 +5164,8 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
>> 
>>        /* Restore stack, if any. */
>>        if (state->stack) {
>> -            ofpbuf_put(&ctx.stack, state->stack->data, state->stack->size);
>> +            ofpbuf_put(&ctx.stack, state->stack,
>> +                       state->n_stack * sizeof *state->stack);
>>        }
>> 
>>        /* Restore mirror state. */
>> -- 
>> 2.1.3
>> 
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev