> Perhaps when (if?) someone submits another ofproto provider to master,
> we can figure out what best suits the set of providers as a whole.
Yah it's hard to get the abstraction right when we only have deep
knowledge of one implementation. Hopefully a significant open source
secondary implementation will pop up and we will learn a lot from it.
Ethan
>
> 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 <b...@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 <b...@nicira.com> wrote:
>> >> > Signed-off-by: Ben Pfaff <b...@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
>> >> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
