From: Jarno Rajahalme Date: Tue, 29 Apr 2014 22:50:38 +0000 (-0700) Subject: ofproto: Inline actions in struct rule_actions. X-Git-Tag: sliver-openvswitch-2.2.90-1~3^2~42 X-Git-Url: http://git.onelab.eu/?p=sliver-openvswitch.git;a=commitdiff_plain;h=dc723c447a797e555d400594133a35b9841eb1de ofproto: Inline actions in struct rule_actions. Allocate struct rule_actions and the space for the actions at once. This reduces one memory indirection and helps reduce cache misses visible in perf annotations. Fix some old comments referring to ref count, since we now use RCU for this. Enforce constness of the actions that are assigned from rule_actions throughout the code. Signed-off-by: Jarno Rajahalme Acked-by: Ben Pfaff --- diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h index 1db3bdee2..be1a0aa7e 100644 --- a/lib/ofp-actions.h +++ b/lib/ofp-actions.h @@ -234,8 +234,8 @@ struct ofpact_enqueue { * Used for NXAST_OUTPUT_REG. */ struct ofpact_output_reg { struct ofpact ofpact; - struct mf_subfield src; uint16_t max_len; + struct mf_subfield src; }; /* OFPACT_BUNDLE. diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c index d2500427e..c759f036b 100644 --- a/lib/ofp-parse.c +++ b/lib/ofp-parse.c @@ -1924,7 +1924,7 @@ parse_ofp_flow_mod_file(const char *file_name, uint16_t command, size_t i; for (i = 0; i < *n_fms; i++) { - free((*fms)[i].ofpacts); + free(CONST_CAST(struct ofpact *, (*fms)[i].ofpacts)); } free(*fms); *fms = NULL; diff --git a/lib/ofp-util.h b/lib/ofp-util.h index 245cc4e06..fe2c7c23b 100644 --- a/lib/ofp-util.h +++ b/lib/ofp-util.h @@ -298,8 +298,8 @@ struct ofputil_flow_mod { ofp_port_t out_port; uint32_t out_group; enum ofputil_flow_mod_flags flags; - struct ofpact *ofpacts; /* Series of "struct ofpact"s. */ - size_t ofpacts_len; /* Length of ofpacts, in bytes. */ + struct ofpact *ofpacts; /* Series of "struct ofpact"s. */ + size_t ofpacts_len; /* Length of ofpacts, in bytes. */ }; enum ofperr ofputil_decode_flow_mod(struct ofputil_flow_mod *, @@ -341,7 +341,7 @@ struct ofputil_flow_stats { int hard_age; /* Seconds since last change, -1 if unknown. */ uint64_t packet_count; /* Packet count, UINT64_MAX if unknown. */ uint64_t byte_count; /* Byte count, UINT64_MAX if unknown. */ - struct ofpact *ofpacts; + const struct ofpact *ofpacts; size_t ofpacts_len; enum ofputil_flow_mod_flags flags; }; @@ -854,7 +854,7 @@ struct ofputil_flow_update { uint16_t priority; ovs_be64 cookie; struct match *match; - struct ofpact *ofpacts; + const struct ofpact *ofpacts; size_t ofpacts_len; /* Used only for NXFME_ABBREV. */ diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index 383fbdacc..9a5167de8 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -2105,7 +2105,7 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule, ovs_mutex_unlock(&rule->mutex); if (flags & NXFMF_ACTIONS) { - struct rule_actions *actions = rule_get_actions(rule); + const struct rule_actions *actions = rule_get_actions(rule); fu.ofpacts = actions->ofpacts; fu.ofpacts_len = actions->ofpacts_len; } else { diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 4a9ba5203..e2ac9adbc 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1992,7 +1992,7 @@ static void xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule) { struct rule_dpif *old_rule = ctx->rule; - struct rule_actions *actions; + const struct rule_actions *actions; if (ctx->xin->resubmit_stats) { rule_dpif_credit_stats(rule, ctx->xin->resubmit_stats); @@ -3191,7 +3191,7 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out *xout) struct flow *flow = &xin->flow; struct rule_dpif *rule = NULL; - struct rule_actions *actions = NULL; + const struct rule_actions *actions = NULL; enum slow_path_reason special; const struct ofpact *ofpacts; struct xport *in_port; diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 58c506436..001a2c044 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -3203,7 +3203,7 @@ rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout, /* Returns 'rule''s actions. The caller owns a reference on the returned * actions and must eventually release it (with rule_actions_unref()) to avoid * a memory leak. */ -struct rule_actions * +const struct rule_actions * rule_dpif_get_actions(const struct rule_dpif *rule) { return rule_get_actions(&rule->up); @@ -3865,7 +3865,7 @@ struct trace_ctx { static void trace_format_rule(struct ds *result, int level, const struct rule_dpif *rule) { - struct rule_actions *actions; + const struct rule_actions *actions; ovs_be64 cookie; ds_put_char_multiple(result, '\t', level); diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index 8af66458e..94d4e1e70 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -109,7 +109,7 @@ bool rule_dpif_is_table_miss(const struct rule_dpif *); bool rule_dpif_is_internal(const struct rule_dpif *); uint8_t rule_dpif_get_table(const struct rule_dpif *); -struct rule_actions *rule_dpif_get_actions(const struct rule_dpif *); +const struct rule_actions *rule_dpif_get_actions(const struct rule_dpif *); ovs_be64 rule_dpif_get_flow_cookie(const struct rule_dpif *rule); diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 11dcd8268..9431be875 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -39,6 +39,7 @@ #include "heap.h" #include "hindex.h" #include "list.h" +#include "ofp-actions.h" #include "ofp-errors.h" #include "ofp-util.h" #include "ofproto/ofproto.h" @@ -50,7 +51,6 @@ #include "timeval.h" struct match; -struct ofpact; struct ofputil_flow_mod; struct bfd_cfg; struct meter; @@ -377,7 +377,7 @@ struct rule { /* OpenFlow actions. See struct rule_actions for more thread-safety * notes. */ - OVSRCU_TYPE(struct rule_actions *) actions; + OVSRCU_TYPE(const struct rule_actions *) actions; /* In owning meter's 'rules' list. An empty list if there is no meter. */ struct list meter_list_node OVS_GUARDED_BY(ofproto_mutex); @@ -406,10 +406,10 @@ struct rule { void ofproto_rule_ref(struct rule *); void ofproto_rule_unref(struct rule *); -static inline struct rule_actions * +static inline const struct rule_actions * rule_get_actions(const struct rule *rule) { - return ovsrcu_get(struct rule_actions *, &rule->actions); + return ovsrcu_get(const struct rule_actions *, &rule->actions); } /* Returns true if 'rule' is an OpenFlow 1.3 "table-miss" rule, false @@ -431,21 +431,22 @@ bool rule_is_internal(const struct rule *); * Thread-safety * ============= * - * A struct rule_actions 'actions' may be accessed without a risk of being + * A struct rule_actions may be accessed without a risk of being * freed by code that holds a read-lock or write-lock on 'rule->mutex' (where - * 'rule' is the rule for which 'rule->actions == actions') or that owns a - * reference to 'actions->ref_count' (or both). */ + * 'rule' is the rule for which 'rule->actions == actions') or during the RCU + * active period. */ struct rule_actions { /* These members are immutable: they do not change during the struct's * lifetime. */ - struct ofpact *ofpacts; /* Sequence of "struct ofpacts". */ - unsigned int ofpacts_len; /* Size of 'ofpacts', in bytes. */ - uint32_t provider_meter_id; /* Datapath meter_id, or UINT32_MAX. */ + uint32_t ofpacts_len; /* Size of 'ofpacts', in bytes. */ + uint32_t provider_meter_id; /* Datapath meter_id, or UINT32_MAX. */ + struct ofpact ofpacts[]; /* Sequence of "struct ofpacts". */ }; +BUILD_ASSERT_DECL(offsetof(struct rule_actions, ofpacts) % OFPACT_ALIGNTO == 0); -struct rule_actions *rule_actions_create(const struct ofproto *, - const struct ofpact *, size_t); -void rule_actions_destroy(struct rule_actions *); +const struct rule_actions *rule_actions_create(const struct ofproto *, + const struct ofpact *, size_t); +void rule_actions_destroy(const struct rule_actions *); /* A set of rules to which an OpenFlow operation applies. */ struct rule_collection { diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 49444c193..6195b75be 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -125,7 +125,7 @@ struct ofoperation { /* OFOPERATION_MODIFY, OFOPERATION_REPLACE: The old actions, if the actions * are changing. */ - struct rule_actions *actions; + const struct rule_actions *actions; /* OFOPERATION_DELETE. */ enum ofp_flow_removed_reason reason; /* Reason flow was removed. */ @@ -1941,7 +1941,7 @@ ofproto_add_flow(struct ofproto *ofproto, const struct match *match, rule = rule_from_cls_rule(classifier_find_match_exactly( &ofproto->tables[0].cls, match, priority)); if (rule) { - struct rule_actions *actions = rule_get_actions(rule); + const struct rule_actions *actions = rule_get_actions(rule); must_add = !ofpacts_equal(actions->ofpacts, actions->ofpacts_len, ofpacts, ofpacts_len); } else { @@ -2686,38 +2686,30 @@ ofproto_rule_unref(struct rule *rule) static uint32_t get_provider_meter_id(const struct ofproto *, uint32_t of_meter_id); -/* Creates and returns a new 'struct rule_actions', with a ref_count of 1, - * whose actions are a copy of from the 'ofpacts_len' bytes of 'ofpacts'. */ -struct rule_actions * +/* Creates and returns a new 'struct rule_actions', whose actions are a copy + * of from the 'ofpacts_len' bytes of 'ofpacts'. */ +const struct rule_actions * rule_actions_create(const struct ofproto *ofproto, const struct ofpact *ofpacts, size_t ofpacts_len) { struct rule_actions *actions; - actions = xmalloc(sizeof *actions); - actions->ofpacts = xmemdup(ofpacts, ofpacts_len); + actions = xmalloc(sizeof *actions + ofpacts_len); actions->ofpacts_len = ofpacts_len; actions->provider_meter_id = get_provider_meter_id(ofproto, ofpacts_get_meter(ofpacts, ofpacts_len)); + memcpy(actions->ofpacts, ofpacts, ofpacts_len); return actions; } -static void -rule_actions_destroy_cb(struct rule_actions *actions) -{ - free(actions->ofpacts); - free(actions); -} - -/* Decrements 'actions''s ref_count and frees 'actions' if the ref_count - * reaches 0. */ +/* Free the actions after the RCU quiescent period is reached. */ void -rule_actions_destroy(struct rule_actions *actions) +rule_actions_destroy(const struct rule_actions *actions) { if (actions) { - ovsrcu_postpone(rule_actions_destroy_cb, actions); + ovsrcu_postpone(free, CONST_CAST(struct rule_actions *, actions)); } } @@ -3661,7 +3653,7 @@ handle_flow_stats_request(struct ofconn *ofconn, long long int now = time_msec(); struct ofputil_flow_stats fs; long long int created, used, modified; - struct rule_actions *actions; + const struct rule_actions *actions; enum ofputil_flow_mod_flags flags; ovs_mutex_lock(&rule->mutex); @@ -3702,7 +3694,7 @@ static void flow_stats_ds(struct rule *rule, struct ds *results) { uint64_t packet_count, byte_count; - struct rule_actions *actions; + const struct rule_actions *actions; long long int created, used; rule->ofproto->ofproto_class->rule_get_stats(rule, &packet_count, @@ -4244,7 +4236,7 @@ modify_flows__(struct ofproto *ofproto, struct ofconn *ofconn, reset_counters = (fm->flags & OFPUTIL_FF_RESET_COUNTS) != 0; if (actions_changed || reset_counters) { - struct rule_actions *new_actions; + const struct rule_actions *new_actions; op->actions = rule_get_actions(rule); new_actions = rule_actions_create(ofproto, @@ -6342,7 +6334,7 @@ ofopgroup_complete(struct ofopgroup *group) rule->hard_timeout = op->hard_timeout; ovs_mutex_unlock(&rule->mutex); if (op->actions) { - struct rule_actions *old_actions; + const struct rule_actions *old_actions; ovs_mutex_lock(&rule->mutex); old_actions = rule_get_actions(rule); @@ -6918,7 +6910,7 @@ oftable_insert_rule(struct rule *rule) { struct ofproto *ofproto = rule->ofproto; struct oftable *table = &ofproto->tables[rule->table_id]; - struct rule_actions *actions; + const struct rule_actions *actions; bool may_expire; ovs_mutex_lock(&rule->mutex); diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c index 8be862686..91c7f28fe 100644 --- a/utilities/ovs-ofctl.c +++ b/utilities/ovs-ofctl.c @@ -1026,7 +1026,7 @@ ofctl_dump_flows(int argc, char *argv[]) ds_destroy(&s); for (i = 0; i < n_fses; i++) { - free(fses[i].ofpacts); + free(CONST_CAST(struct ofpact *, fses[i].ofpacts)); } free(fses); @@ -1136,7 +1136,7 @@ ofctl_flow_mod__(const char *remote, struct ofputil_flow_mod *fms, struct ofputil_flow_mod *fm = &fms[i]; transact_noreply(vconn, ofputil_encode_flow_mod(fm, protocol)); - free(fm->ofpacts); + free(CONST_CAST(struct ofpact *, fm->ofpacts)); } vconn_close(vconn); } @@ -2199,7 +2199,7 @@ static void fte_version_free(struct fte_version *version) { if (version) { - free(version->ofpacts); + free(CONST_CAST(struct ofpact *, version->ofpacts)); free(version); } } @@ -2739,7 +2739,7 @@ ofctl_parse_flows__(struct ofputil_flow_mod *fms, size_t n_fms, ofp_print(stdout, ofpbuf_data(msg), ofpbuf_size(msg), verbosity); ofpbuf_delete(msg); - free(fm->ofpacts); + free(CONST_CAST(struct ofpact *, fm->ofpacts)); } }