[ovs-dev] [megaflow bond rebase] ofproto/bond: Implement bond megaflow using recirculation
Andy Zhou
azhou at nicira.com
Fri Apr 4 20:13:18 UTC 2014
Thanks for the review. I will send out V5 soon.
On Wed, Apr 2, 2014 at 4:28 PM, Ben Pfaff <blp at nicira.com> wrote:
> 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;
My intention was to test recirc_id == 0 and recir_id mask is all 1.
How about the following?
static bool
match_has_default_recirc_id(const struct match *m)
{
return ((m->flow.recirc_id | ~m->wc.masks.recirc_id) == 0);
}
>
> I have some stylistic suggestions, see below:
Thanks for the improvements. I'll try to catch them in future patches.
>
> 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