[ovs-dev] [PATCH] ofproto: ofp_packet_out messages in bundles

Jarno Rajahalme jarno at ovn.org
Thu Jul 14 09:10:57 UTC 2016


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 <mailto: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 <mailto: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 <mailto:amantas at 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 at openvswitch.org <mailto:dev at openvswitch.org>
> > http://openvswitch.org/mailman/listinfo/dev <http://openvswitch.org/mailman/listinfo/dev>
> 




More information about the dev mailing list