I've been working on adding group support for bundles for the last week or so, so I have most of the related code fresh in my head now. So, please submit a new RFC patch with the changes you can make and I'll take it from there.
Jarno > On Jul 13, 2016, at 5:35 PM, André Mantas <andremant...@gmail.com> wrote: > > 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 <ja...@ovn.org <mailto:ja...@ovn.org>> escreveu no dia > quarta, 29/06/2016 às 12:36: > > > On Jun 23, 2016, at 4:51 AM, Andre Mantas <andremant...@gmail.com > > <mailto: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 > > <mailto:aman...@lasige.di.fc.ul.pt>> > > Submitted-at: https://github.com/openvswitch/ovs/pull/117 > > <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 <http://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 <mailto:dev@openvswitch.org> > > http://openvswitch.org/mailman/listinfo/dev > > <http://openvswitch.org/mailman/listinfo/dev> > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
