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 @@
