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

2017-12-06 Thread Zoltán Balogh 
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:b...@ovn.org]
> Sent: Monday, November 13, 2017 8:02 PM
> To: Zoltán Balogh <zoltan.bal...@ericsson.com>
> Cc: Chandran, Sugesh <sugesh.chand...@intel.com>; d...@openvswitch.org; Jan 
> Scheurich <jan.scheur...@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 +, 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(>start, >start, FLOW_PART_LEN(start, end))
> >
> > #define COPY_FLOW_PART(dst, src, start, end) \
> > memcpy(>start, >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);
> > }
> > }
> >
&g

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

2017-12-06 Thread Zoltán Balogh 
Subject is incorrect, missing v2. Please use this instead:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-December/341658.html


> -Original Message-
> From: Zoltán Balogh
> Sent: Wednesday, December 06, 2017 11:55 AM
> To: d...@openvswitch.org
> Cc: Zoltán Balogh ; Sugesh Chandran 
> ; Ben Pfaff 
> Subject: [PATCH 2/2] xlate: normalize the actions after translation
> 
> 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 
> Signed-off-by: Sugesh Chandran 
> Co-authored-by: Sugesh Chandran 
> CC: Ben Pfaff 
> ---
>  ofproto/ofproto-dpif-xlate.c | 67 
> 
>  tests/ofproto-dpif.at| 48 +++
>  2 files changed, 115 insertions(+)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 36d0a0e1f..2af1ec1e8 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -6954,6 +6954,70 @@ 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(const struct nlattr *nla)
> +{
> +enum ovs_action_attr action_type = nl_attr_type(nla);
> +
> +switch (action_type) {
> +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;
> +case OVS_ACTION_ATTR_UNSPEC:
> +case OVS_ACTION_ATTR_SET:
> +case OVS_ACTION_ATTR_PUSH_VLAN:
> +case OVS_ACTION_ATTR_POP_VLAN:
> +case OVS_ACTION_ATTR_HASH:
> +case OVS_ACTION_ATTR_PUSH_MPLS:
> +case OVS_ACTION_ATTR_POP_MPLS:
> +case OVS_ACTION_ATTR_SET_MASKED:
> +case OVS_ACTION_ATTR_PUSH_ETH:
> +case OVS_ACTION_ATTR_POP_ETH:
> +case OVS_ACTION_ATTR_METER:
> +case OVS_ACTION_ATTR_ENCAP_NSH:
> +case OVS_ACTION_ATTR_DECAP_NSH:
> +case __OVS_ACTION_ATTR_MAX:
> +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(const struct nlattr *data, const size_t data_len)
> +{
> +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)) {
> +last = nl_attr_next(a);
> +}
> +}
> +
> +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 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
>   * 'xout'.
>   * The caller must take responsibility for eventually freeing 'xout', with
> @@ -7364,7 +7428,10 @@ exit:
>  if (xin->odp_actions) {
>  ofpbuf_clear(xin->odp_actions);
>  }
> +} else {
> +normalize_odp_actions();
>  }
> +
>  return ctx.error;
>  }
> 
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index e7df1504a..5bcd8bb46 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-dev] [PATCH 2/2] xlate: normalize the actions after translation

2017-12-06 Thread Zoltan Balogh 
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 
Signed-off-by: Sugesh Chandran 
Co-authored-by: Sugesh Chandran 
CC: Ben Pfaff 
---
 ofproto/ofproto-dpif-xlate.c | 67 
 tests/ofproto-dpif.at| 48 +++
 2 files changed, 115 insertions(+)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 36d0a0e1f..2af1ec1e8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6954,6 +6954,70 @@ 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(const struct nlattr *nla)
+{
+enum ovs_action_attr action_type = nl_attr_type(nla);
+
+switch (action_type) {
+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;
+case OVS_ACTION_ATTR_UNSPEC:
+case OVS_ACTION_ATTR_SET:
+case OVS_ACTION_ATTR_PUSH_VLAN:
+case OVS_ACTION_ATTR_POP_VLAN:
+case OVS_ACTION_ATTR_HASH:
+case OVS_ACTION_ATTR_PUSH_MPLS:
+case OVS_ACTION_ATTR_POP_MPLS:
+case OVS_ACTION_ATTR_SET_MASKED:
+case OVS_ACTION_ATTR_PUSH_ETH:
+case OVS_ACTION_ATTR_POP_ETH:
+case OVS_ACTION_ATTR_METER:
+case OVS_ACTION_ATTR_ENCAP_NSH:
+case OVS_ACTION_ATTR_DECAP_NSH:
+case __OVS_ACTION_ATTR_MAX:
+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(const struct nlattr *data, const size_t data_len)
+{
+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)) {
+last = nl_attr_next(a);
+}
+}
+
+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 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
  * 'xout'.
  * The caller must take responsibility for eventually freeing 'xout', with
@@ -7364,7 +7428,10 @@ exit:
 if (xin->odp_actions) {
 ofpbuf_clear(xin->odp_actions);
 }
+} else {
+normalize_odp_actions();
 }
+
 return ctx.error;
 }
 
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index e7df1504a..5bcd8bb46 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
-- 
2.14.1

___
dev mailing list
d...@openvswitch.org

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

2017-11-13 Thread Ben Pfaff 
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 +, 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(>start, >start, FLOW_PART_LEN(start, end))
> 
> #define COPY_FLOW_PART(dst, src, start, end) \
> memcpy(>start, >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(>wc_masks, >wc->masks);
> copy_altered_flow_data(>flow, >xin->flow);
> copy_altered_flow_data(>base_flow, >base_flow);
> }
> }
> 
> What do you think?
> 
> Best regards,
> Zoltan
> 
> 
> > -Original Message-
> > From: Ben Pfaff [mailto:b...@ovn.org]
> > Sent: Tuesday, October 31, 2017 9:30 PM
> > To: Zoltán Balogh <zoltan.bal...@ericsson.com>
> > Cc: d...@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.bal...@ericsson.com>
> > > Signed-off-by: Sugesh Chandran <sugesh.chand...@intel.com>
> > > Co-authored-by: Sugesh Chandran <sugesh.chand...@intel.com>
> > > Tested-by: Sugesh Chandran <sugesh.chand...@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,
>

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

2017-11-13 Thread Zoltán Balogh 
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(>start, >start, FLOW_PART_LEN(start, end))

#define COPY_FLOW_PART(dst, src, start, end) \
memcpy(>start, >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(>wc_masks, >wc->masks);
copy_altered_flow_data(>flow, >xin->flow);
copy_altered_flow_data(>base_flow, >base_flow);
}
}

What do you think?

Best regards,
Zoltan


> -Original Message-
> From: Ben Pfaff [mailto:b...@ovn.org]
> Sent: Tuesday, October 31, 2017 9:30 PM
> To: Zoltán Balogh <zoltan.bal...@ericsson.com>
> Cc: d...@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.bal...@ericsson.com>
> > Signed-off-by: Sugesh Chandran <sugesh.chand...@intel.com>
> > Co-authored-by: Sugesh Chandran <sugesh.chand...@intel.com>
> > Tested-by: Sugesh Chandran <sugesh.chand...@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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2017-10-31 Thread Ben Pfaff 
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 
> Signed-off-by: Sugesh Chandran 
> Co-authored-by: Sugesh Chandran 
> Tested-by: Sugesh Chandran 

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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2017-10-30 Thread Zoltan Balogh 
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 
Signed-off-by: Sugesh Chandran 
Co-authored-by: Sugesh Chandran 
Tested-by: Sugesh Chandran 
---
 ofproto/ofproto-dpif-xlate.c | 59 
 tests/ofproto-dpif.at| 48 +++
 2 files changed, 107 insertions(+)

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();
 }
+
 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
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev