| Message ID | 20230518200834.37806-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] ofproto-dpif-xlate: Reduce stack usage in recursive xlate functions | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On Thu, May 18, 2023 at 04:08:34PM -0400, Mike Pattrick wrote: > Several xlate actions used in recursive translation currently store a > large amount of information on the stack. This can result in handler > threads quickly running out of stack space despite before > xlate_resubmit_resource_check() is able to terminate translation. This > patch reduces stack usage by over 3kb from several translation actions. > > This patch also moves some trace function from do_xlate_actions into its > own function to reduce stack usage. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2104779 > Signed-off-by: Mike Pattrick <mkp@redhat.com> > > --- > > Since v1: > - Refactored code into specialized functions and renamed variables for > improved readability. > > --- > ofproto/ofproto-dpif-xlate.c | 223 ++++++++++++++++++++++------------- > 1 file changed, 141 insertions(+), 82 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 29f4daa63..abfa717e2 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -501,6 +501,90 @@ ctx_cancel_freeze(struct xlate_ctx *ctx) > > static void finish_freezing(struct xlate_ctx *ctx); > > +/* These functions and structure are used to save stack space in actions that > + * need to retain a large amount of xlate_ctx state. */ > +struct xretained_state { > + union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)]; > + uint64_t actset_stub[1024 / 8]; > + struct ofpbuf old_stack; > + struct ofpbuf old_action_set; > + struct flow old_flow; > + struct flow old_base; > + struct flow_tnl flow_tnl; > +}; > + > +/* The return of this function must be freed by xretain_state_restore(). */ > +static inline struct xretained_state * > +xretain_state_save(struct xlate_ctx *ctx) Hi Mike, nit: I don't think the inline keyword is needed here, or elsewhere in this patch. Because the compiler should be able to figure this out. So unless there is a strong reason I'd prefer to drop 'inline'. Otherwise the patch looks good to me.
On 18 May 2023, at 22:08, Mike Pattrick wrote: > Several xlate actions used in recursive translation currently store a > large amount of information on the stack. This can result in handler > threads quickly running out of stack space despite before > xlate_resubmit_resource_check() is able to terminate translation. This > patch reduces stack usage by over 3kb from several translation actions. > > This patch also moves some trace function from do_xlate_actions into its > own function to reduce stack usage. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2104779 > Signed-off-by: Mike Pattrick <mkp@redhat.com> Thanks for taking a look at this, and fixing it. Some comments below. //Eelco > --- > > Since v1: > - Refactored code into specialized functions and renamed variables for > improved readability. > > --- > ofproto/ofproto-dpif-xlate.c | 223 ++++++++++++++++++++++------------- > 1 file changed, 141 insertions(+), 82 deletions(-) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 29f4daa63..abfa717e2 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -501,6 +501,90 @@ ctx_cancel_freeze(struct xlate_ctx *ctx) > > static void finish_freezing(struct xlate_ctx *ctx); > > +/* These functions and structure are used to save stack space in actions that > + * need to retain a large amount of xlate_ctx state. */ > +struct xretained_state { > + union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)]; > + uint64_t actset_stub[1024 / 8]; > + struct ofpbuf old_stack; > + struct ofpbuf old_action_set; > + struct flow old_flow; > + struct flow old_base; > + struct flow_tnl flow_tnl; As this is the tunnel mask data, maybe add this to the name, i.e., flow_tnl_mask? > +}; > + > +/* The return of this function must be freed by xretain_state_restore(). */ > +static inline struct xretained_state * > +xretain_state_save(struct xlate_ctx *ctx) > +{ > + struct xretained_state *retained = xmalloc(sizeof *retained); > + > + retained->old_flow = ctx->xin->flow; > + retained->old_stack = ctx->stack; > + retained->old_action_set = ctx->action_set; > + ofpbuf_use_stub(&ctx->stack, retained->new_stack, > + sizeof retained->new_stack); > + ofpbuf_use_stub(&ctx->action_set, retained->actset_stub, > + sizeof retained->actset_stub); > + > + return retained; > +} > + > +static inline void > +xretain_tunnel_save(struct xlate_ctx *ctx, struct xretained_state *retained, Same as for the structure, should we explicitly state it’s tunnel mask info we store, i.e., xretain_tunnel_mask_save()? > + const struct xport *out_dev) > +{ > + struct flow *flow = &ctx->xin->flow; > + > + retained->flow_tnl = ctx->wc->masks.tunnel; Up till here, it’s an actual save, all the below is clearing existing data. Maybe this should be part of a separate function? Also because the corresponding xretain_tunnel_restore() is not taking care of any of this, which seems odd from an API perspective. > + memset(&ctx->wc->masks.tunnel, 0, sizeof ctx->wc->masks.tunnel); > + > + memset(&flow->tunnel, 0, sizeof flow->tunnel); > + flow->tunnel.metadata.tab > + = ofproto_get_tun_tab(&out_dev->xbridge->ofproto->up); > + ctx->wc->masks.tunnel.metadata.tab = flow->tunnel.metadata.tab; > +} > + > +static inline void > +xretain_base_save(const struct xlate_ctx *ctx, Can we name this base_flow_save() as we have not real clue what base refers to from the API name. > + struct xretained_state *retained) > +{ > + retained->old_base = ctx->base_flow; > +} > + > +static inline void > +xretain_base_restore(struct xlate_ctx *ctx, > + const struct xretained_state *retained) > +{ > + ctx->base_flow = retained->old_base; > +} > + > +static inline void > +xretain_flow_restore(struct xlate_ctx *ctx, > + const struct xretained_state *retained) > +{ > + ctx->xin->flow = retained->old_flow; > +} > + > +static inline void > +xretain_tunnel_restore(struct xlate_ctx *ctx, > + const struct xretained_state *retained) > +{ > + ctx->wc->masks.tunnel = retained->flow_tnl; > +} > + > +static inline void > +xretain_state_restore(struct xlate_ctx *ctx, struct xretained_state *retained) > +{ > + ctx->xin->flow = retained->old_flow; > + ofpbuf_uninit(&ctx->action_set); > + ctx->action_set = retained->old_action_set; > + ofpbuf_uninit(&ctx->stack); > + ctx->stack = retained->old_stack; > + > + free(retained); > +} > + > /* A controller may use OFPP_NONE as the ingress port to indicate that > * it did not arrive on a "real" port. 'ofpp_none_bundle' exists for > * when an input bundle is needed for validation (e.g., mirroring or > @@ -3915,27 +3999,19 @@ static void > patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, > struct xport *out_dev, bool is_last_action) > { > - struct flow *flow = &ctx->xin->flow; > - struct flow old_flow = ctx->xin->flow; > - struct flow_tnl old_flow_tnl_wc = ctx->wc->masks.tunnel; > + struct ovs_list *old_trace = ctx->xin->trace; > bool old_conntrack = ctx->conntracked; > + struct flow *flow = &ctx->xin->flow; > bool old_was_mpls = ctx->was_mpls; > - ovs_version_t old_version = ctx->xin->tables_version; > - struct ofpbuf old_stack = ctx->stack; > - uint8_t new_stack[1024]; > - struct ofpbuf old_action_set = ctx->action_set; > - struct ovs_list *old_trace = ctx->xin->trace; > - uint64_t actset_stub[1024 / 8]; > + struct xretained_state *retained_state; > + > + retained_state = xretain_state_save(ctx); > + > + xretain_tunnel_save(ctx, retained_state, out_dev); > > - ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); > - ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); > + ovs_version_t old_version = ctx->xin->tables_version; As this is a variable declaration and assignment, I think this should still be at the top. > flow->in_port.ofp_port = out_dev->ofp_port; > flow->metadata = htonll(0); > - memset(&flow->tunnel, 0, sizeof flow->tunnel); > - memset(&ctx->wc->masks.tunnel, 0, sizeof ctx->wc->masks.tunnel); > - flow->tunnel.metadata.tab = > - ofproto_get_tun_tab(&out_dev->xbridge->ofproto->up); > - ctx->wc->masks.tunnel.metadata.tab = flow->tunnel.metadata.tab; > memset(flow->regs, 0, sizeof flow->regs); > flow->actset_output = OFPP_UNSET; > clear_conntrack(ctx); > @@ -3967,14 +4043,14 @@ patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, > } else { > /* Forwarding is disabled by STP and RSTP. Let OFPP_NORMAL and > * the learning action look at the packet, then drop it. */ > - struct flow old_base_flow = ctx->base_flow; > size_t old_size = ctx->odp_actions->size; > + xretain_base_save(ctx, retained_state); > mirror_mask_t old_mirrors2 = ctx->mirrors; > > xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true, > false, is_last_action, clone_xlate_actions); > ctx->mirrors = old_mirrors2; > - ctx->base_flow = old_base_flow; > + xretain_base_restore(ctx, retained_state); > ctx->odp_actions->size = old_size; > > /* Undo changes that may have been done for freezing. */ > @@ -3986,18 +4062,15 @@ patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, > if (independent_mirrors) { > ctx->mirrors = old_mirrors; > } > - ctx->xin->flow = old_flow; > ctx->xbridge = in_dev->xbridge; > - ofpbuf_uninit(&ctx->action_set); > - ctx->action_set = old_action_set; > - ofpbuf_uninit(&ctx->stack); > - ctx->stack = old_stack; > > /* Restore calling bridge's lookup version. */ > ctx->xin->tables_version = old_version; > > - /* Restore to calling bridge tunneling information */ > - ctx->wc->masks.tunnel = old_flow_tnl_wc; > + /* Restore to calling bridge tunneling information; the ctx flow, actions, > + * and stack. And free the retained state. */ > + xretain_tunnel_restore(ctx, retained_state); > + xretain_state_restore(ctx, retained_state); > > /* The out bridge popping MPLS should have no effect on the original > * bridge. */ > @@ -4247,7 +4320,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, > const struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port); > struct flow_wildcards *wc = ctx->wc; > struct flow *flow = &ctx->xin->flow; > - struct flow_tnl flow_tnl; > + struct flow_tnl *flow_tnl = NULL; > union flow_vlan_hdr flow_vlans[FLOW_MAX_VLAN_HEADERS]; > uint8_t flow_nw_tos; > odp_port_t out_port, odp_port, odp_tnl_port; > @@ -4261,7 +4334,6 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, > /* If 'struct flow' gets additional metadata, we'll need to zero it out > * before traversing a patch port. */ > BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42); > - memset(&flow_tnl, 0, sizeof flow_tnl); > > if (!check_output_prerequisites(ctx, xport, flow, check_stp)) { > return; > @@ -4305,7 +4377,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, > * the Logical (tunnel) Port are not visible for any further > * matches, while explicit set actions on tunnel metadata are. > */ > - flow_tnl = flow->tunnel; > + flow_tnl = xmalloc(sizeof *flow_tnl); > + *flow_tnl = flow->tunnel; > odp_port = tnl_port_send(xport->ofport, flow, ctx->wc); > if (odp_port == ODPP_NONE) { > xlate_report(ctx, OFT_WARN, "Tunneling decided against output"); > @@ -4336,7 +4409,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, > tnl_type = tnl_port_get_type(xport->ofport); > commit_odp_tunnel_action(flow, &ctx->base_flow, > ctx->odp_actions, tnl_type); > - flow->tunnel = flow_tnl; /* Restore tunnel metadata */ > + flow->tunnel = *flow_tnl; /* Restore tunnel metadata */ Might as well add a . at the end of the comment. > } > } else { > odp_port = xport->odp_port; > @@ -4380,7 +4453,11 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, > /* Output to native tunnel port. */ > native_tunnel_output(ctx, xport, flow, odp_port, truncate, > is_last_action); > - flow->tunnel = flow_tnl; /* Restore tunnel metadata */ > + if (flow_tnl) { > + flow->tunnel = *flow_tnl; /* Restore tunnel metadata */ > + } else { > + memset(&flow->tunnel, 0, sizeof flow->tunnel); > + } > > } else if (terminate_native_tunnel(ctx, xport, flow, wc, > &odp_tnl_port)) { > @@ -4423,7 +4500,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, > xport->xbundle)); > } > > - out: > +out: > /* Restore flow */ > memcpy(flow->vlans, flow_vlans, sizeof flow->vlans); > flow->nw_tos = flow_nw_tos; > @@ -4431,6 +4508,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, > flow->dl_src = flow_dl_src; > flow->packet_type = flow_packet_type; > flow->dl_type = flow_dl_type; > + free(flow_tnl); > } > > static void > @@ -5874,21 +5952,12 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, > struct xlate_ctx *ctx, bool is_last_action, > bool group_bucket_action OVS_UNUSED) > { > - struct ofpbuf old_stack = ctx->stack; > - union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)]; > - ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); > - ofpbuf_put(&ctx->stack, old_stack.data, old_stack.size); > - > - struct ofpbuf old_action_set = ctx->action_set; > - uint64_t actset_stub[1024 / 8]; > - ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); > - ofpbuf_put(&ctx->action_set, old_action_set.data, old_action_set.size); > - > + struct xretained_state *retained_state; > size_t offset, ac_offset; > - struct flow old_flow = ctx->xin->flow; > + > + retained_state = xretain_state_save(ctx); > > if (reversible_actions(actions, actions_len) || is_last_action) { > - old_flow = ctx->xin->flow; > do_xlate_actions(actions, actions_len, ctx, is_last_action, false); > if (!ctx->freezing) { > xlate_action_set(ctx); > @@ -5903,7 +5972,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, > * avoid emitting those actions twice. Once inside > * the clone, another time for the action after clone. */ > xlate_commit_actions(ctx); > - struct flow old_base = ctx->base_flow; > + xretain_base_save(ctx, retained_state); Can we add an empty line here to differentiate between functions being called and inline variable declaration? > bool old_was_mpls = ctx->was_mpls; > bool old_conntracked = ctx->conntracked; > > @@ -5960,14 +6029,10 @@ dp_clone_done: > ctx->was_mpls = old_was_mpls; > > /* Restore the 'base_flow' for the next action. */ > - ctx->base_flow = old_base; > + xretain_base_restore(ctx, retained_state); > > xlate_done: > - ofpbuf_uninit(&ctx->action_set); > - ctx->action_set = old_action_set; > - ofpbuf_uninit(&ctx->stack); > - ctx->stack = old_stack; > - ctx->xin->flow = old_flow; > + xretain_state_restore(ctx, retained_state); > } > > static void > @@ -6471,19 +6536,12 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, > return; > } > > - struct ofpbuf old_stack = ctx->stack; > - union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)]; > - ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); > - ofpbuf_put(&ctx->stack, old_stack.data, old_stack.size); > + struct xretained_state *retained_state; > > - struct ofpbuf old_action_set = ctx->action_set; > - uint64_t actset_stub[1024 / 8]; > - ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); > - ofpbuf_put(&ctx->action_set, old_action_set.data, old_action_set.size); > + retained_state = xretain_state_save(ctx); > > - struct flow old_flow = ctx->xin->flow; > xlate_commit_actions(ctx); > - struct flow old_base = ctx->base_flow; > + xretain_base_save(ctx, retained_state); Can we add an empty line here to differentiate between functions being called and inline variable declaration? > bool old_was_mpls = ctx->was_mpls; > bool old_conntracked = ctx->conntracked; > > @@ -6504,10 +6562,10 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, > } > nl_msg_end_nested(ctx->odp_actions, offset_attr); > > - ctx->base_flow = old_base; > + xretain_base_restore(ctx, retained_state); > + xretain_flow_restore(ctx, retained_state); > ctx->was_mpls = old_was_mpls; > ctx->conntracked = old_conntracked; > - ctx->xin->flow = old_flow; > > /* If the flow translation for the IF_GREATER case requires freezing, > * then ctx->exit would be true. Reset to false so that we can > @@ -6530,15 +6588,11 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, > nl_msg_end_nested(ctx->odp_actions, offset_attr); > nl_msg_end_nested(ctx->odp_actions, offset); > > - ofpbuf_uninit(&ctx->action_set); > - ctx->action_set = old_action_set; > - ofpbuf_uninit(&ctx->stack); > - ctx->stack = old_stack; > - ctx->base_flow = old_base; > ctx->was_mpls = old_was_mpls; > ctx->conntracked = old_conntracked; > - ctx->xin->flow = old_flow; > ctx->exit = old_exit; > + xretain_base_restore(ctx, retained_state); > + xretain_state_restore(ctx, retained_state); > } > > static void > @@ -6989,6 +7043,24 @@ xlate_ofpact_unroll_xlate(struct xlate_ctx *ctx, > "cookie=%#"PRIx64, a->rule_table_id, a->rule_cookie); > } > > +static void > +xlate_trace(struct xlate_ctx *ctx, const struct ofpact *a) { > + struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); > + > + if (ctx->xin->names) { > + struct ofproto_dpif *ofprotop; > + ofprotop = ofproto_dpif_lookup_by_name(ctx->xbridge->name); > + ofproto_append_ports_to_map(&map, ofprotop->up.ports); > + } > + > + struct ds s = DS_EMPTY_INITIALIZER; > + struct ofpact_format_params fp = { .s = &s, .port_map = &map }; > + ofpacts_format(a, OFPACT_ALIGN(a->len), &fp); > + xlate_report(ctx, OFT_ACTION, "%s", ds_cstr(&s)); > + ds_destroy(&s); > + ofputil_port_map_destroy(&map); > +} > + > static void > do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, > struct xlate_ctx *ctx, bool is_last_action, > @@ -7031,20 +7103,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, > } > > if (OVS_UNLIKELY(ctx->xin->trace)) { > - struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); > - > - if (ctx->xin->names) { > - struct ofproto_dpif *ofprotop; > - ofprotop = ofproto_dpif_lookup_by_name(ctx->xbridge->name); > - ofproto_append_ports_to_map(&map, ofprotop->up.ports); > - } > - > - struct ds s = DS_EMPTY_INITIALIZER; > - struct ofpact_format_params fp = { .s = &s, .port_map = &map }; > - ofpacts_format(a, OFPACT_ALIGN(a->len), &fp); > - xlate_report(ctx, OFT_ACTION, "%s", ds_cstr(&s)); > - ds_destroy(&s); > - ofputil_port_map_destroy(&map); > + xlate_trace(ctx, a); > } > > switch (a->type) { > -- > 2.31.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 29f4daa63..abfa717e2 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -501,6 +501,90 @@ ctx_cancel_freeze(struct xlate_ctx *ctx) static void finish_freezing(struct xlate_ctx *ctx); +/* These functions and structure are used to save stack space in actions that + * need to retain a large amount of xlate_ctx state. */ +struct xretained_state { + union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)]; + uint64_t actset_stub[1024 / 8]; + struct ofpbuf old_stack; + struct ofpbuf old_action_set; + struct flow old_flow; + struct flow old_base; + struct flow_tnl flow_tnl; +}; + +/* The return of this function must be freed by xretain_state_restore(). */ +static inline struct xretained_state * +xretain_state_save(struct xlate_ctx *ctx) +{ + struct xretained_state *retained = xmalloc(sizeof *retained); + + retained->old_flow = ctx->xin->flow; + retained->old_stack = ctx->stack; + retained->old_action_set = ctx->action_set; + ofpbuf_use_stub(&ctx->stack, retained->new_stack, + sizeof retained->new_stack); + ofpbuf_use_stub(&ctx->action_set, retained->actset_stub, + sizeof retained->actset_stub); + + return retained; +} + +static inline void +xretain_tunnel_save(struct xlate_ctx *ctx, struct xretained_state *retained, + const struct xport *out_dev) +{ + struct flow *flow = &ctx->xin->flow; + + retained->flow_tnl = ctx->wc->masks.tunnel; + memset(&ctx->wc->masks.tunnel, 0, sizeof ctx->wc->masks.tunnel); + + memset(&flow->tunnel, 0, sizeof flow->tunnel); + flow->tunnel.metadata.tab + = ofproto_get_tun_tab(&out_dev->xbridge->ofproto->up); + ctx->wc->masks.tunnel.metadata.tab = flow->tunnel.metadata.tab; +} + +static inline void +xretain_base_save(const struct xlate_ctx *ctx, + struct xretained_state *retained) +{ + retained->old_base = ctx->base_flow; +} + +static inline void +xretain_base_restore(struct xlate_ctx *ctx, + const struct xretained_state *retained) +{ + ctx->base_flow = retained->old_base; +} + +static inline void +xretain_flow_restore(struct xlate_ctx *ctx, + const struct xretained_state *retained) +{ + ctx->xin->flow = retained->old_flow; +} + +static inline void +xretain_tunnel_restore(struct xlate_ctx *ctx, + const struct xretained_state *retained) +{ + ctx->wc->masks.tunnel = retained->flow_tnl; +} + +static inline void +xretain_state_restore(struct xlate_ctx *ctx, struct xretained_state *retained) +{ + ctx->xin->flow = retained->old_flow; + ofpbuf_uninit(&ctx->action_set); + ctx->action_set = retained->old_action_set; + ofpbuf_uninit(&ctx->stack); + ctx->stack = retained->old_stack; + + free(retained); +} + /* A controller may use OFPP_NONE as the ingress port to indicate that * it did not arrive on a "real" port. 'ofpp_none_bundle' exists for * when an input bundle is needed for validation (e.g., mirroring or @@ -3915,27 +3999,19 @@ static void patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, struct xport *out_dev, bool is_last_action) { - struct flow *flow = &ctx->xin->flow; - struct flow old_flow = ctx->xin->flow; - struct flow_tnl old_flow_tnl_wc = ctx->wc->masks.tunnel; + struct ovs_list *old_trace = ctx->xin->trace; bool old_conntrack = ctx->conntracked; + struct flow *flow = &ctx->xin->flow; bool old_was_mpls = ctx->was_mpls; - ovs_version_t old_version = ctx->xin->tables_version; - struct ofpbuf old_stack = ctx->stack; - uint8_t new_stack[1024]; - struct ofpbuf old_action_set = ctx->action_set; - struct ovs_list *old_trace = ctx->xin->trace; - uint64_t actset_stub[1024 / 8]; + struct xretained_state *retained_state; + + retained_state = xretain_state_save(ctx); + + xretain_tunnel_save(ctx, retained_state, out_dev); - ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); - ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); + ovs_version_t old_version = ctx->xin->tables_version; flow->in_port.ofp_port = out_dev->ofp_port; flow->metadata = htonll(0); - memset(&flow->tunnel, 0, sizeof flow->tunnel); - memset(&ctx->wc->masks.tunnel, 0, sizeof ctx->wc->masks.tunnel); - flow->tunnel.metadata.tab = - ofproto_get_tun_tab(&out_dev->xbridge->ofproto->up); - ctx->wc->masks.tunnel.metadata.tab = flow->tunnel.metadata.tab; memset(flow->regs, 0, sizeof flow->regs); flow->actset_output = OFPP_UNSET; clear_conntrack(ctx); @@ -3967,14 +4043,14 @@ patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, } else { /* Forwarding is disabled by STP and RSTP. Let OFPP_NORMAL and * the learning action look at the packet, then drop it. */ - struct flow old_base_flow = ctx->base_flow; size_t old_size = ctx->odp_actions->size; + xretain_base_save(ctx, retained_state); mirror_mask_t old_mirrors2 = ctx->mirrors; xlate_table_action(ctx, flow->in_port.ofp_port, 0, true, true, false, is_last_action, clone_xlate_actions); ctx->mirrors = old_mirrors2; - ctx->base_flow = old_base_flow; + xretain_base_restore(ctx, retained_state); ctx->odp_actions->size = old_size; /* Undo changes that may have been done for freezing. */ @@ -3986,18 +4062,15 @@ patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, if (independent_mirrors) { ctx->mirrors = old_mirrors; } - ctx->xin->flow = old_flow; ctx->xbridge = in_dev->xbridge; - ofpbuf_uninit(&ctx->action_set); - ctx->action_set = old_action_set; - ofpbuf_uninit(&ctx->stack); - ctx->stack = old_stack; /* Restore calling bridge's lookup version. */ ctx->xin->tables_version = old_version; - /* Restore to calling bridge tunneling information */ - ctx->wc->masks.tunnel = old_flow_tnl_wc; + /* Restore to calling bridge tunneling information; the ctx flow, actions, + * and stack. And free the retained state. */ + xretain_tunnel_restore(ctx, retained_state); + xretain_state_restore(ctx, retained_state); /* The out bridge popping MPLS should have no effect on the original * bridge. */ @@ -4247,7 +4320,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, const struct xport *xport = get_ofp_port(ctx->xbridge, ofp_port); struct flow_wildcards *wc = ctx->wc; struct flow *flow = &ctx->xin->flow; - struct flow_tnl flow_tnl; + struct flow_tnl *flow_tnl = NULL; union flow_vlan_hdr flow_vlans[FLOW_MAX_VLAN_HEADERS]; uint8_t flow_nw_tos; odp_port_t out_port, odp_port, odp_tnl_port; @@ -4261,7 +4334,6 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, /* If 'struct flow' gets additional metadata, we'll need to zero it out * before traversing a patch port. */ BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42); - memset(&flow_tnl, 0, sizeof flow_tnl); if (!check_output_prerequisites(ctx, xport, flow, check_stp)) { return; @@ -4305,7 +4377,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, * the Logical (tunnel) Port are not visible for any further * matches, while explicit set actions on tunnel metadata are. */ - flow_tnl = flow->tunnel; + flow_tnl = xmalloc(sizeof *flow_tnl); + *flow_tnl = flow->tunnel; odp_port = tnl_port_send(xport->ofport, flow, ctx->wc); if (odp_port == ODPP_NONE) { xlate_report(ctx, OFT_WARN, "Tunneling decided against output"); @@ -4336,7 +4409,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, tnl_type = tnl_port_get_type(xport->ofport); commit_odp_tunnel_action(flow, &ctx->base_flow, ctx->odp_actions, tnl_type); - flow->tunnel = flow_tnl; /* Restore tunnel metadata */ + flow->tunnel = *flow_tnl; /* Restore tunnel metadata */ } } else { odp_port = xport->odp_port; @@ -4380,7 +4453,11 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, /* Output to native tunnel port. */ native_tunnel_output(ctx, xport, flow, odp_port, truncate, is_last_action); - flow->tunnel = flow_tnl; /* Restore tunnel metadata */ + if (flow_tnl) { + flow->tunnel = *flow_tnl; /* Restore tunnel metadata */ + } else { + memset(&flow->tunnel, 0, sizeof flow->tunnel); + } } else if (terminate_native_tunnel(ctx, xport, flow, wc, &odp_tnl_port)) { @@ -4423,7 +4500,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, xport->xbundle)); } - out: +out: /* Restore flow */ memcpy(flow->vlans, flow_vlans, sizeof flow->vlans); flow->nw_tos = flow_nw_tos; @@ -4431,6 +4508,7 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, flow->dl_src = flow_dl_src; flow->packet_type = flow_packet_type; flow->dl_type = flow_dl_type; + free(flow_tnl); } static void @@ -5874,21 +5952,12 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, struct xlate_ctx *ctx, bool is_last_action, bool group_bucket_action OVS_UNUSED) { - struct ofpbuf old_stack = ctx->stack; - union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)]; - ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); - ofpbuf_put(&ctx->stack, old_stack.data, old_stack.size); - - struct ofpbuf old_action_set = ctx->action_set; - uint64_t actset_stub[1024 / 8]; - ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); - ofpbuf_put(&ctx->action_set, old_action_set.data, old_action_set.size); - + struct xretained_state *retained_state; size_t offset, ac_offset; - struct flow old_flow = ctx->xin->flow; + + retained_state = xretain_state_save(ctx); if (reversible_actions(actions, actions_len) || is_last_action) { - old_flow = ctx->xin->flow; do_xlate_actions(actions, actions_len, ctx, is_last_action, false); if (!ctx->freezing) { xlate_action_set(ctx); @@ -5903,7 +5972,7 @@ clone_xlate_actions(const struct ofpact *actions, size_t actions_len, * avoid emitting those actions twice. Once inside * the clone, another time for the action after clone. */ xlate_commit_actions(ctx); - struct flow old_base = ctx->base_flow; + xretain_base_save(ctx, retained_state); bool old_was_mpls = ctx->was_mpls; bool old_conntracked = ctx->conntracked; @@ -5960,14 +6029,10 @@ dp_clone_done: ctx->was_mpls = old_was_mpls; /* Restore the 'base_flow' for the next action. */ - ctx->base_flow = old_base; + xretain_base_restore(ctx, retained_state); xlate_done: - ofpbuf_uninit(&ctx->action_set); - ctx->action_set = old_action_set; - ofpbuf_uninit(&ctx->stack); - ctx->stack = old_stack; - ctx->xin->flow = old_flow; + xretain_state_restore(ctx, retained_state); } static void @@ -6471,19 +6536,12 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, return; } - struct ofpbuf old_stack = ctx->stack; - union mf_subvalue new_stack[1024 / sizeof(union mf_subvalue)]; - ofpbuf_use_stub(&ctx->stack, new_stack, sizeof new_stack); - ofpbuf_put(&ctx->stack, old_stack.data, old_stack.size); + struct xretained_state *retained_state; - struct ofpbuf old_action_set = ctx->action_set; - uint64_t actset_stub[1024 / 8]; - ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub); - ofpbuf_put(&ctx->action_set, old_action_set.data, old_action_set.size); + retained_state = xretain_state_save(ctx); - struct flow old_flow = ctx->xin->flow; xlate_commit_actions(ctx); - struct flow old_base = ctx->base_flow; + xretain_base_save(ctx, retained_state); bool old_was_mpls = ctx->was_mpls; bool old_conntracked = ctx->conntracked; @@ -6504,10 +6562,10 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, } nl_msg_end_nested(ctx->odp_actions, offset_attr); - ctx->base_flow = old_base; + xretain_base_restore(ctx, retained_state); + xretain_flow_restore(ctx, retained_state); ctx->was_mpls = old_was_mpls; ctx->conntracked = old_conntracked; - ctx->xin->flow = old_flow; /* If the flow translation for the IF_GREATER case requires freezing, * then ctx->exit would be true. Reset to false so that we can @@ -6530,15 +6588,11 @@ xlate_check_pkt_larger(struct xlate_ctx *ctx, nl_msg_end_nested(ctx->odp_actions, offset_attr); nl_msg_end_nested(ctx->odp_actions, offset); - ofpbuf_uninit(&ctx->action_set); - ctx->action_set = old_action_set; - ofpbuf_uninit(&ctx->stack); - ctx->stack = old_stack; - ctx->base_flow = old_base; ctx->was_mpls = old_was_mpls; ctx->conntracked = old_conntracked; - ctx->xin->flow = old_flow; ctx->exit = old_exit; + xretain_base_restore(ctx, retained_state); + xretain_state_restore(ctx, retained_state); } static void @@ -6989,6 +7043,24 @@ xlate_ofpact_unroll_xlate(struct xlate_ctx *ctx, "cookie=%#"PRIx64, a->rule_table_id, a->rule_cookie); } +static void +xlate_trace(struct xlate_ctx *ctx, const struct ofpact *a) { + struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); + + if (ctx->xin->names) { + struct ofproto_dpif *ofprotop; + ofprotop = ofproto_dpif_lookup_by_name(ctx->xbridge->name); + ofproto_append_ports_to_map(&map, ofprotop->up.ports); + } + + struct ds s = DS_EMPTY_INITIALIZER; + struct ofpact_format_params fp = { .s = &s, .port_map = &map }; + ofpacts_format(a, OFPACT_ALIGN(a->len), &fp); + xlate_report(ctx, OFT_ACTION, "%s", ds_cstr(&s)); + ds_destroy(&s); + ofputil_port_map_destroy(&map); +} + static void do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, struct xlate_ctx *ctx, bool is_last_action, @@ -7031,20 +7103,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, } if (OVS_UNLIKELY(ctx->xin->trace)) { - struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map); - - if (ctx->xin->names) { - struct ofproto_dpif *ofprotop; - ofprotop = ofproto_dpif_lookup_by_name(ctx->xbridge->name); - ofproto_append_ports_to_map(&map, ofprotop->up.ports); - } - - struct ds s = DS_EMPTY_INITIALIZER; - struct ofpact_format_params fp = { .s = &s, .port_map = &map }; - ofpacts_format(a, OFPACT_ALIGN(a->len), &fp); - xlate_report(ctx, OFT_ACTION, "%s", ds_cstr(&s)); - ds_destroy(&s); - ofputil_port_map_destroy(&map); + xlate_trace(ctx, a); } switch (a->type) {
Several xlate actions used in recursive translation currently store a large amount of information on the stack. This can result in handler threads quickly running out of stack space despite before xlate_resubmit_resource_check() is able to terminate translation. This patch reduces stack usage by over 3kb from several translation actions. This patch also moves some trace function from do_xlate_actions into its own function to reduce stack usage. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2104779 Signed-off-by: Mike Pattrick <mkp@redhat.com> --- Since v1: - Refactored code into specialized functions and renamed variables for improved readability. --- ofproto/ofproto-dpif-xlate.c | 223 ++++++++++++++++++++++------------- 1 file changed, 141 insertions(+), 82 deletions(-)