Thanks for catching this, I’m not sure if I was aware of resubmit to the
current table feature when I wrote that.
Acked-by: Jarno Rajahalme <ja...@ovn.org>
> On Jan 22, 2016, at 3:59 PM, Ben Pfaff <b...@ovn.org> wrote:
>
> When recirculation defers actions for processing later, it decides
> based on the actions being saved whether it needs to record the table
> and cookie from which they originated. Until now, it was thought that
> this was only important for actions that send packets to the controller
> (because those actions send the table ID and cookie). This overlooked
> a special case of the "resubmit" action which also depends on the
> current table ID, which meant that this special case malfunctioned if
> it came after recirculation. This commit fixes the problem, and adds
> a test.
>
> Found while testing another feature under development.
>
> Signed-off-by: Ben Pfaff <b...@ovn.org>
> ---
> ofproto/ofproto-dpif-xlate.c | 21 ++++++++++++++-------
> tests/ofproto-dpif.at | 25 +++++++++++++++++++++++++
> 2 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 6549191..4b25bf4 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4165,8 +4165,7 @@ recirc_put_unroll_xlate(struct xlate_ctx *ctx)
>
> /* Copy remaining actions to the action_set to be executed after
> recirculation.
> * UNROLL_XLATE action is inserted, if not already done so, before actions
> that
> - * may generate asynchronous messages from the current table and without
> - * matching another rule. */
> + * may depend on the current table ID or flow cookie. */
> static void
> recirc_unroll_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> struct xlate_ctx *ctx)
> @@ -4175,17 +4174,25 @@ recirc_unroll_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
>
> OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
> switch (a->type) {
> - /* May generate asynchronous messages. */
> case OFPACT_OUTPUT_REG:
> case OFPACT_GROUP:
> case OFPACT_OUTPUT:
> case OFPACT_CONTROLLER:
> case OFPACT_DEC_MPLS_TTL:
> case OFPACT_DEC_TTL:
> + /* These actions may generate asynchronous messages, which
> include
> + * table ID and flow cookie information. */
> recirc_put_unroll_xlate(ctx);
> break;
>
> - /* These may not generate PACKET INs. */
> + case OFPACT_RESUBMIT:
> + if (ofpact_get_RESUBMIT(a)->table_id == 0xff) {
> + /* This resubmit action is relative to the current table, so
> we
> + * need to track what table that is.*/
> + recirc_put_unroll_xlate(ctx);
> + }
> + break;
> +
> case OFPACT_SET_TUNNEL:
> case OFPACT_REG_MOVE:
> case OFPACT_SET_FIELD:
> @@ -4193,8 +4200,7 @@ recirc_unroll_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
> case OFPACT_STACK_POP:
> case OFPACT_LEARN:
> case OFPACT_WRITE_METADATA:
> - case OFPACT_RESUBMIT: /* May indirectly generate PACKET INs,
> */
> - case OFPACT_GOTO_TABLE: /* but from a different table and rule.
> */
> + case OFPACT_GOTO_TABLE:
> case OFPACT_ENQUEUE:
> case OFPACT_SET_VLAN_VID:
> case OFPACT_SET_VLAN_PCP:
> @@ -4228,11 +4234,12 @@ recirc_unroll_actions(const struct ofpact *ofpacts,
> size_t ofpacts_len,
> case OFPACT_DEBUG_RECIRC:
> case OFPACT_CT:
> case OFPACT_NAT:
> + /* These may not generate PACKET INs. */
> break;
>
> - /* These need not be copied for restoration. */
> case OFPACT_NOTE:
> case OFPACT_CONJUNCTION:
> + /* These need not be copied for restoration. */
> continue;
> }
> /* Copy the action over. */
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 3a8c893..bfb1b56 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -4178,6 +4178,31 @@ AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath
> actions: 4
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> +# This test verifies that the table ID is preserved across recirculation
> +# when a resubmit action requires it (because the action is relative to
> +# the current table rather than specifying a table).
> +AT_SETUP([ofproto-dpif - resubmit with recirculation])
> +OVS_VSWITCHD_START
> +ADD_OF_PORTS([br0], [1], [2], [3])
> +
> +AT_DATA([flows.txt], [dnl
> +table=0 in_port=1 actions=2,resubmit(,1)
> +table=1 in_port=1 actions=debug_recirc,resubmit:55
> +table=1 in_port=55 actions=3
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +flow="in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)"
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow" -generate], [0],
> [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 2,recirc(0x1)
> +])
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy "$flow,recirc_id(1)"
> -generate], [0], [stdout])
> +AT_CHECK_UNQUOTED([tail -1 stdout], [0], [Datapath actions: 3
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> # Two testcases below are for the ofproto/trace command
> # The first one tests all correct syntax:
> # ofproto/trace [dp_name] odp_flow [-generate|packet]
> --
> 2.1.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
