[ovs-dev] [PATCH] ofproto: ofp_packet_out messages in bundles
André Mantas
andremantas7 at gmail.com
Sat Jul 16 21:50:44 UTC 2016
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 <jarno at 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 <andremantas7 at 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 <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