I've sent the new patch. I wasn't sure how to do it so I just sent the first patch with subject [PATCH 1/2] and the new patch with [PATCH 2/2].
Hope it doesn't cause much problem. Alternatively I can send a pull request in github if you want. Thanks. Jarno Rajahalme <ja...@ovn.org> escreveu no dia quinta, 14/07/2016 às 10:11: > 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> escreveu no dia quarta, 29/06/2016 às > 12:36: > >> >> > 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