diff mbox series

[ovs-dev,2/2] xlate: normalize the actions after translation

Message ID 1509362570-20757-3-git-send-email-zoltan.balogh@ericsson.com
State Changes Requested
Headers show
Series xlate: optimize dp flow action in case of error in multi-bridge setup | expand

Commit Message

Zoltan Balogh Oct. 30, 2017, 11:22 a.m. UTC 
When all OF actions have been translated, there could be actions at
the end of list of odp actions which are not needed to be executed.
So, the list can be normalized at the end of xlate_actions().

Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
Co-authored-by: Sugesh Chandran <sugesh.chandran@intel.com>
Tested-by: Sugesh Chandran <sugesh.chandran@intel.com>
---
 ofproto/ofproto-dpif-xlate.c | 59 ++++++++++++++++++++++++++++++++++++++++++++
 tests/ofproto-dpif.at        | 48 +++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+)

Comments

Ben Pfaff Oct. 31, 2017, 8:29 p.m. UTC | #1 
On Mon, Oct 30, 2017 at 12:22:50PM +0100, Zoltan Balogh wrote:
> When all OF actions have been translated, there could be actions at
> the end of list of odp actions which are not needed to be executed.
> So, the list can be normalized at the end of xlate_actions().
> 
> Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
> Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
> Co-authored-by: Sugesh Chandran <sugesh.chandran@intel.com>
> Tested-by: Sugesh Chandran <sugesh.chandran@intel.com>

Thanks for working on this.  I wasn't previously aware that there was a
problem, but now I see what you mean.

The description in 0/2 is necessary to properly understand the problem,
but that description will get lost when the patches are committed.  I
recommend adding all or most of it to patch 1.

This approach to saving and restoring is going to be super-expensive
during translation.  On i386, struct xlate_backup_data is 1996 bytes,
and as I read the patch, that's getting copied every time we visit a new
OpenFlow table.  Do you have any idea about how to make it cheaper, or
how to make fewer backups?

Thanks,

Ben.
Ben Pfaff Oct. 31, 2017, 8:42 p.m. UTC | #2 
On Mon, Oct 30, 2017 at 12:22:50PM +0100, Zoltan Balogh wrote:
> When all OF actions have been translated, there could be actions at
> the end of list of odp actions which are not needed to be executed.
> So, the list can be normalized at the end of xlate_actions().
> 
> Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
> Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
> Co-authored-by: Sugesh Chandran <sugesh.chandran@intel.com>
> Tested-by: Sugesh Chandran <sugesh.chandran@intel.com>

Thanks for working on this!  It will be helpful in some cases.

In is_valid_last_action(), I recommend assigning nl_attr_type(nla) to a
variable of type enum ovs_action_attr, then using that for the switch
statement.  That will ensure that, as new kinds of actions are added, we
remember to add new items to the switch statement.

Here are some minor simplifications that you might consider folding in
also.  They compile, but I have not tested them.

--8<--------------------------cut here-------------------------->8--

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 5e400e091ac6..d08cfddc55cb 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6943,7 +6943,7 @@ xlate_wc_finish(struct xlate_ctx *ctx)
 /* Returns true if the action stored in 'nla' can be a valid last action of a
  * datapath flow. */
 static bool
-is_valid_last_action(struct nlattr *nla)
+is_valid_last_action(const struct nlattr *nla)
 {
     switch (nl_attr_type(nla)) {
     case OVS_ACTION_ATTR_USERSPACE:
@@ -6965,35 +6965,27 @@ is_valid_last_action(struct nlattr *nla)
  * 'data'. Execution of actions beyond this last attribute does not make sense.
  */
 static size_t
-last_action_offset(struct nlattr *data, const size_t data_len)
+last_action_offset(const struct nlattr *data, const size_t data_len)
 {
-    uint16_t left;
-    struct nlattr *a, *b = NULL;
+    const struct nlattr *last = data;
 
+    uint16_t left;
+    const struct nlattr *a;
     NL_ATTR_FOR_EACH (a, left, data, data_len) {
         if (is_valid_last_action(a)) {
-            b = a;
+            last = nl_attr_next(a);
         }
     }
 
-    if (b) {
-        return NLA_ALIGN(((char *)b - (char *)data) + b->nla_len);
-    } else {
-        return 0;
-    }
+    return (char *) last - (char *) data;
 }
 
+/* Get rid of any unneeded actions at the tail end. */
 static void
 normalize_odp_actions(struct xlate_ctx *ctx)
 {
-    struct nlattr *data = ctx->odp_actions->data;
-    size_t size = ctx->odp_actions->size;
-    size_t new_size = last_action_offset(data, size);
-
-    /* Get rid of any unneeded actions at the tail end. */
-    if (OVS_UNLIKELY(new_size != size)) {
-        ctx->odp_actions->size = new_size;
-    }
+    struct ofpbuf *oa = ctx->odp_actions;
+    oa->size = last_action_offset(oa->data, oa->size);
 }
 
 /* Translates the flow, actions, or rule in 'xin' into datapath actions in
Zoltan Balogh Nov. 13, 2017, 5:28 p.m. UTC | #3 
Hello Ben,

Thank you for the review. I have been thinking about possible solutions.

1) We could use non-temporary (non-cached) copy when backing up data. That would avoid using write cache and make writing memory faster. In turn reading data back right after the write would be slower but that should not be the regular case. Furthermore it should not be available for all hardware platforms.

2) Sugesh came up with the idea to do conditional backup and restore of data. We should keep track if xlate flow, wildcard and base_flow are modified in translation and only copy needed fields. I think this would be the best choice but has large code impact and would make code maintenance more complicated.

3) Another option would be to spilt up the flow structure into partitions. Then instead of copying the whole structure, partitions could be checked for change by using memcmp(), then do the copy in case of change. We could use something like this:

#define FLOW_PART_LEN(start, end) \
    ((char *)&((struct flow *)0)->end) - ((char *)&((struct flow *)0)->start)

#define COMPARE_FLOW_PART(dst, src, start, end) \
    memcmp(&dst->start, &src->start, FLOW_PART_LEN(start, end))

#define COPY_FLOW_PART(dst, src, start, end) \
    memcpy(&dst->start, &src->start, FLOW_PART_LEN(start, end))

static void
copy_altered_flow_data(struct flow *dst, const struct flow *src)
{
    BUILD_ASSERT_DECL(FLOW_WC_SEQ == 40);

    /* Metadata 1 */
    if (!COMPARE_FLOW_PART(dst, src, tunnel, metadata)) {
        COPY_FLOW_PART(dst, src, tunnel, metadata);
    }

    /* Metadata 2 */
    if (!COMPARE_FLOW_PART(dst, src, metadata, dl_dst)) {
        COPY_FLOW_PART(dst, src, metadata, dl_dst);
    }

    /* L2 */
    if (!COMPARE_FLOW_PART(dst, src, dl_dst, nw_src)) {
        COPY_FLOW_PART(dst, src, dl_dst, nw_src);
    }

    /* L3 */
    if (!COMPARE_FLOW_PART(dst, src, nw_src, tp_src)) {
        COPY_FLOW_PART(dst, src, nw_src, tp_src);
    }

    /* L4 */
    if (!COMPARE_FLOW_PART(dst, src, tp_src, pad3)) {
        COPY_FLOW_PART(dst, src, tp_src, pad3);
    }
}

static void
store_ctx_xlate_data(struct xlate_backup_data *data,
                     const struct xlate_ctx *ctx, bool force)
{
    data->odp_actions_size = ctx->odp_actions->size;
    if (force) {
        data->wc_masks = ctx->wc->masks;
        data->flow = ctx->xin->flow;
        data->base_flow = ctx->base_flow;
    } else {
        copy_altered_flow_data(&data->wc_masks, &ctx->wc->masks);
        copy_altered_flow_data(&data->flow, &ctx->xin->flow);
        copy_altered_flow_data(&data->base_flow, &ctx->base_flow);
    }
}

What do you think?

Best regards,
Zoltan


> -----Original Message-----
> From: Ben Pfaff [mailto:blp@ovn.org]
> Sent: Tuesday, October 31, 2017 9:30 PM
> To: Zoltán Balogh <zoltan.balogh@ericsson.com>
> Cc: dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH 2/2] xlate: normalize the actions after translation
> 
> On Mon, Oct 30, 2017 at 12:22:50PM +0100, Zoltan Balogh wrote:
> > When all OF actions have been translated, there could be actions at
> > the end of list of odp actions which are not needed to be executed.
> > So, the list can be normalized at the end of xlate_actions().
> >
> > Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
> > Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
> > Co-authored-by: Sugesh Chandran <sugesh.chandran@intel.com>
> > Tested-by: Sugesh Chandran <sugesh.chandran@intel.com>
> 
> Thanks for working on this.  I wasn't previously aware that there was a
> problem, but now I see what you mean.
> 
> The description in 0/2 is necessary to properly understand the problem,
> but that description will get lost when the patches are committed.  I
> recommend adding all or most of it to patch 1.
> 
> This approach to saving and restoring is going to be super-expensive
> during translation.  On i386, struct xlate_backup_data is 1996 bytes,
> and as I read the patch, that's getting copied every time we visit a new
> OpenFlow table.  Do you have any idea about how to make it cheaper, or
> how to make fewer backups?
> 
> Thanks,
> 
> Ben.
Ben Pfaff Nov. 13, 2017, 7:02 p.m. UTC | #4 
Here is an idea that just occurred to me.  What if we used a kind of
copy-on-write approach?  That is, whenever the flow is modified, we
first check whether we need to do a "backup" of it?  This should reduce
the number of copies greatly, because flow modifications are relatively
rare compared to, say, "resubmit" actions.  It will require a little
work to make the code in ofproto-dpif-xlate call the right function to
do the copying, but I think that it would be easier to implement and
maintain than the approaches you suggest (assuming that I understand
them correctly).

On Mon, Nov 13, 2017 at 05:28:24PM +0000, Zoltán Balogh wrote:
> Hello Ben,
> 
> Thank you for the review. I have been thinking about possible solutions.
> 
> 1) We could use non-temporary (non-cached) copy when backing up data. That would avoid using write cache and make writing memory faster. In turn reading data back right after the write would be slower but that should not be the regular case. Furthermore it should not be available for all hardware platforms.
> 
> 2) Sugesh came up with the idea to do conditional backup and restore of data. We should keep track if xlate flow, wildcard and base_flow are modified in translation and only copy needed fields. I think this would be the best choice but has large code impact and would make code maintenance more complicated.
> 
> 3) Another option would be to spilt up the flow structure into partitions. Then instead of copying the whole structure, partitions could be checked for change by using memcmp(), then do the copy in case of change. We could use something like this:
> 
> #define FLOW_PART_LEN(start, end) \
>     ((char *)&((struct flow *)0)->end) - ((char *)&((struct flow *)0)->start)
> 
> #define COMPARE_FLOW_PART(dst, src, start, end) \
>     memcmp(&dst->start, &src->start, FLOW_PART_LEN(start, end))
> 
> #define COPY_FLOW_PART(dst, src, start, end) \
>     memcpy(&dst->start, &src->start, FLOW_PART_LEN(start, end))
> 
> static void
> copy_altered_flow_data(struct flow *dst, const struct flow *src)
> {
>     BUILD_ASSERT_DECL(FLOW_WC_SEQ == 40);
> 
>     /* Metadata 1 */
>     if (!COMPARE_FLOW_PART(dst, src, tunnel, metadata)) {
>         COPY_FLOW_PART(dst, src, tunnel, metadata);
>     }
> 
>     /* Metadata 2 */
>     if (!COMPARE_FLOW_PART(dst, src, metadata, dl_dst)) {
>         COPY_FLOW_PART(dst, src, metadata, dl_dst);
>     }
> 
>     /* L2 */
>     if (!COMPARE_FLOW_PART(dst, src, dl_dst, nw_src)) {
>         COPY_FLOW_PART(dst, src, dl_dst, nw_src);
>     }
> 
>     /* L3 */
>     if (!COMPARE_FLOW_PART(dst, src, nw_src, tp_src)) {
>         COPY_FLOW_PART(dst, src, nw_src, tp_src);
>     }
> 
>     /* L4 */
>     if (!COMPARE_FLOW_PART(dst, src, tp_src, pad3)) {
>         COPY_FLOW_PART(dst, src, tp_src, pad3);
>     }
> }
> 
> static void
> store_ctx_xlate_data(struct xlate_backup_data *data,
>                      const struct xlate_ctx *ctx, bool force)
> {
>     data->odp_actions_size = ctx->odp_actions->size;
>     if (force) {
>         data->wc_masks = ctx->wc->masks;
>         data->flow = ctx->xin->flow;
>         data->base_flow = ctx->base_flow;
>     } else {
>         copy_altered_flow_data(&data->wc_masks, &ctx->wc->masks);
>         copy_altered_flow_data(&data->flow, &ctx->xin->flow);
>         copy_altered_flow_data(&data->base_flow, &ctx->base_flow);
>     }
> }
> 
> What do you think?
> 
> Best regards,
> Zoltan
> 
> 
> > -----Original Message-----
> > From: Ben Pfaff [mailto:blp@ovn.org]
> > Sent: Tuesday, October 31, 2017 9:30 PM
> > To: Zoltán Balogh <zoltan.balogh@ericsson.com>
> > Cc: dev@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH 2/2] xlate: normalize the actions after translation
> > 
> > On Mon, Oct 30, 2017 at 12:22:50PM +0100, Zoltan Balogh wrote:
> > > When all OF actions have been translated, there could be actions at
> > > the end of list of odp actions which are not needed to be executed.
> > > So, the list can be normalized at the end of xlate_actions().
> > >
> > > Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
> > > Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
> > > Co-authored-by: Sugesh Chandran <sugesh.chandran@intel.com>
> > > Tested-by: Sugesh Chandran <sugesh.chandran@intel.com>
> > 
> > Thanks for working on this.  I wasn't previously aware that there was a
> > problem, but now I see what you mean.
> > 
> > The description in 0/2 is necessary to properly understand the problem,
> > but that description will get lost when the patches are committed.  I
> > recommend adding all or most of it to patch 1.
> > 
> > This approach to saving and restoring is going to be super-expensive
> > during translation.  On i386, struct xlate_backup_data is 1996 bytes,
> > and as I read the patch, that's getting copied every time we visit a new
> > OpenFlow table.  Do you have any idea about how to make it cheaper, or
> > how to make fewer backups?
> > 
> > Thanks,
> > 
> > Ben.
Zoltan Balogh Dec. 6, 2017, 11:04 a.m. UTC | #5 
Hello Ben,

I added 'const' to the ctx members to be copied during backup in order to see
how big the code impact would be in case of using copy-on-write like method
as you proposed when we were talking about this problem in a conference break.
Actually there were more than 80 places indicated by the compiler in the code.
These were mostly in function headers, so I would say several hundreds of code
lines would be involved.

Instead of this approach I reduced the size of backup data structure by two
times the size of 'struct flow' and used bitwise operations instead of memcpy
for storing backup data. I moved backup data into 'struct ctx', its
initialization is performed when 'ctx' is created in xlate_actions().

I sent this second version of the series to the mailing list. Could you review
it, please? Do you think, copy-on-write is still needed?

https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341655.html
https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341657.html
https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341658.html

https://patchwork.ozlabs.org/patch/845118/
https://patchwork.ozlabs.org/patch/845127/

Best regards,
Zoltan

> -----Original Message-----
> From: Ben Pfaff [mailto:blp@ovn.org]
> Sent: Monday, November 13, 2017 8:02 PM
> To: Zoltán Balogh <zoltan.balogh@ericsson.com>
> Cc: Chandran, Sugesh <sugesh.chandran@intel.com>; dev@openvswitch.org; Jan Scheurich <jan.scheurich@ericsson.com>
> Subject: Re: [ovs-dev] [PATCH 2/2] xlate: normalize the actions after translation
> 
> Here is an idea that just occurred to me.  What if we used a kind of
> copy-on-write approach?  That is, whenever the flow is modified, we
> first check whether we need to do a "backup" of it?  This should reduce
> the number of copies greatly, because flow modifications are relatively
> rare compared to, say, "resubmit" actions.  It will require a little
> work to make the code in ofproto-dpif-xlate call the right function to
> do the copying, but I think that it would be easier to implement and
> maintain than the approaches you suggest (assuming that I understand
> them correctly).
> 
> On Mon, Nov 13, 2017 at 05:28:24PM +0000, Zoltán Balogh wrote:
> > Hello Ben,
> >
> > Thank you for the review. I have been thinking about possible solutions.
> >
> > 1) We could use non-temporary (non-cached) copy when backing up data. That would avoid using write cache and make
> writing memory faster. In turn reading data back right after the write would be slower but that should not be the
> regular case. Furthermore it should not be available for all hardware platforms.
> >
> > 2) Sugesh came up with the idea to do conditional backup and restore of data. We should keep track if xlate flow,
> wildcard and base_flow are modified in translation and only copy needed fields. I think this would be the best
> choice but has large code impact and would make code maintenance more complicated.
> >
> > 3) Another option would be to spilt up the flow structure into partitions. Then instead of copying the whole
> structure, partitions could be checked for change by using memcmp(), then do the copy in case of change. We could
> use something like this:
> >
> > #define FLOW_PART_LEN(start, end) \
> >     ((char *)&((struct flow *)0)->end) - ((char *)&((struct flow *)0)->start)
> >
> > #define COMPARE_FLOW_PART(dst, src, start, end) \
> >     memcmp(&dst->start, &src->start, FLOW_PART_LEN(start, end))
> >
> > #define COPY_FLOW_PART(dst, src, start, end) \
> >     memcpy(&dst->start, &src->start, FLOW_PART_LEN(start, end))
> >
> > static void
> > copy_altered_flow_data(struct flow *dst, const struct flow *src)
> > {
> >     BUILD_ASSERT_DECL(FLOW_WC_SEQ == 40);
> >
> >     /* Metadata 1 */
> >     if (!COMPARE_FLOW_PART(dst, src, tunnel, metadata)) {
> >         COPY_FLOW_PART(dst, src, tunnel, metadata);
> >     }
> >
> >     /* Metadata 2 */
> >     if (!COMPARE_FLOW_PART(dst, src, metadata, dl_dst)) {
> >         COPY_FLOW_PART(dst, src, metadata, dl_dst);
> >     }
> >
> >     /* L2 */
> >     if (!COMPARE_FLOW_PART(dst, src, dl_dst, nw_src)) {
> >         COPY_FLOW_PART(dst, src, dl_dst, nw_src);
> >     }
> >
> >     /* L3 */
> >     if (!COMPARE_FLOW_PART(dst, src, nw_src, tp_src)) {
> >         COPY_FLOW_PART(dst, src, nw_src, tp_src);
> >     }
> >
> >     /* L4 */
> >     if (!COMPARE_FLOW_PART(dst, src, tp_src, pad3)) {
> >         COPY_FLOW_PART(dst, src, tp_src, pad3);
> >     }
> > }
> >
> > static void
> > store_ctx_xlate_data(struct xlate_backup_data *data,
> >                      const struct xlate_ctx *ctx, bool force)
> > {
> >     data->odp_actions_size = ctx->odp_actions->size;
> >     if (force) {
> >         data->wc_masks = ctx->wc->masks;
> >         data->flow = ctx->xin->flow;
> >         data->base_flow = ctx->base_flow;
> >     } else {
> >         copy_altered_flow_data(&data->wc_masks, &ctx->wc->masks);
> >         copy_altered_flow_data(&data->flow, &ctx->xin->flow);
> >         copy_altered_flow_data(&data->base_flow, &ctx->base_flow);
> >     }
> > }
> >
> > What do you think?
> >
> > Best regards,
> > Zoltan
> >
> >
> > > -----Original Message-----
> > > From: Ben Pfaff [mailto:blp@ovn.org]
> > > Sent: Tuesday, October 31, 2017 9:30 PM
> > > To: Zoltán Balogh <zoltan.balogh@ericsson.com>
> > > Cc: dev@openvswitch.org
> > > Subject: Re: [ovs-dev] [PATCH 2/2] xlate: normalize the actions after translation
> > >
> > > On Mon, Oct 30, 2017 at 12:22:50PM +0100, Zoltan Balogh wrote:
> > > > When all OF actions have been translated, there could be actions at
> > > > the end of list of odp actions which are not needed to be executed.
> > > > So, the list can be normalized at the end of xlate_actions().
> > > >
> > > > Signed-off-by: Zoltan Balogh <zoltan.balogh@ericsson.com>
> > > > Signed-off-by: Sugesh Chandran <sugesh.chandran@intel.com>
> > > > Co-authored-by: Sugesh Chandran <sugesh.chandran@intel.com>
> > > > Tested-by: Sugesh Chandran <sugesh.chandran@intel.com>
> > >
> > > Thanks for working on this.  I wasn't previously aware that there was a
> > > problem, but now I see what you mean.
> > >
> > > The description in 0/2 is necessary to properly understand the problem,
> > > but that description will get lost when the patches are committed.  I
> > > recommend adding all or most of it to patch 1.
> > >
> > > This approach to saving and restoring is going to be super-expensive
> > > during translation.  On i386, struct xlate_backup_data is 1996 bytes,
> > > and as I read the patch, that's getting copied every time we visit a new
> > > OpenFlow table.  Do you have any idea about how to make it cheaper, or
> > > how to make fewer backups?
> > >
> > > Thanks,
> > >
> > > Ben.
diff mbox series

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0cc59e7..5f3e5f8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6938,6 +6938,62 @@  xlate_wc_finish(struct xlate_ctx *ctx)
     }
 }
 
+/* Returns true if the action stored in 'nla' can be a valid last action of a
+ * datapath flow. */
+static bool
+is_valid_last_action(struct nlattr *nla)
+{
+    switch (nl_attr_type(nla)) {
+    case OVS_ACTION_ATTR_USERSPACE:
+    case OVS_ACTION_ATTR_SAMPLE:
+    case OVS_ACTION_ATTR_TRUNC:
+    case OVS_ACTION_ATTR_RECIRC:
+    case OVS_ACTION_ATTR_TUNNEL_PUSH:
+    case OVS_ACTION_ATTR_TUNNEL_POP:
+    case OVS_ACTION_ATTR_OUTPUT:
+    case OVS_ACTION_ATTR_CLONE:
+    case OVS_ACTION_ATTR_CT:
+        return true;
+    default:
+        return false;
+    }
+}
+
+/* Returns offset of last netlink attribute storing valid action in array
+ * 'data'. Execution of actions beyond this last attribute does not make sense.
+ */
+static size_t
+last_action_offset(struct nlattr *data, const size_t data_len)
+{
+    uint16_t left;
+    struct nlattr *a, *b = NULL;
+
+    NL_ATTR_FOR_EACH (a, left, data, data_len) {
+        if (is_valid_last_action(a)) {
+            b = a;
+        }
+    }
+
+    if (b) {
+        return NLA_ALIGN(((char *)b - (char *)data) + b->nla_len);
+    } else {
+        return 0;
+    }
+}
+
+static void
+normalize_odp_actions(struct xlate_ctx *ctx)
+{
+    struct nlattr *data = ctx->odp_actions->data;
+    size_t size = ctx->odp_actions->size;
+    size_t new_size = last_action_offset(data, size);
+
+    /* Get rid of any unneeded actions at the tail end. */
+    if (OVS_UNLIKELY(new_size != size)) {
+        ctx->odp_actions->size = new_size;
+    }
+}
+
 /* Translates the flow, actions, or rule in 'xin' into datapath actions in
  * 'xout'.
  * The caller must take responsibility for eventually freeing 'xout', with
@@ -7343,7 +7399,10 @@  exit:
         if (xin->odp_actions) {
             ofpbuf_clear(xin->odp_actions);
         }
+    } else {
+        normalize_odp_actions(&ctx);
     }
+
     return ctx.error;
 }
 
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 1124f0e..d9f0432 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -10133,3 +10133,51 @@  AT_CHECK([grep "Datapath actions" stdout], [0],
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - xlate error - normalize actions])
+
+#      ->-+
+#         | p1
+#       +-o-------+
+#       |   br0   |
+#       +-o-----o-+
+#   patch0|     |patch1
+#         +-->--+
+
+OVS_VSWITCHD_START([dnl
+    -- add-port br0 patch0 \
+    -- set interface patch0 type=patch options:peer=patch1 ofport_request=10 \
+    -- add-port br0 patch1 \
+    -- set interface patch1 type=patch options:peer=patch0 ofport_request=20
+])
+
+AT_CHECK([
+    ovs-vsctl add-port br0 p1 -- set interface p1 type=dummy ofport_request=1
+])
+
+AT_CHECK([
+    ovs-appctl dpif/show | grep -v hit | sed "s/$(printf \\t)/    /g" | sed 's./[[0-9]]\{1,\}..'
+], [0], [dnl
+    br0:
+        br0 65534: (dummy-internal)
+        p1 1: (dummy)
+        patch0 10/none: (patch: peer=patch1)
+        patch1 20/none: (patch: peer=patch0)
+])
+
+# Error due to pushing too many MPLS labels.
+AT_CHECK([
+    ovs-ofctl del-flows br0
+    ovs-ofctl add-flow br0 "table=0,in_port=1,actions=mod_dl_src:aa:aa:aa:bb:bb:00,goto_table:1" -OOpenFlow13
+    ovs-ofctl add-flow br0 "table=0,in_port=patch1,actions=goto_table:2" -OOpenFlow13
+    ovs-ofctl add-flow br0 "table=1,actions=push_vlan:0x8100,set_field:4196->vlan_vid,output:patch0" -OOpenFlow13
+    ovs-ofctl add-flow br0 "table=2,actions=push_mpls:0x8847,output:patch0"
+], [0])
+
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,ip'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: drop
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP