> On Jun 23, 2016, at 4:51 AM, Andre Mantas <andremant...@gmail.com> wrote:
>
> This patch allows ofp_packet_out messages to be added to bundles. In a
> multi controller scenario, packet_out messages inside bundles can serve
> as a commit_reply for other controllers - since the original commit_reply
> is only forwarded to the controller that sent the commit_request.
>
> Tested with make check and external controller adding packet_out messages
> to a bundle with different destinations (hosts and controllers). Also tested
> grouping packet_out with port_mod messages in the same bundle. After
> committing the bundle, the destinations received the packet_out.
>
> Signed-off-by: Andre Mantas <aman...@lasige.di.fc.ul.pt>
> Submitted-at: https://github.com/openvswitch/ovs/pull/117
> ---
> lib/ofp-util.c | 2 +-
> ofproto/bundles.h | 5 ++++
> ofproto/ofproto-provider.h | 7 +++++
> ofproto/ofproto.c | 70 ++++++++++++++++++++++++++++++++++++++++++++--
> 4 files changed, 81 insertions(+), 3 deletions(-)
>
> diff --git a/lib/ofp-util.c b/lib/ofp-util.c
> index c5353cc..6725220 100644
> --- a/lib/ofp-util.c
> +++ b/lib/ofp-util.c
> @@ -9578,6 +9578,7 @@ ofputil_is_bundlable(enum ofptype type)
> /* Minimum required by OpenFlow 1.4. */
> case OFPTYPE_PORT_MOD:
> case OFPTYPE_FLOW_MOD:
> + case OFPTYPE_PACKET_OUT:
> return true;
>
You should update the preceding comment, too.
> /* Nice to have later. */
> @@ -9585,7 +9586,6 @@ ofputil_is_bundlable(enum ofptype type)
> case OFPTYPE_GROUP_MOD:
> case OFPTYPE_TABLE_MOD:
> case OFPTYPE_METER_MOD:
> - case OFPTYPE_PACKET_OUT:
> case OFPTYPE_NXT_TLV_TABLE_MOD:
>
> /* Not to be bundlable. */
> diff --git a/ofproto/bundles.h b/ofproto/bundles.h
> index d045031..61c1b48 100644
> --- a/ofproto/bundles.h
> +++ b/ofproto/bundles.h
> @@ -22,6 +22,7 @@
> #include <sys/types.h>
>
> #include "connmgr.h"
> +#include "dp-packet.h"
> #include "ofproto-provider.h"
> #include "openvswitch/ofp-msgs.h"
> #include "openvswitch/ofp-util.h"
> @@ -37,6 +38,7 @@ struct ofp_bundle_entry {
> union {
> struct ofproto_flow_mod ofm; /* ofm.fm.ofpacts must be malloced. */
> struct ofproto_port_mod opm;
> + struct ofproto_packet_out opo; /* opo.po.ofpacts must be malloced */
> };
>
> /* OpenFlow header and some of the message contents for error reporting.
> */
> @@ -89,6 +91,9 @@ ofp_bundle_entry_free(struct ofp_bundle_entry *entry)
> if (entry) {
> if (entry->type == OFPTYPE_FLOW_MOD) {
> free(entry->ofm.fm.ofpacts);
> + } else if (entry->type == OFPTYPE_PACKET_OUT) {
> + dp_packet_delete(entry->opo.payload);
> + free(entry->opo.po.ofpacts);
> }
> free(entry);
> }
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index daa0077..c98f110 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1814,6 +1814,13 @@ struct ofproto_port_mod {
> struct ofport *port; /* Affected port. */
> };
>
> +/* packet_out with execution context. */
> +struct ofproto_packet_out {
> + struct ofputil_packet_out po;
> + struct dp_packet *payload;
> + struct flow flow;
> +};
> +
> enum ofperr ofproto_flow_mod(struct ofproto *, struct ofproto_flow_mod *)
> OVS_EXCLUDED(ofproto_mutex);
> void ofproto_add_flow(struct ofproto *, const struct match *, int priority,
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index ff6affd..c5a6097 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -6996,6 +6996,8 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id,
> uint16_t flags)
> * effect. */
> be->ofm.version = version;
> error = ofproto_flow_mod_start(ofproto, &be->ofm);
> + } else if (be->type == OFPTYPE_PACKET_OUT) {
> + prev_is_port_mod = false;
> } else {
> OVS_NOT_REACHED();
> }
> @@ -7016,7 +7018,7 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id,
> uint16_t flags)
> if (be->type == OFPTYPE_FLOW_MOD) {
> ofproto_flow_mod_revert(ofproto, &be->ofm);
> }
> - /* Nothing needs to be reverted for a port mod. */
> + /* Nothing needs to be reverted for a port mod or packet
> out. */
> }
> } else {
> /* 4. Finish. */
> @@ -7042,6 +7044,18 @@ do_bundle_commit(struct ofconn *ofconn, uint32_t id,
> uint16_t flags)
> * also from OVSDB changes asynchronously to all upcall
> * processing. */
> port_mod_finish(ofconn, &be->opm.pm, be->opm.port);
> + } else if (be->type == OFPTYPE_PACKET_OUT) {
> + /* Try to send the packet. The bundle is committed
> + * regardless of errors */
> + error = ofproto->ofproto_class->packet_out(ofproto,
> + be->opo.payload,
> + &(be->opo.flow),
> + be->opo.po.ofpacts,
> +
> be->opo.po.ofpacts_len);
> + if (error) {
> + VLOG_INFO("Error sending packet out while committing
> a "
> + "bundle: %d", error);
> + }
In principle having failures at this stage is not acceptable. You should
refactor the packet_out() callback to two parts, one that does all the work
except for actually sending out the packet and save enough context to be able
to revert or execute the sending at the later stages.
Also, the actions in packet out can depend on the changes introduced earlier in
the transaction, so you should consider the interaction with the 'ordered' and
'atomic' flags. It might be possible to support both by doing the translation
for the packet out in the latest, 'unpublished' version, even though all the
other upcalls use only the published version of the flow tables. This should be
safe as the translation happens from the same thread that is doing the changes
to the flow tables (i.e., the main thread). This way we could have both: Each
packet out would reflect the flow table as modified by the preceding flow_mods
in the transaction and the whole transaction (including the possible flow_mods
after the packet_out) would be made visible to datapath lookups as one big
atomic transaction.
> }
> }
> }
> @@ -7150,6 +7164,58 @@ handle_bundle_add(struct ofconn *ofconn, const struct
> ofp_header *oh)
> error = ofproto_check_ofpacts(ofproto, bmsg->ofm.fm.ofpacts,
> bmsg->ofm.fm.ofpacts_len);
> }
> + } else if (type == OFPTYPE_PACKET_OUT) {
> + uint64_t ofpacts_stub[1024 / 8];
> + struct ofpbuf ofpacts;
> +
> + /* Decode message. */
> + ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
> + error = ofputil_decode_packet_out(&(bmsg->opo.po), badd.msg,
> &ofpacts);
> + /* Same validation steps as in handle_packet_out */
> + if (error) {
> + goto exit;
> + }
> +
> + /* Move actions to heap. */
> + bmsg->opo.po.ofpacts = ofpbuf_steal_data(&ofpacts);
> +
> + if (ofp_to_u16(bmsg->opo.po.in_port) >= ofproto->max_ports
> + && ofp_to_u16(bmsg->opo.po.in_port) < ofp_to_u16(OFPP_MAX)) {
> + error = OFPERR_OFPBRC_BAD_PORT;
> + goto exit;
> + }
> +
> + /* Get payload. */
> + if (bmsg->opo.po.buffer_id != UINT32_MAX) {
> + error = ofconn_pktbuf_retrieve(ofconn, bmsg->opo.po.buffer_id,
> + &(bmsg->opo.payload), NULL);
> + if (error) {
> + goto exit;
> + }
> + } else {
> + /* Ensure that the L3 header is 32-bit aligned. */
> + bmsg->opo.payload = dp_packet_clone_data_with_headroom(
> + bmsg->opo.po.packet,
> +
> bmsg->opo.po.packet_len,
> + 2);
> + }
> +
> + /* Verify actions against packet */
> + flow_extract(bmsg->opo.payload, &(bmsg->opo.flow));
> + bmsg->opo.flow.in_port.ofp_port = bmsg->opo.po.in_port;
> + /* Check actions like for flow mods. */
> + error = ofpacts_check_consistency(bmsg->opo.po.ofpacts,
> + bmsg->opo.po.ofpacts_len,
> + &(bmsg->opo.flow),
> + u16_to_ofp(ofproto->max_ports),
> + 0, ofproto->n_tables,
> + ofconn_get_protocol(ofconn));
> + if (error) {
> + goto exit;
> + }
> +
> + error = ofproto_check_ofpacts(ofproto, bmsg->opo.po.ofpacts,
> + bmsg->opo.po.ofpacts_len);
You should try to minimize code duplication by refactoring as much of this as
possible to new helper function(s) shared by both code paths.
> } else {
> OVS_NOT_REACHED();
> }
> @@ -7158,7 +7224,7 @@ handle_bundle_add(struct ofconn *ofconn, const struct
> ofp_header *oh)
> error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags,
> bmsg);
> }
> -
> +exit:
> if (error) {
> ofp_bundle_entry_free(bmsg);
> }
> --
> 2.5.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
