[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