[ovs-dev] [PATCH] ofproto: ofp_packet_out messages in bundles
André Mantas
andremantas7 at gmail.com
Thu Jul 14 00:35:53 UTC 2016
Thanks for the review.
I'm not sure if I will be able to address your second point tho.
Would this be a problem?
As for the third point, would ofproto.c be a good place for the helper
function?
Something like:
static enum ofperr
ofproto_extract_packet_out(struct ofproto *p, const struct ofp_header *oh,
struct ofputil_packet_out *po,
struct dp_packet *payload, struct flow *flow)
Basically it receives pointers and validates + fills them.
The caller can then invoke ofproto_class->packet_out() passing the args
that were filled.
Jarno Rajahalme <jarno at ovn.org> escreveu no dia quarta, 29/06/2016 às 12:36:
>
> > On Jun 23, 2016, at 4:51 AM, Andre Mantas <andremantas7 at 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 <amantas at 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 at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
>
More information about the dev
mailing list