[ovs-dev] [PATCH] ofproto: Inline actions in struct rule_actions.
Jarno Rajahalme
jrajahalme at nicira.com
Fri Apr 25 15:49:10 UTC 2014
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 throughout the code.
Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
lib/ofp-actions.c | 8 ++++----
lib/ofp-actions.h | 6 +++---
lib/ofp-parse.c | 2 +-
lib/ofp-util.c | 2 +-
lib/ofp-util.h | 10 +++++-----
ofproto/connmgr.c | 2 +-
ofproto/ofproto-dpif-xlate.c | 4 ++--
ofproto/ofproto-dpif.c | 4 ++--
ofproto/ofproto-dpif.h | 2 +-
ofproto/ofproto-provider.h | 27 ++++++++++++++-------------
ofproto/ofproto.c | 38 +++++++++++++++-----------------------
utilities/ovs-ofctl.c | 10 +++++-----
12 files changed, 54 insertions(+), 61 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index ce14004..9abb383 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1899,7 +1899,7 @@ inconsistent_match(enum ofputil_protocol *usable_protocols)
* Modifies some actions, filling in fields that could not be properly set
* without context. */
static enum ofperr
-ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
+ofpact_check__(enum ofputil_protocol *usable_protocols, const struct ofpact *a,
struct flow *flow, ofp_port_t max_ports,
uint8_t table_id, uint8_t n_tables)
{
@@ -2150,12 +2150,12 @@ ofpact_check__(enum ofputil_protocol *usable_protocols, struct ofpact *a,
*
* May temporarily modify 'flow', but restores the changes before returning. */
enum ofperr
-ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len,
+ofpacts_check(const struct ofpact ofpacts[], size_t ofpacts_len,
struct flow *flow, ofp_port_t max_ports,
uint8_t table_id, uint8_t n_tables,
enum ofputil_protocol *usable_protocols)
{
- struct ofpact *a;
+ const struct ofpact *a;
ovs_be16 dl_type = flow->dl_type;
ovs_be16 vlan_tci = flow->vlan_tci;
uint8_t nw_proto = flow->nw_proto;
@@ -2178,7 +2178,7 @@ ofpacts_check(struct ofpact ofpacts[], size_t ofpacts_len,
/* Like ofpacts_check(), but reports inconsistencies as
* OFPERR_OFPBAC_MATCH_INCONSISTENT rather than clearing bits. */
enum ofperr
-ofpacts_check_consistency(struct ofpact ofpacts[], size_t ofpacts_len,
+ofpacts_check_consistency(const struct ofpact ofpacts[], size_t ofpacts_len,
struct flow *flow, ofp_port_t max_ports,
uint8_t table_id, uint8_t n_tables,
enum ofputil_protocol usable_protocols)
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 89bf867..a0fd5c7 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.
@@ -592,11 +592,11 @@ enum ofperr ofpacts_pull_openflow_instructions(struct ofpbuf *openflow,
unsigned int instructions_len,
enum ofp_version version,
struct ofpbuf *ofpacts);
-enum ofperr ofpacts_check(struct ofpact[], size_t ofpacts_len,
+enum ofperr ofpacts_check(const struct ofpact[], size_t ofpacts_len,
struct flow *, ofp_port_t max_ports,
uint8_t table_id, uint8_t n_tables,
enum ofputil_protocol *usable_protocols);
-enum ofperr ofpacts_check_consistency(struct ofpact[], size_t ofpacts_len,
+enum ofperr ofpacts_check_consistency(const struct ofpact[], size_t ofpacts_len,
struct flow *, ofp_port_t max_ports,
uint8_t table_id, uint8_t n_tables,
enum ofputil_protocol usable_protocols);
diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
index d250042..c759f03 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.c b/lib/ofp-util.c
index 3484394..4473e3c 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -6208,7 +6208,7 @@ ofputil_bucket_list_destroy(struct list *buckets)
LIST_FOR_EACH_SAFE (bucket, next_bucket, list_node, buckets) {
list_remove(&bucket->list_node);
- free(bucket->ofpacts);
+ free(CONST_CAST(struct ofpact *, bucket->ofpacts));
free(bucket);
}
}
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 245cc4e..9594bbb 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -298,7 +298,7 @@ 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. */
+ const struct ofpact *ofpacts; /* Series of "struct ofpact"s. */
size_t ofpacts_len; /* Length of ofpacts, in bytes. */
};
@@ -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;
};
@@ -440,7 +440,7 @@ struct ofputil_packet_out {
size_t packet_len; /* Length of packet data in bytes. */
uint32_t buffer_id; /* Buffer id or UINT32_MAX if no buffer. */
ofp_port_t in_port; /* Packet's input port. */
- struct ofpact *ofpacts; /* Actions. */
+ const struct ofpact *ofpacts; /* Actions. */
size_t ofpacts_len; /* Size of ofpacts in bytes. */
};
@@ -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. */
@@ -1050,7 +1050,7 @@ struct ofputil_bucket {
uint32_t watch_group; /* Group whose state affects whether this
* bucket is live. Only required for fast
* failover groups. */
- struct ofpact *ofpacts; /* Series of "struct ofpact"s. */
+ const struct ofpact *ofpacts; /* Series of "struct ofpact"s. */
size_t ofpacts_len; /* Length of ofpacts, in bytes. */
};
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 383fbda..9a5167d 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 4a9ba52..e2ac9ad 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 30cbb24..ad5cabe 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3192,7 +3192,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);
@@ -3854,7 +3854,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 8af6645..94d4e1e 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 11dcd82..9431be8 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 49444c1..6195b75 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 8be8626..2a9bf4b 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);
}
@@ -2190,7 +2190,7 @@ struct fte_version {
uint16_t idle_timeout;
uint16_t hard_timeout;
uint16_t flags;
- struct ofpact *ofpacts;
+ const struct ofpact *ofpacts;
size_t ofpacts_len;
};
@@ -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));
}
}
--
1.7.10.4
More information about the dev
mailing list