[ovs-dev] [classifier-opt 10/28] ofproto: Move ofpacts_check() calls from ofproto-dpif to ofproto.

Ben Pfaff blp at nicira.com
Tue Aug 7 19:47:11 UTC 2012


OK, thanks.

Perhaps when (if?) someone submits another ofproto provider to master,
we can figure out what best suits the set of providers as a whole.

On Tue, Aug 07, 2012 at 12:44:52PM -0700, Ethan Jackson wrote:
> I don't feel particularly strongly about it.  The current
> implementation is fine with me.
> 
> Ethan
> 
> On Tue, Aug 7, 2012 at 12:40 PM, Ben Pfaff <blp at nicira.com> wrote:
> > I think you and I might have slightly different models in mind.  The
> > model that I'm after at the moment is that the ofproto generic layer
> > checks that a set of actions is valid (which can be done without
> > knowing anything about the hardware) and then the implementation
> > checks that it can actually implement those valid actions.
> >
> > I agree that the approach you suggest would work too, and that it
> > could work with either the model I have in mind or a model where the
> > implementation has to do all of the validation as well as the
> > implement-ability checking.
> >
> > So, I'm inclined to leave it the way I have it, but I remain open to
> > further discussion.
> >
> > On Mon, Jul 30, 2012 at 02:45:26PM -0700, Ethan Jackson wrote:
> >> Looks good to me.
> >>
> >> I had a thought but I don't feel strongly about it.  What if we made a
> >> new hook for ofproto implementations:
> >>
> >> enum ofperr (*validate_actions)(const struct ofpact ofpacts[], size_t
> >> ofpacts_len, const struct flow *, int max_ports).
> >>
> >> In the ofproto-dpif implementation this would simply pass through to
> >> ofpacts_check(), but in other implementations it could reject action
> >> lists which are not implementable by their dpif.  This would allow us
> >> to make *packet_out() and *rule_construct() assume that the actions
> >> are both valid and implementable.  Then they could return void instead
> >> of an ofperr.  Also if we did this, rule_modify_actions() may be
> >> completely unnecessary.  The complete_operation() calls could be
> >> pulled into ofproto.
> >>
> >> At any rate, this seems marginally cleaner to me, but you may feel
> >> strongly in the other direction in which case the current approach is
> >> fine.
> >>
> >> Ethan
> >>
> >> On Fri, Jul 20, 2012 at 4:25 PM, Ben Pfaff <blp at nicira.com> wrote:
> >> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> >> > ---
> >> >  ofproto/ofproto-dpif.c     |   58 ++++++++++++++-----------------------------
> >> >  ofproto/ofproto-provider.h |   14 ++++-------
> >> >  ofproto/ofproto.c          |   16 +++++++++---
> >> >  3 files changed, 36 insertions(+), 52 deletions(-)
> >> >
> >> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> >> > index f1d2ae0..44cbd17 100644
> >> > --- a/ofproto/ofproto-dpif.c
> >> > +++ b/ofproto/ofproto-dpif.c
> >> > @@ -4592,13 +4592,6 @@ rule_construct(struct rule *rule_)
> >> >      struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> >> >      struct rule_dpif *victim;
> >> >      uint8_t table_id;
> >> > -    enum ofperr error;
> >> > -
> >> > -    error = ofpacts_check(rule->up.ofpacts, rule->up.ofpacts_len,
> >> > -                          &rule->up.cr.flow, ofproto->up.max_ports);
> >> > -    if (error) {
> >> > -        return error;
> >> > -    }
> >> >
> >> >      rule->packet_count = 0;
> >> >      rule->byte_count = 0;
> >> > @@ -4701,15 +4694,6 @@ static void
> >> >  rule_modify_actions(struct rule *rule_)
> >> >  {
> >> >      struct rule_dpif *rule = rule_dpif_cast(rule_);
> >> > -    struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
> >> > -    enum ofperr error;
> >> > -
> >> > -    error = ofpacts_check(rule->up.ofpacts, rule->up.ofpacts_len,
> >> > -                          &rule->up.cr.flow, ofproto->up.max_ports);
> >> > -    if (error) {
> >> > -        ofoperation_complete(rule->up.pending, error);
> >> > -        return;
> >> > -    }
> >> >
> >> >      complete_operation(rule);
> >> >  }
> >> > @@ -6359,36 +6343,32 @@ packet_out(struct ofproto *ofproto_, struct ofpbuf *packet,
> >> >             const struct ofpact *ofpacts, size_t ofpacts_len)
> >> >  {
> >> >      struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> >> > -    enum ofperr error;
> >> > +    struct odputil_keybuf keybuf;
> >> > +    struct dpif_flow_stats stats;
> >> >
> >> > -    error = ofpacts_check(ofpacts, ofpacts_len, flow, ofproto->up.max_ports);
> >> > -    if (!error) {
> >> > -        struct odputil_keybuf keybuf;
> >> > -        struct dpif_flow_stats stats;
> >> > +    struct ofpbuf key;
> >> >
> >> > -        struct ofpbuf key;
> >> > +    struct action_xlate_ctx ctx;
> >> > +    uint64_t odp_actions_stub[1024 / 8];
> >> > +    struct ofpbuf odp_actions;
> >> >
> >> > -        struct action_xlate_ctx ctx;
> >> > -        uint64_t odp_actions_stub[1024 / 8];
> >> > -        struct ofpbuf odp_actions;
> >> > +    ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> >> > +    odp_flow_key_from_flow(&key, flow);
> >> >
> >> > -        ofpbuf_use_stack(&key, &keybuf, sizeof keybuf);
> >> > -        odp_flow_key_from_flow(&key, flow);
> >> > +    dpif_flow_stats_extract(flow, packet, &stats);
> >> >
> >> > -        dpif_flow_stats_extract(flow, packet, &stats);
> >> > +    action_xlate_ctx_init(&ctx, ofproto, flow, flow->vlan_tci, NULL,
> >> > +                          packet_get_tcp_flags(packet, flow), packet);
> >> > +    ctx.resubmit_stats = &stats;
> >> >
> >> > -        action_xlate_ctx_init(&ctx, ofproto, flow, flow->vlan_tci, NULL,
> >> > -                              packet_get_tcp_flags(packet, flow), packet);
> >> > -        ctx.resubmit_stats = &stats;
> >> > +    ofpbuf_use_stub(&odp_actions,
> >> > +                    odp_actions_stub, sizeof odp_actions_stub);
> >> > +    xlate_actions(&ctx, ofpacts, ofpacts_len, &odp_actions);
> >> > +    dpif_execute(ofproto->dpif, key.data, key.size,
> >> > +                 odp_actions.data, odp_actions.size, packet);
> >> > +    ofpbuf_uninit(&odp_actions);
> >> >
> >> > -        ofpbuf_use_stub(&odp_actions,
> >> > -                        odp_actions_stub, sizeof odp_actions_stub);
> >> > -        xlate_actions(&ctx, ofpacts, ofpacts_len, &odp_actions);
> >> > -        dpif_execute(ofproto->dpif, key.data, key.size,
> >> > -                     odp_actions.data, odp_actions.size, packet);
> >> > -        ofpbuf_uninit(&odp_actions);
> >> > -    }
> >> > -    return error;
> >> > +    return 0;
> >> >  }
> >> >
> >> >  /* NetFlow. */
> >> > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> >> > index 2f62473..80cd55f 100644
> >> > --- a/ofproto/ofproto-provider.h
> >> > +++ b/ofproto/ofproto-provider.h
> >> > @@ -803,11 +803,7 @@ struct ofproto_class {
> >> >       *     registers, then it is an error if 'rule->cr' does not wildcard all
> >> >       *     registers.
> >> >       *
> >> > -     *   - Validate that 'rule->ofpacts' is a sequence of well-formed actions
> >> > -     *     that the datapath can correctly implement.  If your ofproto
> >> > -     *     implementation only implements a subset of the actions that Open
> >> > -     *     vSwitch understands, then you should implement your own action
> >> > -     *     validation.
> >> > +     *   - Validate that the datapath can correctly implement 'rule->ofpacts'.
> >> >       *
> >> >       *   - If the rule is valid, update the datapath flow table, adding the new
> >> >       *     rule or replacing the existing one.
> >> > @@ -889,8 +885,8 @@ struct ofproto_class {
> >> >       *
> >> >       * ->rule_modify_actions() should set the following in motion:
> >> >       *
> >> > -     *   - Validate that the actions now in 'rule' are well-formed OpenFlow
> >> > -     *     actions that the datapath can correctly implement.
> >> > +     *   - Validate that the datapath can correctly implement the actions now
> >> > +     *     in 'rule'.
> >> >       *
> >> >       *   - Update the datapath flow table with the new actions.
> >> >       *
> >> > @@ -943,8 +939,8 @@ struct ofproto_class {
> >> >       * The caller retains ownership of 'packet' and of 'ofpacts', so
> >> >       * ->packet_out() should not modify or free them.
> >> >       *
> >> > -     * This function must validate that it can implement 'ofpacts'.  If not,
> >> > -     * then it should return an OpenFlow error code.
> >> > +     * This function must validate that it can correctly implement 'ofpacts'.
> >> > +     * If not, then it should return an OpenFlow error code.
> >> >       *
> >> >       * 'flow' reflects the flow information for 'packet'.  All of the
> >> >       * information in 'flow' is extracted from 'packet', except for
> >> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> >> > index cbcf0d2..17131ca 100644
> >> > --- a/ofproto/ofproto.c
> >> > +++ b/ofproto/ofproto.c
> >> > @@ -2138,10 +2138,13 @@ handle_packet_out(struct ofconn *ofconn, const struct ofp_packet_out *opo)
> >> >          ofpbuf_use_const(payload, po.packet, po.packet_len);
> >> >      }
> >> >
> >> > -    /* Send out packet. */
> >> > +    /* Verify actions against packet, then send packet if successful. */
> >> >      flow_extract(payload, 0, 0, po.in_port, &flow);
> >> > -    error = p->ofproto_class->packet_out(p, payload, &flow,
> >> > -                                         po.ofpacts, po.ofpacts_len);
> >> > +    error = ofpacts_check(po.ofpacts, po.ofpacts_len, &flow, p->max_ports);
> >> > +    if (!error) {
> >> > +        error = p->ofproto_class->packet_out(p, payload, &flow,
> >> > +                                             po.ofpacts, po.ofpacts_len);
> >> > +    }
> >> >      ofpbuf_delete(payload);
> >> >
> >> >  exit_free_ofpacts:
> >> > @@ -3237,7 +3240,12 @@ handle_flow_mod(struct ofconn *ofconn, const struct ofp_header *oh)
> >> >           * dropped from OpenFlow in the near future.  There is no good error
> >> >           * code, so just state that the flow table is full. */
> >> >          error = OFPERR_OFPFMFC_ALL_TABLES_FULL;
> >> > -    } else {
> >> > +    }
> >> > +    if (!error) {
> >> > +        error = ofpacts_check(fm.ofpacts, fm.ofpacts_len,
> >> > +                              &fm.cr.flow, ofproto->max_ports);
> >> > +    }
> >> > +    if (!error) {
> >> >          error = handle_flow_mod__(ofconn_get_ofproto(ofconn), ofconn, &fm, oh);
> >> >      }
> >> >      if (error) {
> >> > --
> >> > 1.7.2.5
> >> >



More information about the dev mailing list