With the (few) comments below:
Acked-by: Jarno Rajahalme <ja...@ovn.org>
(snip)
> + * In the current implementation, Open vSwitch forks the packet processing
> + * pipeline across patch ports. Suppose, for example, that the pipeline for
> + * br0 outputs to a patch port whose peer belongs to br1, and that the
> pipeline
> + * for br1 executes a "pause" action. This "pause" only pauses processing
> + * within br1, and processing in br0 continues and possibly completes with
> + * visible side effects, such as outputting to ports, before br1's controller
> + * receives or processes the closure. This implementation maintains the
> + * independence of separate bridges and, since processing in br1 cannot
> affect
> + * the behavior of br0 anyway, should not cause visible behavioral changes.
> + */
>
(snip)
> --- a/lib/ofp-util.h
> +++ b/lib/ofp-util.h
> @@ -31,6 +31,7 @@
> #include "openflow/nicira-ext.h"
> #include "openvswitch/types.h"
> #include "type-props.h"
> +#include "uuid.h"
>
> struct ofpbuf;
> union ofp_action;
> @@ -459,6 +460,60 @@ const char *ofputil_packet_in_reason_to_string(enum
> ofp_packet_in_reason,
> bool ofputil_packet_in_reason_from_string(const char *,
> enum ofp_packet_in_reason *);
>
> +/* Abstract NXT_CLOSURE. */
> +struct ofputil_closure {
> + /* NXCPT_USERDATA. */
> + uint8_t *userdata;
> + size_t userdata_len;
> +
> + /* NXCPT_PACKET. */
> + void *packet;
Maybe comment that ‘packet’ will be freed with free(), so it must be a pointer
to malloced memory?
> + size_t packet_len;
> +
> + /* NXCPT_METADATA. */
> + struct match metadata;
> +};
> +void ofputil_closure_destroy(struct ofputil_closure *);
> +
> +enum ofperr ofputil_decode_closure(const struct ofp_header *,
> + struct ofputil_closure *,
> + struct ofpbuf *private_properties);
> +struct ofpbuf *ofputil_encode_resume(const struct ofputil_closure *,
> + const struct ofpbuf *private_properties,
> + enum ofputil_protocol);
> +
> +struct ofputil_closure_private {
> + struct ofputil_closure public;
> +
> + /* NXCPT_BRIDGE. */
> + struct uuid bridge;
> +
> + /* NXCPT_STACK. */
> + union mf_subvalue *stack;
> + size_t n_stack;
> +
> + /* NXCPT_MIRRORS. */
> + uint32_t mirrors;
> +
> + /* NXCPT_CONNTRACKED. */
> + bool conntracked;
> +
> + /* NXCPT_ACTIONS. */
> + struct ofpact *actions;
> + size_t actions_len;
> +
> + /* NXCPT_ACTION_SET. */
> + struct ofpact *action_set;
> + size_t action_set_len;
> +};
> +void ofputil_closure_private_destroy(struct ofputil_closure_private *);
> +
> +enum ofperr ofputil_decode_closure_private(const struct ofp_header *,
> + bool loose,
> + struct ofputil_closure_private *);
> +struct ofpbuf *ofputil_encode_closure_private(
> + const struct ofputil_closure_private *, enum ofputil_protocol);
> +
> /* Abstract packet-out message.
> *
> * ofputil_decode_packet_out() will ensure that 'in_port' is a physical port
> @@ -1310,8 +1365,9 @@ enum ofputil_async_msg_type {
> OAM_TABLE_STATUS, /* OFPT_TABLE_STATUS. */
> OAM_REQUESTFORWARD, /* OFPT_REQUESTFORWARD. */
>
> - /* Extension asynchronous messages (none yet--coming soon!). */
> -#define OAM_EXTENSIONS 0 /* Bitmap of all extensions. */
> + /* Extension asynchronous messages. */
> + OAM_CLOSURE, /* NXT_CLOSURE. */
> +#define OAM_EXTENSIONS (1u << OAM_CLOSURE) /* Bitmap of all extensions. */
>
> OAM_N_TYPES
> };
>
(snip)
> +enum ofperr
> +xlate_closure(struct ofproto_dpif *ofproto,
> + const struct ofputil_closure_private *closure,
> + struct ofpbuf *odp_actions,
> + enum slow_path_reason *slow)
> +{
> + struct dp_packet packet;
> + dp_packet_use_const(&packet, closure->public.packet,
> + closure->public.packet_len);
> +
> + struct flow flow;
> + flow_extract(&packet, &flow);
> +
> + struct xlate_in xin;
> + xlate_in_init(&xin, ofproto, &flow, 0, NULL, ntohs(flow.tcp_flags),
> + &packet, NULL, odp_actions);
> +
> + struct ofpact_note noop;
> + ofpact_init_NOTE(&noop);
> + noop.length = 0;
> +
> + bool any_actions = closure->actions_len > 0;
> + struct recirc_state recirc = {
> + .table_id = 0, /* Not the table where NXAST_PAUSE was executed.
> */
> + .ofproto_uuid = closure->bridge,
> + .stack = closure->stack,
> + .n_stack = closure->n_stack,
> + .mirrors = closure->mirrors,
> + .conntracked = closure->conntracked,
> +
> + /* When there are no actions, xlate_actions() will search the flow
> + * table. We don't want it to do that (we want it to resume), so
> + * supply a no-op action if there aren't any.
> + *
> + * (We can't necessarily avoid translating actions entirely if there
> + * aren't any actions, because there might be some finishing-up to do
> + * at the end of the pipeline, and we don't check for those
> + * conditions.) */
> + .ofpacts = any_actions ? closure->actions : &noop.ofpact,
> + .ofpacts_len = any_actions ? closure->actions_len : sizeof noop,
> +
> + .action_set = closure->action_set,
> + .action_set_len = closure->action_set_len,
> + };
> + recirc_metadata_from_flow(&recirc.metadata,
> + &closure->public.metadata.flow);
> + xin.recirc = &recirc;
Maybe this could be called ‘xin.frozen_state’ now? Also the 'struct
recirc_state' could be renamed as 'struct frozen_state’.
> +
> + struct xlate_out xout;
> + enum xlate_error error = xlate_actions(&xin, &xout);
> + *slow = xout.slow;
> + xlate_out_uninit(&xout);
> +
> + /* xlate_actions() can generate a number of errors, but only
> + * XLATE_BRIDGE_NOT_FOUND really stands out to me as one that we should
> be
> + * sure to report over OpenFlow. The others could come up in packet-outs
> + * or regular flow translation and I don't think that it's going to be
> too
> + * useful to report them to the controller. */
> + return error == XLATE_BRIDGE_NOT_FOUND ? OFPERR_NXR_STALE : 0;
> +}
> +
> /* Sends 'packet' out 'ofport'.
> * May modify 'packet'.
> * Returns 0 if successful, otherwise a positive errno value. */
>
(snip)
> +# Check that pause at the end of the pipeline works OK.
> +#
> +# (xlate_closure() has a special case for no-op actions; this fails without
> +# that special case.)
> +CHECK_CLOSURE([pause at end of pipeline], [2], [0], [actions=2 pause])
> +
> +# Check that remaining actions are preserved following resume.
> +CHECK_CLOSURE([actions], [7], [0],
> + [in_port=1 actions=pause 2 pause 3 pause 4 pause 5 pause 6 pause 7])
> +
> +# Check that multiple levels of resubmit continue following resume.
> +#
> +# The "resubmit:5", which is relative to the current table, is
Should be “:55"
> +# particularly interesting because it checks that the notion of the
> +# current table is correctly preserved.
> +CHECK_CLOSURE([resubmit], [10], [0],
> + [table=0 in_port=1 actions=pause 2 pause resubmit(,1) pause 10 pause
> + table=1 in_port=1 actions=pause 3 pause resubmit(,2) pause 9 pause
> + table=2 in_port=1 actions=pause 4 pause resubmit(,3) pause 8 pause
> + table=3 in_port=1 actions=pause 5 pause resubmit:55 pause 7 pause
> + table=3 in_port=55 actions=pause 6 pause])
> +
> +# Check that the action set is preserved across pause/resume.
> +CHECK_CLOSURE([action set], [3], [0],
> + [in_port=1 actions=1 pause resubmit(,1) pause 2
> + table=1 actions=write_actions(3)])
> +
> +# Check that metadata and the stack used by push and pop is preserved
> +# across pause/resume.
> +CHECK_CLOSURE([data stack], [3], [0],
> + [in_port=1 actions=pause dnl
> + set_field:1->reg0 dnl
> + pause dnl
> + set_field:2->reg1 dnl
> + pause dnl
> + output:NXM_NX_REG0[[]] dnl
> + pause dnl
> + push:NXM_NX_REG1[[]] dnl
> + dnl
> + pop:NXM_NX_REG2[[]] dnl
> + pause dnl
> + output:NXM_NX_REG2[[]] dnl
> + pause dnl
> + 3])
> +
> +# Check that mirror output occurs once and once only, even if
> +# separated by pause/resume.
> +CHECK_CLOSURE([mirroring], [5], [0],
> + [in_port=1 actions=pause 2 pause 3 pause 4 pause], [],
> + [ovs-vsctl \
> + set Bridge br0 mirrors=@m --\
> + --id=@p2 get Port p2 --\
> + --id=@p3 get Port p3 --\
> + --id=@p4 get Port p4 --\
> + --id=@p5 get Port p5 --\
> + --id=@m create Mirror name=mymirror select_dst_port=@p2,@p3,@p4
> output_port=@p5])
> +
> +# Check that hops across patch ports resume the entire pipeline.
This seems to be in conflict with the comment about pausing at the peer bridge
above. The pause at the peer bridge should only resume the pipeline at the peer
bridge (here br1), not the original bridge (here br0).
> +CHECK_CLOSURE([patch ports], [4], [1],
> + [table=0 in_port=1 actions=pause 2 resubmit(,1) pause 4
> + table=1 in_port=1 actions=pause 3 pause 10 pause],
> + [table=0 in_port=11 actions=pause 5 pause],
> + [ovs-vsctl \
> + -- add-port br0 patch10 \
> + -- set interface patch10 type=patch options:peer=patch11 \
> + ofport_request=10 \
> + -- add-port br1 patch11 \
> + -- set interface patch11 type=patch options:peer=patch10 \
> + ofport_request=11])
> +
(snip)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
