On Thu, Jun 29, 2023 at 3:10 PM Ilya Maximets <[email protected]> wrote:
>
> On 6/9/23 17:05, 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.
>
> I'm assuming that this number is mostly about patch_port_output function,
> right? I see only ~30% stack usage reduction for the main recursive
> do_xlate_actions function. It went from 720 to 512 bytes.
>
> Or am I calculating it wrong? I'm using -fstack-size compiler option.
I was using GDB for this, finding the difference in the stack pointer
between calls to the same function during recursion. The intention of
this patch is to get to xlate_resubmit_resource_check() without
smashing the stack, no individual functions stack usage is as
important as the holistic stack usage while recursing.
For example, without patch:
(gdb) fram 1
#1 0x000000000044dee3 in xlate_table_action (...) at
ofproto/ofproto-dpif-xlate.c:4539
4539 if (xlate_resubmit_resource_check(ctx)) {
(gdb) p $rsp
$4 = (void *) 0x7f888e7aa700
(gdb) frame 8
#8 xlate_table_action (...) at ofproto/ofproto-dpif-xlate.c:4584
4584 xlate_recursively(ctx, rule, table_id <= old_table_id,
(gdb) p $rsp
$5 = (void *) 0x7f888e7aba70
(gdb) p 0x7f888e7aba70-0x7f888e7aa700
$6 = 4976
With patch:
#1 0x000000000044e3bc in xlate_table_action (...) at
ofproto/ofproto-dpif-xlate.c:4552
4552 if (xlate_resubmit_resource_check(ctx)) {
(gdb) p $rsp
$4 = (void *) 0x7f78e69a9860
(gdb) frame 9
#9 0x000000000044ec00 in xlate_table_action (...) at
ofproto/ofproto-dpif-xlate.c:7212
7212 xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
(gdb) p $rsp
$5 = (void *) 0x7f78e69a9c60
(gdb) p 0x7f78e69a9c60-0x7f78e69a9860
$6 = 1024
>
> > This patch also moves some trace function from do_xlate_actions into its
> > own function to reduce stack usage.
>
> This doesn't seem to work. With GCC and -O2, i.e. the standard optimization
> level, this new function is always inlined for me, and there is no real
> difference in the produced code and the stack usage.
When I compile with O2, I get the following code generation:
Without patch:
000000000044cda0 <do_xlate_actions>:
[...]
44cdad: 48 81 ec 78 02 00 00 sub rsp,0x278
With patch:
000000000044cff0 <do_xlate_actions>:
[...]
44cffd: 48 81 ec 98 01 00 00 sub rsp,0x198
Do you get different results?
>
> In order to achieve actual reduction of stack usage there we need to move
> to dynamic allocation of the port name map.q
>
> There are few more functions that are fully or partially inlined into
> do_xlate_actions with default compiler options:
>
> - xlate_output_reg_action: Allocates a large mf_subvalue on stack.
> - xlate_sample_action: Allocates huge struct flow_tnl(!) on stack.
> - compose_conntrack_action: Allocates mf_subvalue.
> - xlate_check_pkt_larger: Allocates mf_subvalue as well.
>
> With these moved to heap, it should be possible to reduce the stack usage
> by about 60% for do_xlate_actions function.
These don't inline for me on O2. What GCC version are you using?
Thanks,
M
>
> Some more comments inline.
>
> Best regards, Ilya Maximets.
>
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2104779
> > Signed-off-by: Mike Pattrick <[email protected]>
> > Reviewed-by: Simon Horman <[email protected]>
> >
> > ---
> > Since v1:
> > - Refactored code into specialized functions and renamed variables for
> > improved readability.
> > Since v2:
> > - Removed inline keywords
> > Since v3:
> > - Improved formatting.
> > Since v4:
> > - v4 patch was an incorrect revision
> > ---
> > ofproto/ofproto-dpif-xlate.c | 211 ++++++++++++++++++++++-------------
> > 1 file changed, 135 insertions(+), 76 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 29f4daa63..9e6d5138d 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -501,6 +501,82 @@ 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_mask;
> > +};
> > +
> > +/* The return of this function must be freed by xretain_state_restore(). */
> > +static 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 void
> > +xretain_tunnel_mask_save(struct xlate_ctx *ctx,
>
> const
>
> > + struct xretained_state *retained)
> > +{
> > + retained->flow_tnl_mask = ctx->wc->masks.tunnel;
> > +}
> > +
> > +static void
> > +xretain_base_flow_save(const struct xlate_ctx *ctx,
> > + struct xretained_state *retained)
> > +{
> > + retained->old_base = ctx->base_flow;
> > +}
> > +
> > +static void
> > +xretain_base_flow_restore(struct xlate_ctx *ctx,
> > + const struct xretained_state *retained)
> > +{
> > + ctx->base_flow = retained->old_base;
> > +}
> > +
> > +static void
> > +xretain_flow_restore(struct xlate_ctx *ctx,
> > + const struct xretained_state *retained)
> > +{
> > + ctx->xin->flow = retained->old_flow;
> > +}
> > +
> > +static void
> > +xretain_tunnel_mask_restore(struct xlate_ctx *ctx,
> > + const struct xretained_state *retained)
> > +{
> > + ctx->wc->masks.tunnel = retained->flow_tnl_mask;
> > +}
> > +
> > +static void
> > +xretain_state_restore(struct xlate_ctx *ctx, struct xretained_state
> > *retained)
>
> s/xretain_state_restore/xretain_state_restore_and_free/ ?
>
> > +{
> > + 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,20 +3991,17 @@ 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;
> > + struct xretained_state *retained_state;
> > 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(&ctx->stack, new_stack, sizeof new_stack);
> > - ofpbuf_use_stub(&ctx->action_set, actset_stub, sizeof actset_stub);
> > + retained_state = xretain_state_save(ctx);
> > +
> > + xretain_tunnel_mask_save(ctx, retained_state);
> > +
> > flow->in_port.ofp_port = out_dev->ofp_port;
> > flow->metadata = htonll(0);
> > memset(&flow->tunnel, 0, sizeof flow->tunnel);
> > @@ -3967,14 +4040,15 @@ 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_flow_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_flow_restore(ctx, retained_state);
> > ctx->odp_actions->size = old_size;
> >
> > /* Undo changes that may have been done for freezing. */
> > @@ -3986,18 +4060,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_mask_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 +4318,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 +4332,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 +4375,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;
>
> This should be an xmemdup instead.
>
> > 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 +4407,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 +4451,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);
>
> This should not be reachable.
> We could do an ovs_assert(flow_tnl) instead before restoration.
>
> > + }
> >
> > } else if (terminate_native_tunnel(ctx, xport, flow, wc,
> > &odp_tnl_port)) {
> > @@ -4423,7 +4498,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 +4506,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 +5950,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 +5970,8 @@ 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_flow_save(ctx, retained_state);
> > +
> > bool old_was_mpls = ctx->was_mpls;
> > bool old_conntracked = ctx->conntracked;
> >
> > @@ -5960,14 +6028,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_flow_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 +6535,13 @@ 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_flow_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_flow_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_flow_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) {
>
> '{' should be on a new line.
>
> > + struct ofputil_port_map map = OFPUTIL_PORT_MAP_INITIALIZER(&map);
> > +
> > + if (ctx->xin->names) {
> > + struct ofproto_dpif *ofprotop;
>
> An empty line here would be nice. We tried to same some space in
> do_xlate_actions, but it's not necessary here.
>
> > + 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 };
>
> An empty line here as well.
>
> > + 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) {
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
