Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Reduce stack usage in recursive xlate functions
On Fri, Mar 3, 2023 at 12:31 PM Adrian Moreno wrote: > > Hi Mike, > > I've gone though this patch and have some minor style comments and nits. I've > also played a bit with it and run it through valgrind and looks solid. > > On 12/8/22 17:22, 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 > > --- > > ofproto/ofproto-dpif-xlate.c | 168 +-- > > 1 file changed, 99 insertions(+), 69 deletions(-) > > > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index a9cf3cbee..8ed264d0b 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -411,6 +411,18 @@ struct xlate_ctx { > > enum xlate_error error; /* Translation failed. */ > > }; > > > > +/* This structure is used to save stack space in actions that need to > > + * retain a large amount of xlate_ctx state. */ > > +struct xsaved_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; > > +}; > > + > > Nit: not that I have a better alternative but the name of this struct is > sligthly confusing. The name suggets it's a part of xlate_ctx state so one > would > expect a simple function that creates this struct from an existing xlate_ctx > and > one that copies its content back. However, this has a mix of old and new data. > > > /* Structure to track VLAN manipulation */ > > struct xvlan_single { > > uint16_t tpid; > > @@ -3906,20 +3918,21 @@ 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 = >xin->flow; > > -struct flow old_flow = ctx->xin->flow; > > -struct flow_tnl old_flow_tnl_wc = ctx->wc->masks.tunnel; > > +struct xsaved_state * old_state = xmalloc(sizeof * old_state); > > Nits: > s/xsaved_state * old_state/xsaved_state *old_state/ > s/sizeof * old_state/sizeof *old_state/ > > > > +struct ovs_list *old_trace = ctx->xin->trace; > > bool old_conntrack = ctx->conntracked; > > +struct flow *flow = >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]; > > > > -ofpbuf_use_stub(>stack, new_stack, sizeof new_stack); > > -ofpbuf_use_stub(>action_set, actset_stub, sizeof actset_stub); > > +old_state->old_flow = ctx->xin->flow; > > +old_state->flow_tnl = ctx->wc->masks.tunnel; > > +ovs_version_t old_version = ctx->xin->tables_version; > > Any reason why we would not want to put this into xsaved_state as well? I had only moved very large types to xsaved_state. But I can see how the case could be made, for readability and simplicity, to move all these variables into it. > > > +old_state->old_stack = ctx->stack; > > +old_state->old_action_set = ctx->action_set; > > +ofpbuf_use_stub(>stack, old_state->new_stack, > > +sizeof old_state->new_stack); > > +ofpbuf_use_stub(>action_set, old_state->actset_stub, > > +sizeof old_state->actset_stub); > > I think something that would help with the naming nit above would be to, > instead > of using the xsaved_state to store the new stack's data plus the old stack's > ofpbuf, just use ofpbuf_clone to save the old stack entirely (the ofpbuf > header > plus its data). Same for actset_stub. > > That way, when we go down the recursion we would reuse the current ctx->stack > (which we might want to zero before depending on the case). > > It doesn't make a huge difference technically speaking but I think would make > the code cleaner because we would now be able to move the state-saving and > state-restoring to helper functions if we wanted, or just make this one easier > to read. Plus we would not need to prefix all of xsaved_state's members with > "old" or "new". > > Again, not that it really matters in terms of logic but I think it might yield > some simpler code. > What do you think? I see what
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Reduce stack usage in recursive xlate functions
Hi Mike, I've gone though this patch and have some minor style comments and nits. I've also played a bit with it and run it through valgrind and looks solid. On 12/8/22 17:22, 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 --- ofproto/ofproto-dpif-xlate.c | 168 +-- 1 file changed, 99 insertions(+), 69 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index a9cf3cbee..8ed264d0b 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -411,6 +411,18 @@ struct xlate_ctx { enum xlate_error error; /* Translation failed. */ }; +/* This structure is used to save stack space in actions that need to + * retain a large amount of xlate_ctx state. */ +struct xsaved_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; +}; + Nit: not that I have a better alternative but the name of this struct is sligthly confusing. The name suggets it's a part of xlate_ctx state so one would expect a simple function that creates this struct from an existing xlate_ctx and one that copies its content back. However, this has a mix of old and new data. /* Structure to track VLAN manipulation */ struct xvlan_single { uint16_t tpid; @@ -3906,20 +3918,21 @@ 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 = >xin->flow; -struct flow old_flow = ctx->xin->flow; -struct flow_tnl old_flow_tnl_wc = ctx->wc->masks.tunnel; +struct xsaved_state * old_state = xmalloc(sizeof * old_state); Nits: s/xsaved_state * old_state/xsaved_state *old_state/ s/sizeof * old_state/sizeof *old_state/ +struct ovs_list *old_trace = ctx->xin->trace; bool old_conntrack = ctx->conntracked; +struct flow *flow = >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]; -ofpbuf_use_stub(>stack, new_stack, sizeof new_stack); -ofpbuf_use_stub(>action_set, actset_stub, sizeof actset_stub); +old_state->old_flow = ctx->xin->flow; +old_state->flow_tnl = ctx->wc->masks.tunnel; +ovs_version_t old_version = ctx->xin->tables_version; Any reason why we would not want to put this into xsaved_state as well? +old_state->old_stack = ctx->stack; +old_state->old_action_set = ctx->action_set; +ofpbuf_use_stub(>stack, old_state->new_stack, +sizeof old_state->new_stack); +ofpbuf_use_stub(>action_set, old_state->actset_stub, +sizeof old_state->actset_stub); I think something that would help with the naming nit above would be to, instead of using the xsaved_state to store the new stack's data plus the old stack's ofpbuf, just use ofpbuf_clone to save the old stack entirely (the ofpbuf header plus its data). Same for actset_stub. That way, when we go down the recursion we would reuse the current ctx->stack (which we might want to zero before depending on the case). It doesn't make a huge difference technically speaking but I think would make the code cleaner because we would now be able to move the state-saving and state-restoring to helper functions if we wanted, or just make this one easier to read. Plus we would not need to prefix all of xsaved_state's members with "old" or "new". Again, not that it really matters in terms of logic but I think it might yield some simpler code. What do you think? flow->in_port.ofp_port = out_dev->ofp_port; flow->metadata = htonll(0); memset(>tunnel, 0, sizeof flow->tunnel); @@ -3958,14 +3971,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; +old_state->old_base
[ovs-dev] [PATCH] ofproto-dpif-xlate: Reduce stack usage in recursive xlate functions
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 --- ofproto/ofproto-dpif-xlate.c | 168 +-- 1 file changed, 99 insertions(+), 69 deletions(-) diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index a9cf3cbee..8ed264d0b 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -411,6 +411,18 @@ struct xlate_ctx { enum xlate_error error; /* Translation failed. */ }; +/* This structure is used to save stack space in actions that need to + * retain a large amount of xlate_ctx state. */ +struct xsaved_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; +}; + /* Structure to track VLAN manipulation */ struct xvlan_single { uint16_t tpid; @@ -3906,20 +3918,21 @@ 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 = >xin->flow; -struct flow old_flow = ctx->xin->flow; -struct flow_tnl old_flow_tnl_wc = ctx->wc->masks.tunnel; +struct xsaved_state * old_state = xmalloc(sizeof * old_state); +struct ovs_list *old_trace = ctx->xin->trace; bool old_conntrack = ctx->conntracked; +struct flow *flow = >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]; -ofpbuf_use_stub(>stack, new_stack, sizeof new_stack); -ofpbuf_use_stub(>action_set, actset_stub, sizeof actset_stub); +old_state->old_flow = ctx->xin->flow; +old_state->flow_tnl = ctx->wc->masks.tunnel; +ovs_version_t old_version = ctx->xin->tables_version; +old_state->old_stack = ctx->stack; +old_state->old_action_set = ctx->action_set; +ofpbuf_use_stub(>stack, old_state->new_stack, +sizeof old_state->new_stack); +ofpbuf_use_stub(>action_set, old_state->actset_stub, +sizeof old_state->actset_stub); flow->in_port.ofp_port = out_dev->ofp_port; flow->metadata = htonll(0); memset(>tunnel, 0, sizeof flow->tunnel); @@ -3958,14 +3971,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; +old_state->old_base = ctx->base_flow; 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; +ctx->base_flow = old_state->old_base; ctx->odp_actions->size = old_size; /* Undo changes that may have been done for freezing. */ @@ -3977,18 +3990,18 @@ 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->xin->flow = old_state->old_flow; ctx->xbridge = in_dev->xbridge; ofpbuf_uninit(>action_set); -ctx->action_set = old_action_set; +ctx->action_set = old_state->old_action_set; ofpbuf_uninit(>stack); -ctx->stack = old_stack; +ctx->stack = old_state->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; +ctx->wc->masks.tunnel = old_state->flow_tnl; /* The out bridge popping MPLS should have no effect on the original * bridge. */ @@ -4022,6 +4035,7 @@ patch_port_output(struct xlate_ctx *ctx, const struct xport *in_dev, entry->dev.rx = netdev_ref(out_dev->netdev); entry->dev.bfd = bfd_ref(out_dev->bfd); } +free(old_state); } static bool @@ -4238,7 +4252,7 @@