[ovs-dev] [megaflow bond rebase] ofproto/bond: Implement bond megaflow using recirculation
Ben Pfaff
blp at nicira.com
Wed Apr 2 23:28:15 UTC 2014
On Tue, Apr 01, 2014 at 03:22:54PM -0700, Andy Zhou wrote:
> Infrastructure to enable megaflow support for bond ports using
> recirculation. This patch adds the following features:
> * Generate RECIRC action when bond can benefit from recirculation.
> * Populate post recirculation rules in a hidden table. Currently table 254.
> * Uses post recirculation rules for bond rebalancing
> * A recirculation implementation in dpif-netdev.
I would add to the commit message why we want to do this (so that we
can megaflow bond outputs and thus greatly improve performance) and
that this commit doesn't actually improve the megaflow generation (and
that that is left for a later commit).
match_has_default_recirc_id() seems to be implemented as essentially:
return (a | (a & b)) == 0;
and I don't understand that, since it seems to be equivalent to just
return a == 0;
I have some stylistic suggestions, see below:
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a630a63..9ceb2d8 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2160,7 +2160,7 @@ dp_execute_cb(void *aux_, struct ofpbuf *packet,
md->recirc_id = old_dp_hash;
break;
} else {
- VLOG_WARN("Packet dropped. Max recirculation dpeth exceeded.");
+ VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
}
break;
diff --git a/lib/match.c b/lib/match.c
index ec97b8f..1fd083f 100644
--- a/lib/match.c
+++ b/lib/match.c
@@ -802,12 +802,11 @@ match_has_default_dp_hash(const struct match *m)
}
/* Return true if the hidden fields of the match are set to the default values.
- * The default values equals to those set up by match_init_hidden_fiels(). */
+ * The default values equals to those set up by match_init_hidden_fields(). */
bool
match_has_default_hidden_fields(const struct match *m)
{
- return match_has_default_recirc_id(m)
- && match_has_default_dp_hash(m);
+ return match_has_default_recirc_id(m) && match_has_default_dp_hash(m);
}
void
diff --git a/lib/odp-execute.c b/lib/odp-execute.c
index c48101d..a431b5d 100644
--- a/lib/odp-execute.c
+++ b/lib/odp-execute.c
@@ -210,8 +210,8 @@ odp_execute_actions__(void *dp, struct ofpbuf *packet, bool steal,
/* Allow 'dp_execute_action' to steal the packet data if we do
* not need it any more. */
bool may_steal = steal && (!more_actions
- && left <= NLA_ALIGN(a->nla_len)
- && type != OVS_ACTION_ATTR_RECIRC);
+ && left <= NLA_ALIGN(a->nla_len)
+ && type != OVS_ACTION_ATTR_RECIRC);
dp_execute_action(dp, packet, md, a, may_steal);
}
break;
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index a944558..245cc4e 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -247,8 +247,8 @@ enum ofputil_flow_mod_flags {
OFPUTIL_FF_EMERG = 1 << 4, /* OpenFlow 1.0 only. */
OFPUTIL_FF_RESET_COUNTS = 1 << 5, /* OpenFlow 1.2+. */
- /* Flags that are only set by OVS for its internal use. Open flow
- * controller will Never set them. */
+ /* Flags that are only set by OVS for its internal use. Cannot be set via
+ * OpenFlow. */
OFPUTIL_FF_HIDDEN_FIELDS = 1 << 6, /* Allow hidden match fields to be
set or modified. */
OFPUTIL_FF_NO_READONLY = 1 << 7, /* Allow rules within read only tables
diff --git a/ofproto/bond.c b/ofproto/bond.c
index 92a3109..49dd49e 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -360,8 +360,7 @@ update_recirc_rules(struct bond *bond)
char *err_s = match_to_string(&pr_op->match,
RECIRC_RULE_PRIORITY);
- VLOG_ERR("Bond: Failed to add post recirculation flow %s",
- err_s);
+ VLOG_ERR("failed to add post recirculation flow %s", err_s);
free(err_s);
pr_op->pr_rule = NULL;
} else {
@@ -377,8 +376,7 @@ update_recirc_rules(struct bond *bond)
char *err_s = match_to_string(&pr_op->match,
RECIRC_RULE_PRIORITY);
- VLOG_ERR("Bond: Failed to remove post recirculation flow %s",
- err_s);
+ VLOG_ERR("failed to remove post recirculation flow %s", err_s);
free(err_s);
}
@@ -855,7 +853,7 @@ bond_choose_output_slave(struct bond *bond, const struct flow *flow,
/* Recirculation. */
static void
-bond_entry_acount(struct bond_entry *entry, uint64_t rule_tx_bytes)
+bond_entry_account(struct bond_entry *entry, uint64_t rule_tx_bytes)
OVS_REQ_RDLOCK(rwlock)
{
if (entry->slave) {
@@ -876,9 +874,8 @@ bond_recirculation_account(struct bond *bond)
ovs_rwlock_rdlock(&rwlock);
for (i=0; i<=BOND_MASK; i++) {
struct bond_entry *entry = &bond->hash[i];
- struct rule *rule;
+ struct rule *rule = entry->pr_rule;
- rule = entry->pr_rule;
if (rule) {
uint64_t n_packets OVS_UNUSED;
long long int used OVS_UNUSED;
@@ -886,7 +883,7 @@ bond_recirculation_account(struct bond *bond)
rule->ofproto->ofproto_class->rule_get_stats(
rule, &n_packets, &n_bytes, &used);
- bond_entry_acount(entry, n_bytes);
+ bond_entry_account(entry, n_bytes);
}
}
ovs_rwlock_unlock(&rwlock);
@@ -896,8 +893,6 @@ bool
bond_may_recirc(const struct bond *bond, uint32_t *recirc_id,
uint32_t *hash_bias)
{
- bool rv = false;
-
if (bond->balance == BM_TCP) {
if (recirc_id) {
*recirc_id = bond->recirc_id;
@@ -905,17 +900,17 @@ bond_may_recirc(const struct bond *bond, uint32_t *recirc_id,
if (hash_bias) {
*hash_bias = bond->basis;
}
- rv = true;
+ return true;
+ } else {
+ return false;
}
-
- return rv;
}
void
bond_update_post_recirc_rules(struct bond* bond, const bool force)
{
struct bond_entry *e;
- bool update_rules = force; /* Always update rules if caller forces it.*/
+ bool update_rules = force; /* Always update rules if caller forces it. */
/* Make sure all bond entries are populated */
for (e = bond->hash; e <= &bond->hash[BOND_MASK]; e++) {
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7805f19..1e9eeef 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2118,7 +2118,7 @@ xlate_ofpact_resubmit(struct xlate_ctx *ctx,
if (ctx->rule && rule_dpif_is_internal(ctx->rule)) {
/* Still allow missed packets to be sent to the controller
- * if Resubmiting from an internal table */
+ * if resubmitting from an internal table. */
may_packet_in = true;
honor_table_miss = true;
}
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index 966eec1..8b53e10 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -33,9 +33,9 @@ struct dpif_sflow;
struct mac_learning;
struct xlate_recirc {
- uint32_t recirc_id; /* !0 Use recirculation instead of output.*/
- uint8_t hash_alg; /* !0 Compute hash for recirc before.*/
- uint32_t hash_bias; /* Compute hash for recirc before.*/
+ uint32_t recirc_id; /* !0 Use recirculation instead of output. */
+ uint8_t hash_alg; /* !0 Compute hash for recirc before. */
+ uint32_t hash_bias; /* Compute hash for recirc before. */
};
struct xlate_out {
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index daf7488..6a2a752 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -918,14 +918,13 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp)
return error;
}
-/* Tests whether 'backer''s datapath supports recirculation
- * Only newer datapath supports OVS_KEY_ATTR in OVS_ACTION_ATTR_USERSPACE actions. We need
- * to disable some features on older datapaths that don't support this
- * feature.
+/* Tests whether 'backer''s datapath supports recirculation Only newer datapath
+ * supports OVS_KEY_ATTR in OVS_ACTION_ATTR_USERSPACE actions. We need to
+ * disable some features on older datapaths that don't support this feature.
*
- * Returns false if 'backer' definitely does not support variable-length
- * userdata, true if it seems to support them or if at least the error we get
- * is ambiguous. */
+ * Returns false if 'backer' definitely does not support recirculation, true if
+ * it seems to support recirculation or if at least the error we get is
+ * ambiguous. */
static bool
check_recirc(struct dpif_backer *backer)
{
@@ -4775,7 +4774,7 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
int
ofproto_dpif_delete_internal_flow(struct ofproto_dpif *ofproto,
- struct match *match, int priority)
+ struct match *match, int priority)
{
struct ofputil_flow_mod fm;
int error;
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 99b9da1..6785077 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -202,10 +202,11 @@ struct ofport_dpif *odp_port_to_ofport(const struct dpif_backer *, odp_port_t);
uint32_t ofproto_dpif_alloc_recirc_id(struct ofproto_dpif *ofproto);
void ofproto_dpif_free_recirc_id(struct ofproto_dpif *ofproto, uint32_t recirc_id);
-int ofproto_dpif_add_internal_flow(struct ofproto_dpif *ofproto,
- struct match *match, int proiroty,
+int ofproto_dpif_add_internal_flow(struct ofproto_dpif *,
+ struct match *, int priority,
const struct ofpbuf *ofpacts,
struct rule **rulep);
-int ofproto_dpif_delete_internal_flow(struct ofproto_dpif *ofproto, struct match *match,
- int proiroty);
+int ofproto_dpif_delete_internal_flow(struct ofproto_dpif *, struct match *,
+ int priority);
+
#endif /* ofproto-dpif.h */
More information about the dev
mailing list