| Message ID | 1446855055-38378-3-git-send-email-jrajahalme@nicira.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Nov 06, 2015 at 04:10:49PM -0800, Jarno Rajahalme wrote: > When modifying an existing datapath flow with recirculation actions, > the references to old (if any) recirculation actions need to be freed, > and references to new recirculation actions need to be stored. > > Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> > Acked-by: Joe Stringer <joestringer@nicira.com> Our coding style calls for a new-line after "void": > +static void reval_op_init(struct ukey_op *op, enum reval_result result, > + struct udpif *udpif, struct udpif_key *ukey, > + struct recirc_refs *recircs, > + struct ofpbuf *odp_actions) Here, it wasn't obvious to me why the logic changed from only allocating a recirc_id if we have a packet, to always allocating one (don't we still need to reuse the recirc id from a previous translation?): > @@ -3588,30 +3588,16 @@ compose_recirculate_action__(struct xlate_ctx *ctx, uint8_t table) > .ofpacts = ctx->action_set.data, > }; > > - /* Only allocate recirculation ID if we have a packet. */ > - if (ctx->xin->packet) { > - /* Allocate a unique recirc id for the given metadata state in the > - * flow. The life-cycle of this recirc id is managed by associating it > - * with the udpif key ('ukey') created for each new datapath flow. */ > - id = recirc_alloc_id_ctx(&state); > - if (!id) { > - XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id"); > - ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; > - return; > - } > - xlate_out_add_recirc(ctx->xout, id); > - } else { > - /* Look up an existing recirc id for the given metadata state in the > - * flow. No new reference is taken, as the ID is RCU protected and is > - * only required temporarily for verification. > - * If flow tables have changed sufficiently this can fail and we will > - * delete the old datapath flow. */ > - id = recirc_find_id(&state); > - if (!id) { > - ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; > - return; > - } > + /* Allocate a unique recirc id for the given metadata state in the > + * flow. The life-cycle of this recirc id is managed by associating it > + * with the udpif key ('ukey') created for each new datapath flow. */ > + id = recirc_alloc_id_ctx(&state); > + if (!id) { > + XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id"); > + ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; > + return; > } > + recirc_refs_add(&ctx->xout->recircs, id); Thanks, Ben.
> On Nov 24, 2015, at 10:27 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Fri, Nov 06, 2015 at 04:10:49PM -0800, Jarno Rajahalme wrote: >> When modifying an existing datapath flow with recirculation actions, >> the references to old (if any) recirculation actions need to be freed, >> and references to new recirculation actions need to be stored. >> >> Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com> >> Acked-by: Joe Stringer <joestringer@nicira.com> > > Our coding style calls for a new-line after "void": >> +static void reval_op_init(struct ukey_op *op, enum reval_result result, >> + struct udpif *udpif, struct udpif_key *ukey, >> + struct recirc_refs *recircs, >> + struct ofpbuf *odp_actions) > Fixed. > Here, it wasn't obvious to me why the logic changed from only allocating > a recirc_id if we have a packet, to always allocating one (don't we > still need to reuse the recirc id from a previous translation?): > The separation of the packet (upcall) and no packet (revalidation) was suitable before we added the support for modifying datapath flows in-place, when only actions change. Before, when doing revalidation the produced actions were only used for comparison, but now they can also be used as a replacement for the old datapath actions. This is why we now need to allocate and hold a reference to a recirculation context also when revalidating. The reference will be freed if the actions are freed without installing them to an existing flow. Also, the recirc_alloc_id_ctx() will reuse existing recirculation contexts (and adding a reference) if possible. I’ll update the comment to mention this. Jarno >> @@ -3588,30 +3588,16 @@ compose_recirculate_action__(struct xlate_ctx *ctx, uint8_t table) >> .ofpacts = ctx->action_set.data, >> }; >> >> - /* Only allocate recirculation ID if we have a packet. */ >> - if (ctx->xin->packet) { >> - /* Allocate a unique recirc id for the given metadata state in the >> - * flow. The life-cycle of this recirc id is managed by associating it >> - * with the udpif key ('ukey') created for each new datapath flow. */ >> - id = recirc_alloc_id_ctx(&state); >> - if (!id) { >> - XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id"); >> - ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; >> - return; >> - } >> - xlate_out_add_recirc(ctx->xout, id); >> - } else { >> - /* Look up an existing recirc id for the given metadata state in the >> - * flow. No new reference is taken, as the ID is RCU protected and is >> - * only required temporarily for verification. >> - * If flow tables have changed sufficiently this can fail and we will >> - * delete the old datapath flow. */ >> - id = recirc_find_id(&state); >> - if (!id) { >> - ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; >> - return; >> - } >> + /* Allocate a unique recirc id for the given metadata state in the >> + * flow. The life-cycle of this recirc id is managed by associating it >> + * with the udpif key ('ukey') created for each new datapath flow. */ >> + id = recirc_alloc_id_ctx(&state); >> + if (!id) { >> + XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id"); >> + ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; >> + return; >> } >> + recirc_refs_add(&ctx->xout->recircs, id); > > Thanks, > > Ben.
On Tue, Nov 24, 2015 at 01:10:35PM -0800, Jarno Rajahalme wrote: > > > On Nov 24, 2015, at 10:27 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Fri, Nov 06, 2015 at 04:10:49PM -0800, Jarno Rajahalme wrote: > >> When modifying an existing datapath flow with recirculation actions, > >> the references to old (if any) recirculation actions need to be freed, > >> and references to new recirculation actions need to be stored. > >> > > Here, it wasn't obvious to me why the logic changed from only allocating > > a recirc_id if we have a packet, to always allocating one (don't we > > still need to reuse the recirc id from a previous translation?): > > The separation of the packet (upcall) and no packet (revalidation) was > suitable before we added the support for modifying datapath flows > in-place, when only actions change. Before, when doing revalidation > the produced actions were only used for comparison, but now they can > also be used as a replacement for the old datapath actions. This is > why we now need to allocate and hold a reference to a recirculation > context also when revalidating. The reference will be freed if the > actions are freed without installing them to an existing flow. Also, > the recirc_alloc_id_ctx() will reuse existing recirculation contexts > (and adding a reference) if possible. I’ll update the comment to > mention this. If we always allocate a new recirc id, does that mean that the revalidated flow will always differ from the original one?
> On Nov 24, 2015, at 1:53 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Nov 24, 2015 at 01:10:35PM -0800, Jarno Rajahalme wrote: >> >>> On Nov 24, 2015, at 10:27 AM, Ben Pfaff <blp@ovn.org> wrote: >>> >>> On Fri, Nov 06, 2015 at 04:10:49PM -0800, Jarno Rajahalme wrote: >>>> When modifying an existing datapath flow with recirculation actions, >>>> the references to old (if any) recirculation actions need to be freed, >>>> and references to new recirculation actions need to be stored. >>>> >>> Here, it wasn't obvious to me why the logic changed from only allocating >>> a recirc_id if we have a packet, to always allocating one (don't we >>> still need to reuse the recirc id from a previous translation?): >> >> The separation of the packet (upcall) and no packet (revalidation) was >> suitable before we added the support for modifying datapath flows >> in-place, when only actions change. Before, when doing revalidation >> the produced actions were only used for comparison, but now they can >> also be used as a replacement for the old datapath actions. This is >> why we now need to allocate and hold a reference to a recirculation >> context also when revalidating. The reference will be freed if the >> actions are freed without installing them to an existing flow. Also, >> the recirc_alloc_id_ctx() will reuse existing recirculation contexts >> (and adding a reference) if possible. I’ll update the comment to >> mention this. > > If we always allocate a new recirc id, does that mean that the > revalidated flow will always differ from the original one? recirc_alloc_id_ctx() will reuse existing recirculation contexts (and adding a reference) if possible, so it will return the same recirculation Id if the post-recirculation processing will be the same. This already makes it possible for two different upcall paths to get the same recirculation ID and to share the same post-recirculation flow. Jarno
On Tue, Nov 24, 2015 at 02:25:47PM -0800, Jarno Rajahalme wrote: > > > On Nov 24, 2015, at 1:53 PM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Tue, Nov 24, 2015 at 01:10:35PM -0800, Jarno Rajahalme wrote: > >> > >>> On Nov 24, 2015, at 10:27 AM, Ben Pfaff <blp@ovn.org> wrote: > >>> > >>> On Fri, Nov 06, 2015 at 04:10:49PM -0800, Jarno Rajahalme wrote: > >>>> When modifying an existing datapath flow with recirculation actions, > >>>> the references to old (if any) recirculation actions need to be freed, > >>>> and references to new recirculation actions need to be stored. > >>>> > >>> Here, it wasn't obvious to me why the logic changed from only allocating > >>> a recirc_id if we have a packet, to always allocating one (don't we > >>> still need to reuse the recirc id from a previous translation?): > >> > >> The separation of the packet (upcall) and no packet (revalidation) was > >> suitable before we added the support for modifying datapath flows > >> in-place, when only actions change. Before, when doing revalidation > >> the produced actions were only used for comparison, but now they can > >> also be used as a replacement for the old datapath actions. This is > >> why we now need to allocate and hold a reference to a recirculation > >> context also when revalidating. The reference will be freed if the > >> actions are freed without installing them to an existing flow. Also, > >> the recirc_alloc_id_ctx() will reuse existing recirculation contexts > >> (and adding a reference) if possible. I’ll update the comment to > >> mention this. > > > > If we always allocate a new recirc id, does that mean that the > > revalidated flow will always differ from the original one? > > recirc_alloc_id_ctx() will reuse existing recirculation contexts (and adding a reference) if possible, so it will return the same recirculation Id if the post-recirculation processing will be the same. This already makes it possible for two different upcall paths to get the same recirculation ID and to share the same post-recirculation flow. Thanks for walking me through it. Acked-by: Ben Pfaff <blp@ovn.org>
> On Nov 24, 2015, at 3:56 PM, Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Nov 24, 2015 at 02:25:47PM -0800, Jarno Rajahalme wrote: >> >>> On Nov 24, 2015, at 1:53 PM, Ben Pfaff <blp@ovn.org> wrote: >>> >>> On Tue, Nov 24, 2015 at 01:10:35PM -0800, Jarno Rajahalme wrote: >>>> >>>>> On Nov 24, 2015, at 10:27 AM, Ben Pfaff <blp@ovn.org> wrote: >>>>> >>>>> On Fri, Nov 06, 2015 at 04:10:49PM -0800, Jarno Rajahalme wrote: >>>>>> When modifying an existing datapath flow with recirculation actions, >>>>>> the references to old (if any) recirculation actions need to be freed, >>>>>> and references to new recirculation actions need to be stored. >>>>>> >>>>> Here, it wasn't obvious to me why the logic changed from only allocating >>>>> a recirc_id if we have a packet, to always allocating one (don't we >>>>> still need to reuse the recirc id from a previous translation?): >>>> >>>> The separation of the packet (upcall) and no packet (revalidation) was >>>> suitable before we added the support for modifying datapath flows >>>> in-place, when only actions change. Before, when doing revalidation >>>> the produced actions were only used for comparison, but now they can >>>> also be used as a replacement for the old datapath actions. This is >>>> why we now need to allocate and hold a reference to a recirculation >>>> context also when revalidating. The reference will be freed if the >>>> actions are freed without installing them to an existing flow. Also, >>>> the recirc_alloc_id_ctx() will reuse existing recirculation contexts >>>> (and adding a reference) if possible. I’ll update the comment to >>>> mention this. >>> >>> If we always allocate a new recirc id, does that mean that the >>> revalidated flow will always differ from the original one? >> >> recirc_alloc_id_ctx() will reuse existing recirculation contexts (and adding a reference) if possible, so it will return the same recirculation Id if the post-recirculation processing will be the same. This already makes it possible for two different upcall paths to get the same recirculation ID and to share the same post-recirculation flow. > > Thanks for walking me through it. > > Acked-by: Ben Pfaff <blp@ovn.org <mailto:blp@ovn.org>> Thanks for taking the time to ask the questions. Pushed to master, Jarno
diff --git a/ofproto/ofproto-dpif-rid.h b/ofproto/ofproto-dpif-rid.h index 6ac5fef..135810d 100644 --- a/ofproto/ofproto-dpif-rid.h +++ b/ofproto/ofproto-dpif-rid.h @@ -196,4 +196,67 @@ void recirc_id_node_unref(const struct recirc_id_node *); void recirc_run(void); +/* Recirculation IDs on which references are held. */ +struct recirc_refs { + unsigned n_recircs; + union { + uint32_t recirc[2]; /* When n_recircs == 1 or 2 */ + uint32_t *recircs; /* When 'n_recircs' > 2 */ + }; +}; + +#define RECIRC_REFS_EMPTY_INITIALIZER ((struct recirc_refs) \ + { 0, { { 0, 0 } } }) +/* Helpers to abstract the recirculation union away. */ +static inline void +recirc_refs_init(struct recirc_refs *rr) +{ + *rr = RECIRC_REFS_EMPTY_INITIALIZER; +} + +static inline void +recirc_refs_add(struct recirc_refs *rr, uint32_t id) +{ + if (OVS_LIKELY(rr->n_recircs < ARRAY_SIZE(rr->recirc))) { + rr->recirc[rr->n_recircs++] = id; + } else { + if (rr->n_recircs == ARRAY_SIZE(rr->recirc)) { + uint32_t *recircs = xmalloc(sizeof rr->recirc + sizeof id); + + memcpy(recircs, rr->recirc, sizeof rr->recirc); + rr->recircs = recircs; + } else { + rr->recircs = xrealloc(rr->recircs, + (rr->n_recircs + 1) * sizeof id); + } + rr->recircs[rr->n_recircs++] = id; + } +} + +static inline void +recirc_refs_swap(struct recirc_refs *a, struct recirc_refs *b) +{ + struct recirc_refs tmp; + + tmp = *a; + *a = *b; + *b = tmp; +} + +static inline void +recirc_refs_unref(struct recirc_refs *rr) +{ + if (OVS_LIKELY(rr->n_recircs <= ARRAY_SIZE(rr->recirc))) { + for (int i = 0; i < rr->n_recircs; i++) { + recirc_free_id(rr->recirc[i]); + } + } else { + for (int i = 0; i < rr->n_recircs; i++) { + recirc_free_id(rr->recircs[i]); + } + free(rr->recircs); + } + rr->n_recircs = 0; +} + #endif diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 6aa1aad..2734696 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -261,9 +261,8 @@ struct udpif_key { struct nlattr nla; } keybuf, maskbuf; - /* Recirculation IDs with references held by the ukey. */ - unsigned n_recircs; - uint32_t recircs[]; /* 'n_recircs' id's for which references are held. */ + uint32_t key_recirc_id; /* Non-zero if reference is held by the ukey. */ + struct recirc_refs recircs; /* Action recirc IDs with references held. */ }; /* Datapath operation with optional ukey attached. */ @@ -1450,12 +1449,10 @@ ukey_create__(const struct nlattr *key, size_t key_len, bool ufid_present, const ovs_u128 *ufid, const unsigned pmd_id, const struct ofpbuf *actions, uint64_t dump_seq, uint64_t reval_seq, long long int used, - const struct recirc_id_node *key_recirc, struct xlate_out *xout) + uint32_t key_recirc_id, struct xlate_out *xout) OVS_NO_THREAD_SAFETY_ANALYSIS { - unsigned n_recircs = (key_recirc ? 1 : 0) + (xout ? xout->n_recircs : 0); - struct udpif_key *ukey = xmalloc(sizeof *ukey + - n_recircs * sizeof *ukey->recircs); + struct udpif_key *ukey = xmalloc(sizeof *ukey); memcpy(&ukey->keybuf, key, key_len); ukey->key = &ukey->keybuf.nla; @@ -1480,17 +1477,13 @@ ukey_create__(const struct nlattr *key, size_t key_len, ukey->stats.used = used; ukey->xcache = NULL; - ukey->n_recircs = n_recircs; - if (key_recirc) { - ukey->recircs[0] = key_recirc->id; + ukey->key_recirc_id = key_recirc_id; + recirc_refs_init(&ukey->recircs); + if (xout) { + /* Take ownership of the action recirc id references. */ + recirc_refs_swap(&ukey->recircs, &xout->recircs); } - if (xout && xout->n_recircs) { - const uint32_t *act_recircs = xlate_out_get_recircs(xout); - memcpy(ukey->recircs + (key_recirc ? 1 : 0), act_recircs, - xout->n_recircs * sizeof *ukey->recircs); - xlate_out_take_recircs(xout); - } return ukey; } @@ -1529,7 +1522,7 @@ ukey_create_from_upcall(struct upcall *upcall, struct flow_wildcards *wc) true, upcall->ufid, upcall->pmd_id, &upcall->put_actions, upcall->dump_seq, upcall->reval_seq, 0, - upcall->have_recirc_ref ? upcall->recirc : NULL, + upcall->have_recirc_ref ? upcall->recirc->id : 0, &upcall->xout); } @@ -1582,7 +1575,7 @@ ukey_create_from_dpif_flow(const struct udpif *udpif, *ukey = ukey_create__(flow->key, flow->key_len, flow->mask, flow->mask_len, flow->ufid_present, &flow->ufid, flow->pmd_id, &actions, dump_seq, - reval_seq, flow->stats.used, NULL, NULL); + reval_seq, flow->stats.used, 0, NULL); return 0; } @@ -1725,9 +1718,10 @@ ukey_delete__(struct udpif_key *ukey) OVS_NO_THREAD_SAFETY_ANALYSIS { if (ukey) { - for (int i = 0; i < ukey->n_recircs; i++) { - recirc_free_id(ukey->recircs[i]); + if (ukey->key_recirc_id) { + recirc_free_id(ukey->key_recirc_id); } + recirc_refs_unref(&ukey->recircs); xlate_cache_delete(ukey->xcache); ofpbuf_delete(ovsrcu_get(struct ofpbuf *, &ukey->actions)); ovs_mutex_destroy(&ukey->mutex); @@ -1784,11 +1778,21 @@ should_revalidate(const struct udpif *udpif, uint64_t packets, * UKEY_KEEP The ukey is fine as is. * UKEY_MODIFY The ukey's actions should be changed but is otherwise * fine. Callers should change the actions to those found - * in the caller supplied 'odp_actions' buffer. */ + * in the caller supplied 'odp_actions' buffer. The + * recirculation references can be found in 'recircs' and + * must be handled by the caller. + * + * If the result is UKEY_MODIFY, then references to all recirc_ids used by the + * new flow will be held within 'recircs' (which may be none). + * + * The caller is responsible for both initializing 'recircs' prior this call, + * and ensuring any references are eventually freed. + */ static enum reval_result revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey, const struct dpif_flow_stats *stats, - struct ofpbuf *odp_actions, uint64_t reval_seq) + struct ofpbuf *odp_actions, uint64_t reval_seq, + struct recirc_refs *recircs) OVS_REQUIRES(ukey->mutex) { struct xlate_out xout, *xoutp; @@ -1902,6 +1906,8 @@ revalidate_ukey(struct udpif *udpif, struct udpif_key *ukey, /* The datapath mask was OK, but the actions seem to have changed. * Let's modify it in place. */ result = UKEY_MODIFY; + /* Transfer recirc action ID references to the caller. */ + recirc_refs_swap(recircs, &xoutp->recircs); goto exit; } @@ -2073,6 +2079,25 @@ log_unexpected_flow(const struct dpif_flow *flow, int error) VLOG_WARN_RL(&rl, "%s", ds_cstr(&ds)); } +static void reval_op_init(struct ukey_op *op, enum reval_result result, + struct udpif *udpif, struct udpif_key *ukey, + struct recirc_refs *recircs, + struct ofpbuf *odp_actions) +{ + if (result == UKEY_DELETE) { + delete_op_init(udpif, op, ukey); + } else if (result == UKEY_MODIFY) { + /* Store the new recircs. */ + recirc_refs_swap(&ukey->recircs, recircs); + /* Release old recircs. */ + recirc_refs_unref(recircs); + /* ukey->key_recirc_id remains, as the key is the same as before. */ + + ukey_set_actions(ukey, odp_actions); + modify_op_init(op, ukey); + } +} + static void revalidate(struct revalidator *revalidator) { @@ -2126,6 +2151,7 @@ revalidate(struct revalidator *revalidator) for (f = flows; f < &flows[n_dumped]; f++) { long long int used = f->stats.used; + struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER; enum reval_result result; struct udpif_key *ukey; bool already_dumped; @@ -2165,16 +2191,15 @@ revalidate(struct revalidator *revalidator) result = UKEY_DELETE; } else { result = revalidate_ukey(udpif, ukey, &f->stats, &odp_actions, - reval_seq); + reval_seq, &recircs); } ukey->dump_seq = dump_seq; ukey->flow_exists = result != UKEY_DELETE; - if (result == UKEY_DELETE) { - delete_op_init(udpif, &ops[n_ops++], ukey); - } else if (result == UKEY_MODIFY) { - ukey_set_actions(ukey, &odp_actions); - modify_op_init(&ops[n_ops++], ukey); + if (result != UKEY_KEEP) { + /* Takes ownership of 'recircs'. */ + reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs, + &odp_actions); } ovs_mutex_unlock(&ukey->mutex); } @@ -2223,6 +2248,7 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) CMAP_FOR_EACH(ukey, cmap_node, &umap->cmap) { bool flow_exists, seq_mismatch; + struct recirc_refs recircs = RECIRC_REFS_EMPTY_INITIALIZER; enum reval_result result; /* Handler threads could be holding a ukey lock while it installs a @@ -2243,16 +2269,14 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge) COVERAGE_INC(revalidate_missed_dp_flow); memset(&stats, 0, sizeof stats); result = revalidate_ukey(udpif, ukey, &stats, &odp_actions, - reval_seq); + reval_seq, &recircs); } - ovs_mutex_unlock(&ukey->mutex); - - if (result == UKEY_DELETE) { - delete_op_init(udpif, &ops[n_ops++], ukey); - } else if (result == UKEY_MODIFY) { - ukey_set_actions(ukey, &odp_actions); - modify_op_init(&ops[n_ops++], ukey); + if (result != UKEY_KEEP) { + /* Takes ownership of 'recircs'. */ + reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs, + &odp_actions); } + ovs_mutex_unlock(&ukey->mutex); if (n_ops == REVALIDATE_MAX_BATCH) { push_ukey_ops(udpif, umap, ops, n_ops); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 192d0fa..6baea7f 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -3588,30 +3588,16 @@ compose_recirculate_action__(struct xlate_ctx *ctx, uint8_t table) .ofpacts = ctx->action_set.data, }; - /* Only allocate recirculation ID if we have a packet. */ - if (ctx->xin->packet) { - /* Allocate a unique recirc id for the given metadata state in the - * flow. The life-cycle of this recirc id is managed by associating it - * with the udpif key ('ukey') created for each new datapath flow. */ - id = recirc_alloc_id_ctx(&state); - if (!id) { - XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id"); - ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; - return; - } - xlate_out_add_recirc(ctx->xout, id); - } else { - /* Look up an existing recirc id for the given metadata state in the - * flow. No new reference is taken, as the ID is RCU protected and is - * only required temporarily for verification. - * If flow tables have changed sufficiently this can fail and we will - * delete the old datapath flow. */ - id = recirc_find_id(&state); - if (!id) { - ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; - return; - } + /* Allocate a unique recirc id for the given metadata state in the + * flow. The life-cycle of this recirc id is managed by associating it + * with the udpif key ('ukey') created for each new datapath flow. */ + id = recirc_alloc_id_ctx(&state); + if (!id) { + XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id"); + ctx->error = XLATE_NO_RECIRCULATION_CONTEXT; + return; } + recirc_refs_add(&ctx->xout->recircs, id); nl_msg_put_u32(ctx->odp_actions, OVS_ACTION_ATTR_RECIRC, id); @@ -4708,7 +4694,7 @@ void xlate_out_uninit(struct xlate_out *xout) { if (xout) { - xlate_out_free_recircs(xout); + recirc_refs_unref(&xout->recircs); } } @@ -4922,7 +4908,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout) *xout = (struct xlate_out) { .slow = 0, .fail_open = false, - .n_recircs = 0, + .recircs = RECIRC_REFS_EMPTY_INITIALIZER, }; struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp); diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h index ca63b1f..8aaae1a 100644 --- a/ofproto/ofproto-dpif-xlate.h +++ b/ofproto/ofproto-dpif-xlate.h @@ -41,68 +41,10 @@ struct xlate_out { enum slow_path_reason slow; /* 0 if fast path may be used. */ bool fail_open; /* Initial rule is fail open? */ - /* Recirculation IDs on which references are held. */ - unsigned n_recircs; - union { - uint32_t recirc[2]; /* When n_recircs == 1 or 2 */ - uint32_t *recircs; /* When 'n_recircs' > 2 */ - }; + struct recirc_refs recircs; /* Recirc action IDs on which references are + * held. */ }; -/* Helpers to abstract the recirculation union away. */ -static inline void -xlate_out_add_recirc(struct xlate_out *xout, uint32_t id) -{ - if (OVS_LIKELY(xout->n_recircs < ARRAY_SIZE(xout->recirc))) { - xout->recirc[xout->n_recircs++] = id; - } else { - if (xout->n_recircs == ARRAY_SIZE(xout->recirc)) { - uint32_t *recircs = xmalloc(sizeof xout->recirc + sizeof id); - - memcpy(recircs, xout->recirc, sizeof xout->recirc); - xout->recircs = recircs; - } else { - xout->recircs = xrealloc(xout->recircs, - (xout->n_recircs + 1) * sizeof id); - } - xout->recircs[xout->n_recircs++] = id; - } -} - -static inline const uint32_t * -xlate_out_get_recircs(const struct xlate_out *xout) -{ - if (OVS_LIKELY(xout->n_recircs <= ARRAY_SIZE(xout->recirc))) { - return xout->recirc; - } else { - return xout->recircs; - } -} - -static inline void -xlate_out_take_recircs(struct xlate_out *xout) -{ - if (OVS_UNLIKELY(xout->n_recircs > ARRAY_SIZE(xout->recirc))) { - free(xout->recircs); - } - xout->n_recircs = 0; -} - -static inline void -xlate_out_free_recircs(struct xlate_out *xout) -{ - if (OVS_LIKELY(xout->n_recircs <= ARRAY_SIZE(xout->recirc))) { - for (int i = 0; i < xout->n_recircs; i++) { - recirc_free_id(xout->recirc[i]); - } - } else { - for (int i = 0; i < xout->n_recircs; i++) { - recirc_free_id(xout->recircs[i]); - } - free(xout->recircs); - } -} - struct xlate_in { struct ofproto_dpif *ofproto;