> On Jun 1, 2015, at 2:48 PM, Ben Pfaff <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> wrote:
>>>
>>>
>>>> On May 29, 2015, at 5:38 PM, Ben Pfaff <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>
>>>>
>>>> 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
