On 9 Jun 2023, at 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.
>
> 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 <m...@redhat.com>
> Reviewed-by: Simon Horman <simon.hor...@corigine.com>
Changes look good to me, one small nit below, but not related to your changes.
So if no more revs are needed, we can leave it as is.
Acked-by: Eelco Chaudron <echau...@redhat.com>
> ---
> 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,
> + 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)
> +{
> + 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];
nit: Maybe re-order the variable declaration in reversed Christmas tree order?
>
> - 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;
> 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);
> + }
>
> } 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) {
> + 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
