Pushed to master, Jarno
> On Jun 1, 2015, at 6:06 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote: > > >> On Jun 1, 2015, at 2:48 PM, Ben Pfaff <b...@nicira.com >> <mailto:b...@nicira.com>> wrote: >> >> On Mon, Jun 01, 2015 at 01:58:41PM -0700, Jarno Rajahalme wrote: >>> >>>> On Jun 1, 2015, at 1:47 PM, Jarno Rajahalme <jrajaha...@nicira.com >>>> <mailto:jrajaha...@nicira.com>> wrote: >>>> >>>> >>>>> On May 29, 2015, at 5:38 PM, Ben Pfaff <b...@nicira.com >>>>> <mailto:b...@nicira.com>> wrote: >>>>> >>>>> On Mon, May 18, 2015 at 04:10:15PM -0700, Jarno Rajahalme wrote: >>>>>> OpenFlow bundle messages should be decoded and validated at the time >>>>>> they are added to the bundle. This commit does this for flow mod and >>>>>> port mod messages. >>>>>> >>>>>> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com >>>>>> <mailto:jrajaha...@nicira.com>> >>>>> >>>>> It's pretty expensive to give every ofp_bundle_entry a 1-kB stub for >>>>> actions, when there might be many thousands of them allocated at a time >>>>> into a bundle. It might be better to accumulate the actions into a >>>>> function-local stub then use ofpbuf_clone() or similar to allocate a >>>>> correctly sized buffer for ofp_bundle_entry. >>>>> >>>>> I guess that it would be even cheaper, memory-wise (struct match by >>>>> itself is almost 1/2 kB!), to just decode the raw message a second time >>>>> when we apply the bundle, but maybe that is not worth the extra trouble. >>>> >>>> I???ll look if I can get the actual replacement rule created at the >>>> validation time for the versioned case (after patch 17). For this patch, >>>> I???ll just make the stub smaller (64 bytes?). >>>> >>> >>> Are you OK with deferring further optimization to the end of the series? >>> I.e., can I push this with a smaller stub? >> >> Why bother with a stub at all in the struct? I suggest using a >> function-local stub on the stack when decoding the flow_mod, then >> ofpbuf_steal_data() to put that data into the ofp_bundle. > > > OK, I got it now :-) Incremental: > > diff --git a/ofproto/bundles.h b/ofproto/bundles.h > index 7084b08..c8ce5c9 100644 > --- a/ofproto/bundles.h > +++ b/ofproto/bundles.h > @@ -33,13 +33,11 @@ extern "C" { > struct ofp_bundle_entry { > struct ovs_list node; > ovs_be32 xid; /* For error returns. */ > - enum ofptype type; > + enum ofptype type; /* OFPTYPE_FLOW_MOD or OFPTYPE_PORT_MOD. */ > union { > - struct ofputil_flow_mod fm; > + struct ofputil_flow_mod fm; /* 'fm.ofpacts' must be malloced. */ > struct ofputil_port_mod pm; > }; > - struct ofpbuf ofpacts; > - uint64_t ofpacts_stub[64 / 8]; > }; > > static inline struct ofp_bundle_entry *ofp_bundle_entry_alloc( > @@ -59,8 +57,6 @@ ofp_bundle_entry_alloc(enum ofptype type, ovs_be32 xid) > { > struct ofp_bundle_entry *entry = xmalloc(sizeof *entry); > > - ofpbuf_use_stub(&entry->ofpacts, entry->ofpacts_stub, > - sizeof entry->ofpacts_stub); > entry->xid = xid; > entry->type = type; > > @@ -70,7 +66,9 @@ ofp_bundle_entry_alloc(enum ofptype type, ovs_be32 xid) > static inline void ofp_bundle_entry_free(struct ofp_bundle_entry *entry) > { > if (entry) { > - ofpbuf_uninit(&entry->ofpacts); > + if (entry->type == OFPTYPE_FLOW_MOD) { > + free(entry->fm.ofpacts); > + } > free(entry); > } > } > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c > index 54d66b6..0a8c82a 100644 > --- a/ofproto/ofproto.c > +++ b/ofproto/ofproto.c > @@ -6378,20 +6378,26 @@ handle_bundle_add(struct ofconn *ofconn, const struct > ofp_header *oh) > if (type == OFPTYPE_PORT_MOD) { > error = ofputil_decode_port_mod(badd.msg, &bmsg->pm, false); > } else if (type == OFPTYPE_FLOW_MOD) { > + struct ofpbuf ofpacts; > + uint64_t ofpacts_stub[1024 / 8]; > + > + ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub); > error = ofputil_decode_flow_mod(&bmsg->fm, badd.msg, > ofconn_get_protocol(ofconn), > - &bmsg->ofpacts, > + &ofpacts, > u16_to_ofp(ofproto->max_ports), > ofproto->n_tables); > + /* Move actions to heap. */ > + bmsg->fm.ofpacts = ofpbuf_steal_data(&ofpacts); > + > + if (!error && bmsg->fm.ofpacts_len) { > + error = ofproto_check_ofpacts(ofproto, bmsg->fm.ofpacts, > + bmsg->fm.ofpacts_len); > + } > } else { > OVS_NOT_REACHED(); > } > > - if (!error && bmsg->ofpacts.size) { > - error = ofproto_check_ofpacts(ofproto, bmsg->ofpacts.data, > - bmsg->ofpacts.size); > - } > - > if (!error) { > error = ofp_bundle_add_message(ofconn, badd.bundle_id, badd.flags, > bmsg); > > > Jarno _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
