[ovs-dev] [PATCH v9] ofproto-dpif: APIs and CLI option to add/delete static fdb entry

Eelco Chaudron echaudro at redhat.com
Fri Jun 25 12:55:02 UTC 2021


This one looks good, however, I would make some small changes.
If you do, you can add my acked by to the new revision.

//Eelco


On 24 Jun 2021, at 17:57, Vasu Dasari wrote:

> Currently there is an option to add/flush/show ARP/ND neighbor. This 
> covers L3
> side.  For L2 side, there is only fdb show command. This patch gives 
> an option
> to add/del an fdb entry via ovs-appctl.
>
> CLI command looks like:
>
> To add:
>     ovs-appctl fdb/add <bridge> <port> <vlan> <Mac>
>     ovs-appctl fdb/add br0 p1 0 50:54:00:00:00:05
>
> To del:
>     ovs-appctl fdb/del <bridge> <vlan> <Mac>
>     ovs-appctl fdb/del br0 0 50:54:00:00:00:05
>
> Added two new APIs to provide convenient interface to add and delete 
> static-macs.
> bool xlate_add_static_mac_entry(const struct ofproto_dpif *, 
> ofp_port_t in_port,
>                                struct eth_addr dl_src, int vlan);
> bool xlate_delete_static_mac_entry(const struct ofproto_dpif *,
>                                   struct eth_addr dl_src, int vlan);
>
> 1. Static entry should not age. To indicate that entry being 
> programmed is a static entry,
>    'expires' field in 'struct mac_entry' will be set to a 
> MAC_ENTRY_AGE_STATIC_ENTRY. A
>    check for this value is made while deleting mac entry as part of 
> regular aging process.
> 2. Another change to of mac-update logic, when a packet with same 
> dl_src as that of a
>    static-mac entry arrives on any port, the logic will not modify the 
> expires field.
> 3. While flushing fdb entries, made sure static ones are not evicted.
> 4. Updated "ovs-appctl fdb/stats-show br0" to display numberof static 
> entries in switch
>
> Added following tests:
>   ofproto-dpif - static-mac add/del/flush
>   ofproto-dpif - static-mac mac moves
>
> Signed-off-by: Vasu Dasari <vdasari at gmail.com>
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048894.html
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1597752
> Tested-by: Eelco Chaudron <echaudro at redhat.com>
> ---
> v1:
>  - Fixed 0-day robot warnings
> v2:
>  - Fix valgrind error in the modified code in mac_learning_insert() 
> where a read is
>    is performed on e->expires which is not initialized
> v3:
>  - Addressed code review comments
>  - Added more documentation
>  - Fixed mac_entry_age() and is_mac_learning_update_needed() to have 
> common
>    understanding of return values when mac_entry is a static one.
>  - Added NEWS item
> v4:
>  - Addressed code review comments
>  - Static entries will not be purged when fdb/flush is performed.
>  - Static entries will not be overwritten when packet with same dl_src 
> arrives on
>    any port of switch
>  - Provided bit more detail while doing fdb/add, to indicate if 
> static-mac is
>    overriding already present entry
>  - Separated test cases for a bit more clarity
>  v5:
>  - Addressed code review comments
>  - Added new total_static counter to count number of static entries.
>  - Removed mac_entry_set_idle_time()
>  - Added mac_learning_add_static_entry() and 
> mac_learning_del_static_entry()
>  - Modified APIs xlate_add_static_mac_entry() and 
> xlate_delete_static_mac_entry()
>    return 0 on success else a failure code
>  v6:
>  - Fixed a probable bug with Eelco's code review comments in
>    is_mac_learning_update_needed()
>  v7:
>  - Added a ovs-vswitchd.8 man page entry for fdb add/del commands
>  v8:
>  - Updaed with code review comments from Eelco.
>  - Renamed total_static to static_entries
>  - Added coverage counter mac_learning_static_none_move
>  - Fixed a possible bug with static_entries getting cleared via
>    fdb/stats-clear command
>  - Initialize static_entries in mac_learning_create()
>  - Modified fdb/del command by removing option to specify port-name
>  - Breakup ofproto_unixctl_fdb_update into ofproto_unixctl_fdb_add
>    and ofproto_unixctl_fdb_delete
>  - Updated test "static-mac add/del/flush" to have interleaved mac
>    entries before fdb/flush
>  - Updated test "static-mac mac move" to check for newly added
>    coverage counter mac_learning_static_none_move
> v9:
>  - Updated source code comments and addressed code review comments
> ---
>  NEWS                         |   4 +
>  lib/mac-learning.c           | 155 
> +++++++++++++++++++++++++++++++----
>  lib/mac-learning.h           |  17 ++++
>  ofproto/ofproto-dpif-xlate.c |  48 +++++++++--
>  ofproto/ofproto-dpif-xlate.h |   5 ++
>  ofproto/ofproto-dpif.c       | 117 +++++++++++++++++++++++++-
>  tests/ofproto-dpif.at        |  99 ++++++++++++++++++++++
>  vswitchd/ovs-vswitchd.8.in   |   5 ++
>  8 files changed, 421 insertions(+), 29 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index db3faf4cc..12fb40054 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -20,6 +20,10 @@ Post-v2.15.0
>     - ovsdb-tool:
>       * New option '--election-timer' to the 'create-cluster' command 
> to set the
>         leader election timer during cluster creation.
> +   - ovs-appctl:
> +     * Added ability to add and delete static mac entries using:
> +       'ovs-appctl fdb/add <bridge> <port> <vlan> <mac>'
> +       'ovs-appctl fdb/del <bridge> <vlan> <mac>'
>
>
>  v2.15.0 - 15 Feb 2021
> diff --git a/lib/mac-learning.c b/lib/mac-learning.c
> index 3d5293d3b..dd3f46a8b 100644
> --- a/lib/mac-learning.c
> +++ b/lib/mac-learning.c
> @@ -34,13 +34,25 @@ COVERAGE_DEFINE(mac_learning_learned);
>  COVERAGE_DEFINE(mac_learning_expired);
>  COVERAGE_DEFINE(mac_learning_evicted);
>  COVERAGE_DEFINE(mac_learning_moved);
> +COVERAGE_DEFINE(mac_learning_static_none_move);
>
> -/* Returns the number of seconds since 'e' (within 'ml') was last 
> learned. */
> +/*
> + * This function will return age of mac entry in the fdb.
> + * It will return either one of the following:
> + *  1. Number of seconds since 'e' (within 'ml') was last learned.
> + *  2. If the mac entry is a static entry, it returns
> + *     MAC_ENTRY_AGE_STATIC_ENTRY.
> + */
>  int
>  mac_entry_age(const struct mac_learning *ml, const struct mac_entry 
> *e)
>  {
> -    time_t remaining = e->expires - time_now();
> -    return ml->idle_time - remaining;
> +    /* For static fdb entries, expires would be 
> MAC_ENTRY_AGE_STATIC_ENTRY. */
> +    if (MAC_ENTRY_AGE_STATIC_ENTRY == e->expires) {
> +        return MAC_ENTRY_AGE_STATIC_ENTRY;
> +    } else {
> +        time_t remaining = e->expires - time_now();
> +        return ml->idle_time - remaining;
> +    }
>  }
>
>  static uint32_t
> @@ -214,6 +226,7 @@ mac_learning_create(unsigned int idle_time)
>      ovs_refcount_init(&ml->ref_cnt);
>      ovs_rwlock_init(&ml->rwlock);
>      mac_learning_clear_statistics(ml);
> +    ml->static_entries = 0;
>      return ml;
>  }
>
> @@ -309,16 +322,19 @@ mac_learning_may_learn(const struct mac_learning 
> *ml,
>  }
>
>  /* Searches 'ml' for and returns a MAC learning entry for 'src_mac' 
> in 'vlan',
> - * inserting a new entry if necessary.  The caller must have already 
> verified,
> - * by calling mac_learning_may_learn(), that 'src_mac' and 'vlan' are
> - * learnable.
> + * inserting a new entry if necessary.  If entry being added is a
> + *  1. cache entry: caller must have already verified, by calling
> + *     mac_learning_may_learn(), that 'src_mac' and 'vlan' are 
> learnable.
> + *  2. static entry: new mac static fdb entry will be created or if 
> one
> + *     exists already, converts that entry to a static fdb type.
>   *
>   * If the returned MAC entry is new (that is, if it has a NULL 
> client-provided
>   * port, as returned by mac_entry_get_port()), then the caller must 
> initialize
>   * the new entry's port to a nonnull value with mac_entry_set_port(). 
> */
> -struct mac_entry *
> -mac_learning_insert(struct mac_learning *ml,
> -                    const struct eth_addr src_mac, uint16_t vlan)
> +static struct mac_entry *
> +mac_learning_insert__(struct mac_learning *ml, const struct eth_addr 
> src_mac,
> +                      uint16_t vlan, bool is_static)
> +    OVS_REQ_WRLOCK(ml->rwlock)
>  {
>      struct mac_entry *e;
>
> @@ -336,8 +352,11 @@ mac_learning_insert(struct mac_learning *ml,
>          e->vlan = vlan;
>          e->grat_arp_lock = TIME_MIN;
>          e->mlport = NULL;
> -        COVERAGE_INC(mac_learning_learned);
> -        ml->total_learned++;
> +        e->expires = 0;
> +        if (!is_static) {
> +            COVERAGE_INC(mac_learning_learned);
> +            ml->total_learned++;
> +        }
>      } else {
>          ovs_list_remove(&e->lru_node);
>      }
> @@ -348,11 +367,78 @@ mac_learning_insert(struct mac_learning *ml,
>          ovs_list_remove(&e->port_lru_node);
>          ovs_list_push_back(&e->mlport->port_lrus, &e->port_lru_node);
>      }
> -    e->expires = time_now() + ml->idle_time;
> +
> +    /* Update 'expires' for mac entry. */
> +    if (is_static) {
> +        /* Increment static_entries only if entry is a new one or 
> entry is
> +         * converted from cache to static type. */
> +        if (e->expires != MAC_ENTRY_AGE_STATIC_ENTRY) {
> +            ml->static_entries++;
> +        }
> +        e->expires = MAC_ENTRY_AGE_STATIC_ENTRY;
> +    } else {
> +        e->expires = time_now() + ml->idle_time;
> +    }
>
>      return e;
>  }
>
> +/* Adds a new dynamic mac entry to fdb. */
> +struct mac_entry *
> +mac_learning_insert(struct mac_learning *ml,
> +                    const struct eth_addr src_mac, uint16_t vlan)
> +{
> +    return mac_learning_insert__(ml, src_mac, vlan, false);
> +}
> +
> +/* Adds a new static mac entry to fdb. It returns pointer to 
> mac_entry
> + * pointing to the fdb entry.
> + *
> + * Returns 'true' if  mac entry is inserted, 'false' otherwise.
> + */
> +bool
> +mac_learning_add_static_entry(struct mac_learning *ml,
> +                              const struct eth_addr src_mac, uint16_t 
> vlan,
> +                              void *in_port)
> +    OVS_EXCLUDED(ml->rwlock)
> +{
> +    struct mac_entry *mac = NULL;
> +    bool inserted = false;
> +
> +    ovs_rwlock_wrlock(&ml->rwlock);
> +    mac = mac_learning_insert__(ml, src_mac, vlan, true);
> +    if (mac) {
> +        mac_entry_set_port(ml, mac, in_port);
> +        inserted = true;
> +    }
> +    ovs_rwlock_unlock(&ml->rwlock);
> +
> +    return inserted;
> +}
> +
> +/* Delete a static mac entry from fdb if it exists.
> + *
> + * Returns 'true' if  mac entry is found, 'false' otherwise.
> + */
> +bool
> +mac_learning_del_static_entry(struct mac_learning *ml,
> +                              const struct eth_addr dl_src, uint16_t 
> vlan)
> +{
> +    struct mac_entry *mac = NULL;
> +    bool deleted = false;
> +
> +    ovs_rwlock_wrlock(&ml->rwlock);
> +    mac = mac_learning_lookup(ml, dl_src, vlan);
> +    if (mac && mac_entry_age(ml, mac) == MAC_ENTRY_AGE_STATIC_ENTRY) 
> {
> +        mac_learning_expire(ml, mac);
> +        ml->static_entries--;
> +        deleted = true;
> +    }
> +    ovs_rwlock_unlock(&ml->rwlock);
> +
> +    return deleted;
> +}
> +
>  /* Checks whether a MAC learning update is necessary for MAC learning 
> table
>   * 'ml' given that a packet matching 'src' was received on 'in_port' 
> in 'vlan',
>   * and given that the packet was gratuitous ARP if 
> 'is_gratuitous_arp' is
> @@ -372,13 +458,32 @@ is_mac_learning_update_needed(const struct 
> mac_learning *ml,
>      OVS_REQ_RDLOCK(ml->rwlock)
>  {
>      struct mac_entry *mac;
> +    int age;
>
>      if (!mac_learning_may_learn(ml, src, vlan)) {
>          return false;
>      }
>
>      mac = mac_learning_lookup(ml, src, vlan);
> -    if (!mac || mac_entry_age(ml, mac)) {
> +    /* If mac entry is missing it needs to be added to fdb. */
> +    if (!mac) {
> +        return true;
> +    }
> +
> +    age = mac_entry_age(ml, mac);
> +    /* if mac is a static entry, then there is no need to update. */
> +    if (age == MAC_ENTRY_AGE_STATIC_ENTRY) {
> +        /* coverage counter to increment when a packet with same
> +         * static-mac appears on a different port. */
> +        if (mac_entry_get_port(ml, mac) != in_port) {
> +            COVERAGE_INC(mac_learning_static_none_move);
> +        }
> +        return false;
> +    }
> +
> +    /* If entry is still alive, just update the mac_entry so, that 
> expires
> +     * gets updated. */
> +    if (age > 0) {
>          return true;
>      }
>
> @@ -513,13 +618,29 @@ mac_learning_expire(struct mac_learning *ml, 
> struct mac_entry *e)
>      free(e);
>  }
>
> -/* Expires all the mac-learning entries in 'ml'. */
> +/* Expires all the dynamic mac-learning entries in 'ml'. */
>  void
>  mac_learning_flush(struct mac_learning *ml)
>  {
> -    struct mac_entry *e;
> -    while (get_lru(ml, &e)){
> -        mac_learning_expire(ml, e);
> +    struct mac_entry *e, *first_static_mac = NULL;
> +
> +    while (get_lru(ml, &e) && (e != first_static_mac)) {
> +
> +        /* static-mac should not be evicted. */
> +        if (MAC_ENTRY_AGE_STATIC_ENTRY == e->expires) {
> +
> +            /* Make note of first static-mac encountered, so that 
> this while
> +             * loop will break on visting this mac again via 
> get_lru(). */
> +            if (!first_static_mac) {
> +                first_static_mac = e;
> +            }
> +
> +            /* Remove from lru head and append it to tail. */
> +            ovs_list_remove(&e->lru_node);
> +            ovs_list_push_back(&ml->lrus, &e->lru_node);
> +        } else {
> +            mac_learning_expire(ml, e);
> +        }
>      }
>      hmap_shrink(&ml->table);
>  }
> diff --git a/lib/mac-learning.h b/lib/mac-learning.h
> index 0ddab06cb..270fbd70d 100644
> --- a/lib/mac-learning.h
> +++ b/lib/mac-learning.h
> @@ -57,6 +57,11 @@
>   * list starting from the LRU end, deleting each entry that has been 
> idle too
>   * long.
>   *
> + * Fourth, a mac entry can be configured statically via API or appctl 
> commands.
> + * Static entries are programmed to have an age of 
> MAC_ENTRY_AGE_STATIC_ENTRY.
> + * Age of static entries will not be updated by a receiving packet as 
> part of
> + * regular packet processing.
> + *
>   * Finally, the number of MAC learning table entries has a 
> configurable maximum
>   * size to prevent memory exhaustion.  When a new entry must be 
> inserted but
>   * the table is already full, the implementation uses an eviction 
> strategy
> @@ -94,6 +99,9 @@ struct mac_learning;
>  /* Time, in seconds, before expiring a mac_entry due to inactivity. 
> */
>  #define MAC_ENTRY_DEFAULT_IDLE_TIME 300
>
> +/* Age value to represent a static entry. */
> +#define MAC_ENTRY_AGE_STATIC_ENTRY INT_MAX
> +
>  /* Time, in seconds, to lock an entry updated by a gratuitous ARP to 
> avoid
>   * relearning based on a reflection from a bond member. */
>  #define MAC_GRAT_ARP_LOCK_TIME 5
> @@ -156,6 +164,7 @@ struct mac_learning {
>      unsigned long *flood_vlans; /* Bitmap of learning disabled VLANs. 
> */
>      unsigned int idle_time;     /* Max age before deleting an entry. 
> */
>      size_t max_entries;         /* Max number of learned MACs. */
> +    size_t static_entries;      /* Current number of static MAC 
> entries. */
>      struct ovs_refcount ref_cnt;
>      struct ovs_rwlock rwlock;
>      bool need_revalidate;
> @@ -218,6 +227,14 @@ bool mac_learning_update(struct mac_learning *ml, 
> struct eth_addr src,
>                           int vlan, bool is_gratuitous_arp, bool 
> is_bond,
>                           void *in_port)
>      OVS_EXCLUDED(ml->rwlock);
> +bool mac_learning_add_static_entry(struct mac_learning *ml,
> +                                   const struct eth_addr src,
> +                                   uint16_t vlan, void *in_port)
> +    OVS_EXCLUDED(ml->rwlock);
> +bool mac_learning_del_static_entry(struct mac_learning *ml,
> +                                   const struct eth_addr src,
> +                                   uint16_t vlan)
> +    OVS_EXCLUDED(ml->rwlock);
>
>  /* Lookup. */
>  struct mac_entry *mac_learning_lookup(const struct mac_learning *ml,
> diff --git a/ofproto/ofproto-dpif-xlate.c 
> b/ofproto/ofproto-dpif-xlate.c
> index a6f4ea334..c65723283 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7991,26 +7991,58 @@ xlate_send_packet(const struct ofport_dpif 
> *ofport, bool oam,
>                                          ofpacts.data, ofpacts.size, 
> packet);
>  }
>
> -void
> -xlate_mac_learning_update(const struct ofproto_dpif *ofproto,
> -                          ofp_port_t in_port, struct eth_addr dl_src,
> -                          int vlan, bool is_grat_arp)
> +/* Get xbundle for a ofp_port in a ofproto datapath. */
> +static struct xbundle*
> +ofp_port_to_xbundle(const struct ofproto_dpif *ofproto, ofp_port_t 
> ofp_port)
>  {
>      struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp);
>      struct xbridge *xbridge;
> -    struct xbundle *xbundle;
>
>      xbridge = xbridge_lookup(xcfg, ofproto);
>      if (!xbridge) {
> -        return;
> +        return NULL;
>      }
>
> -    xbundle = lookup_input_bundle__(xbridge, in_port, NULL);
> +    return lookup_input_bundle__(xbridge, ofp_port, NULL);
> +}
> +
> +void
> +xlate_mac_learning_update(const struct ofproto_dpif *ofproto,
> +                          ofp_port_t in_port, struct eth_addr dl_src,
> +                          int vlan, bool is_grat_arp)
> +{
> +    struct xbundle *xbundle = NULL;
> +
> +    xbundle = ofp_port_to_xbundle(ofproto, in_port);
>      if (!xbundle) {
>          return;
>      }
>
> -    update_learning_table__(xbridge, xbundle, dl_src, vlan, 
> is_grat_arp);
> +    update_learning_table__(xbundle->xbridge,
> +                            xbundle, dl_src, vlan, is_grat_arp);
> +}
> +
> +bool
> +xlate_add_static_mac_entry(const struct ofproto_dpif *ofproto,
> +                           ofp_port_t in_port,
> +                           struct eth_addr dl_src, int vlan)
> +{
> +    struct xbundle *xbundle = ofp_port_to_xbundle(ofproto, in_port);
> +
> +    /* Return here if xbundle is NULL. */
> +    if (!xbundle || (xbundle == &ofpp_none_bundle)) {
> +        return false;
> +    }
> +
> +    return mac_learning_add_static_entry(ofproto->ml, dl_src, vlan,
> +                                         xbundle->ofbundle);
> +}
> +
> +bool
> +xlate_delete_static_mac_entry(const struct ofproto_dpif *ofproto,
> +                              struct eth_addr dl_src, int vlan)
> +{
> +    return mac_learning_del_static_entry(ofproto->ml, dl_src, vlan);
>  }
>
>  void
> diff --git a/ofproto/ofproto-dpif-xlate.h 
> b/ofproto/ofproto-dpif-xlate.h
> index 3426a27b2..851088d79 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -225,6 +225,11 @@ int xlate_send_packet(const struct ofport_dpif *, 
> bool oam, struct dp_packet *);
>  void xlate_mac_learning_update(const struct ofproto_dpif *ofproto,
>                                 ofp_port_t in_port, struct eth_addr 
> dl_src,
>                                 int vlan, bool is_grat_arp);
> +bool xlate_add_static_mac_entry(const struct ofproto_dpif *,
> +                                ofp_port_t in_port,
> +                                struct eth_addr dl_src, int vlan);
> +bool xlate_delete_static_mac_entry(const struct ofproto_dpif *,
> +                                   struct eth_addr dl_src, int vlan);
>
>  void xlate_set_support(const struct ofproto_dpif *,
>                         const struct dpif_backer_support *);
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 47db9bb57..c2352b318 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5854,18 +5854,120 @@ ofproto_unixctl_fdb_show(struct unixctl_conn 
> *conn, int argc OVS_UNUSED,
>      LIST_FOR_EACH (e, lru_node, &ofproto->ml->lrus) {
>          struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, e);
>          char name[OFP_MAX_PORT_NAME_LEN];
> +        int age = mac_entry_age(ofproto->ml, e);
>
>          ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port,
> -                               NULL, name, sizeof name);
> -        ds_put_format(&ds, "%5s  %4d  "ETH_ADDR_FMT"  %3d\n",
> -                      name, e->vlan, ETH_ADDR_ARGS(e->mac),
> -                      mac_entry_age(ofproto->ml, e));
> +                NULL, name, sizeof name);
> +        ds_put_format(&ds, "%5s  %4d  "ETH_ADDR_FMT"  ",
> +                name, e->vlan, ETH_ADDR_ARGS(e->mac));
> +        if (MAC_ENTRY_AGE_STATIC_ENTRY == age) {
> +            ds_put_format(&ds, "static\n");
> +        } else {
> +            ds_put_format(&ds, "%3d\n", age);
> +        }
>      }
>      ovs_rwlock_unlock(&ofproto->ml->rwlock);
>      unixctl_command_reply(conn, ds_cstr(&ds));
>      ds_destroy(&ds);
>  }
>
> +static void
> +ofproto_unixctl_fdb_add(struct unixctl_conn *conn, int argc 
> OVS_UNUSED,
> +                         const char *argv[], void *aux OVS_UNUSED)
> +{
> +    const char *br_name = argv[1];
> +    const char *port_name = argv[2];
> +    struct eth_addr mac;
> +    uint16_t vlan = atoi(argv[3]);
> +    const struct ofproto_dpif *ofproto;
> +    ofp_port_t    in_port = OFPP_NONE;
> +    struct ofproto_port ofproto_port;
> +    struct ds ds = DS_EMPTY_INITIALIZER;
> +    const struct mac_entry *mac_entry;
> +    const struct ofbundle *bundle = NULL;
> +    int age;
> +
> +    ofproto = ofproto_dpif_lookup_by_name(br_name);
> +    if (!ofproto) {
> +        unixctl_command_reply_error(conn, "no such bridge");
> +        return;
> +    }
> +
> +    if (!eth_addr_from_string(argv[4], &mac)) {
> +        unixctl_command_reply_error(conn, "bad MAC address");
> +        return;
> +    }
> +
> +    if (ofproto_port_query_by_name(&ofproto->up, port_name, 
> &ofproto_port)) {
> +        unixctl_command_reply_error(conn,
> +                "software error, odp port is present but no ofp 
> port");
> +        return;
> +    }
> +    in_port = ofproto_port.ofp_port;
> +    ofproto_port_destroy(&ofproto_port);
> +
> +    /* Give a bit more information if the entry being added is 
> overriding
> +     * an existing entry. */
> +
> +    ovs_rwlock_rdlock(&ofproto->ml->rwlock);
> +    mac_entry = mac_learning_lookup(ofproto->ml, mac, vlan);
> +    if (mac_entry) {
> +        bundle = mac_entry_get_port(ofproto->ml, mac_entry);
> +        age = mac_entry->expires;
> +    }
> +    ovs_rwlock_unlock(&ofproto->ml->rwlock);
> +
> +    if (bundle && ((strcmp(bundle->name, port_name)) ||
> +                   (age != MAC_ENTRY_AGE_STATIC_ENTRY))) {
> +        char old_port_name[OFP_MAX_PORT_NAME_LEN];
> +
> +        
> ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port,
> +                NULL, old_port_name, sizeof old_port_name);
> +        ds_put_format(&ds, "Overriding already existing %s entry on 
> %s\n",
> +                (age == MAC_ENTRY_AGE_STATIC_ENTRY) ? "static" : 
> "dynamic",
> +                old_port_name);
> +    }
> +
> +    if (!xlate_add_static_mac_entry(ofproto, in_port, mac, vlan)) {
> +        ds_put_format(&ds, "could not add static mac entry\n");
> +        unixctl_command_reply_error(conn, ds_cstr(&ds));

Think on error you do not need the potential warning, just the error 
message, so it could become:


unixctl_command_reply_error(conn, "could not add static mac entry\n");

> +    } else {
> +        unixctl_command_reply(conn, ds_cstr(&ds));
> +    }
> +
> +    ds_destroy(&ds);
> +}
> +
> +static void
> +ofproto_unixctl_fdb_delete(struct unixctl_conn *conn, int argc 
> OVS_UNUSED,
> +                           const char *argv[], void *aux OVS_UNUSED)
> +{
> +    const char *br_name = argv[1];
> +    struct eth_addr mac;
> +    uint16_t vlan = atoi(argv[2]);
> +    const struct ofproto_dpif *ofproto;
> +    struct ds ds = DS_EMPTY_INITIALIZER;

Delete the above line.

> +
> +    ofproto = ofproto_dpif_lookup_by_name(br_name);
> +    if (!ofproto) {
> +        unixctl_command_reply_error(conn, "no such bridge");
> +        return;
> +    }
> +
> +    if (!eth_addr_from_string(argv[3], &mac)) {
> +        unixctl_command_reply_error(conn, "bad MAC address");
> +        return;
> +    }
> +    if (!xlate_delete_static_mac_entry(ofproto, mac, vlan)) {
> +        ds_put_format(&ds, "could not find static mac entry\n");
> +        unixctl_command_reply_error(conn, ds_cstr(&ds));

Replace the two above with:

            unixctl_command_reply_error(conn, “could not find static 
mac entry\n”);
> +    } else {
> +        unixctl_command_reply(conn, ds_cstr(&ds));

Change the above with:

			unixctl_command_reply(conn, NULL);
> +    }
> +
> +    ds_destroy(&ds);

Remove the above.

> +}
> +
>  static void
>  ofproto_unixctl_fdb_stats_clear(struct unixctl_conn *conn, int argc,
>                                  const char *argv[], void *aux 
> OVS_UNUSED)
> @@ -5911,6 +6013,9 @@ ofproto_unixctl_fdb_stats_show(struct 
> unixctl_conn *conn, int argc OVS_UNUSED,
>      ds_put_format(&ds, "  Current/maximum MAC entries in the table: 
> %"
>                    PRIuSIZE"/%"PRIuSIZE"\n",
>                    hmap_count(&ofproto->ml->table), 
> ofproto->ml->max_entries);
> +    ds_put_format(&ds,
> +                  "  Current static MAC entries in the table : 
> %"PRIuSIZE"\n",
> +                  ofproto->ml->static_entries);
>      ds_put_format(&ds,
>                    "  Total number of learned MAC entries     : 
> %"PRIu64"\n",
>                    ofproto->ml->total_learned);
> @@ -6417,6 +6522,10 @@ ofproto_unixctl_init(void)
>      }
>      registered = true;
>
> +    unixctl_command_register("fdb/add", "[bridge port vlan mac]", 4, 
> 4,
> +                             ofproto_unixctl_fdb_add, NULL);
> +    unixctl_command_register("fdb/del", "[bridge vlan mac]", 3, 3,
> +                             ofproto_unixctl_fdb_delete, NULL);
>      unixctl_command_register("fdb/flush", "[bridge]", 0, 1,
>                               ofproto_unixctl_fdb_flush, NULL);
>      unixctl_command_register("fdb/show", "bridge", 1, 1,
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index 31064ed95..88035bd10 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6753,6 +6753,105 @@ PORTNAME
>          portName=p2
>  ])])
>
> +AT_SETUP([ofproto-dpif - static-mac add/del/flush])
> +OVS_VSWITCHD_START([set bridge br0 fail-mode=standalone])
> +add_of_ports br0 1 2
> +
> +dnl Generate some dynamic fdb entries on some ports.
> +OFPROTO_TRACE([ovs-dummy], [in_port(1),eth(src=50:54:00:00:00:01)], 
> [-generate], [100,2])
> +OFPROTO_TRACE([ovs-dummy], [in_port(2),eth(src=50:54:00:00:00:02)], 
> [-generate], [100,1])
> +
> +dnl Add some static mac entries.
> +AT_CHECK([ovs-appctl fdb/add br0 p1 0 50:54:00:00:01:01])
> +AT_CHECK([ovs-appctl fdb/add br0 p2 0 50:54:00:00:02:02])
> +
> +dnl Check initial fdb.
> +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ 
> *[[0-9]]\{1,\}$//' | grep -v port | sort], [0], [dnl
> +    1     0  50:54:00:00:00:01
> +    1     0  50:54:00:00:01:01  static
> +    2     0  50:54:00:00:00:02
> +    2     0  50:54:00:00:02:02  static
> +])
> +
> +dnl Remove static mac entry.
> +AT_CHECK([ovs-appctl fdb/del br0 0 50:54:00:00:01:01])
> +
> +dnl Check that entry is removed.
> +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | grep 
> "50:54:00:00:01:01"], [1], [dnl
> +])
> +
> +# Add some more cache and static entries, to test out flush operation
> +for i in 0 1 2 3 4 5 6 7 8 9 a b c d e f; do
> +    ovs-appctl ofproto/trace ovs-dummy 
> "in_port(1),eth(src=50:54:00:00:0$i:ff)" -generate
> +    ovs-appctl fdb/add br0 p2 0 50:54:00:0$i:00:ff
> +done
> +
> +for i in 0 1 2 3 4 5 6 7 8 9 a b c d e f; do
> +    ovs-appctl fdb/add br0 p2 0 50:54:00:0$i:00:ff
> +done
> +
> +dnl Flush mac entries, only dynamic ones should be evicted.
> +AT_CHECK([ovs-appctl fdb/flush br0], [0], [dnl
> +table successfully flushed
> +])
> +
> +dnl Count number of static entries remaining.
> +AT_CHECK_UNQUOTED([ovs-appctl fdb/stats-show br0 | grep static], [0], 
> [dnl
> +  Current static MAC entries in the table : 17
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - static-mac mac moves])
> +OVS_VSWITCHD_START([set bridge br0 fail-mode=standalone])
> +add_of_ports br0 1 2
> +
> +dnl Generate a dynamic entry.
> +OFPROTO_TRACE([ovs-dummy], [in_port(1),eth(src=50:54:00:00:00:00)], 
> [-generate], [100,2])
> +
> +dnl Convert dynamically learnt dl_src to a static-mac.
> +AT_CHECK([ovs-appctl fdb/add br0 p1 0 50:54:00:00:00:00], [0], [dnl
> +Overriding already existing dynamic entry on 1
> +])
> +
> +dnl Check fdb.
> +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ 
> *[[0-9]]\{1,\}$//' | grep -v port | sort], [0], [dnl
> +    1     0  50:54:00:00:00:00  static
> +])
> +
> +dnl Move the static mac to different port.
> +AT_CHECK([ovs-appctl fdb/add br0 p2 0 50:54:00:00:00:00], [0], [dnl
> +Overriding already existing static entry on 1
> +])
> +
> +dnl Check fdb.
> +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ 
> *[[0-9]]\{1,\}$//' | grep -v port | sort], [0], [dnl
> +    2     0  50:54:00:00:00:00  static
> +])
> +
> +dnl static-mac should not be converted to a dynamic one when a packet 
> with same dl_src
> +dnl arrives on any port of the switch.
> +dnl Packet arriving on p1.
> +OFPROTO_TRACE([ovs-dummy], [in_port(1),eth(src=50:54:00:00:00:00)], 
> [-generate], [100,2])
> +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ 
> *[[0-9]]\{1,\}$//' | grep -v port | sort], [0], [dnl
> +    2     0  50:54:00:00:00:00  static
> +])
> +
> +dnl Packet arriving on p2.
> +OFPROTO_TRACE([ovs-dummy], [in_port(2),eth(src=50:54:00:00:00:00)], 
> [-generate], [100,1])
> +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ 
> *[[0-9]]\{1,\}$//' | grep -v port | sort], [0], [dnl
> +    2     0  50:54:00:00:00:00  static
> +])
> +
> +dnl Check mac_move coverage counter mac_learning_static_none_move.
> +AT_CHECK([ovs-appctl coverage/read-counter 
> mac_learning_static_none_move], [0], [dnl
> +1
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_SETUP([ofproto-dpif - basic truncate action])
>  OVS_VSWITCHD_START
>  add_of_ports br0 1 2 3 4 5
> diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in
> index 50dad7208..08920600e 100644
> --- a/vswitchd/ovs-vswitchd.8.in
> +++ b/vswitchd/ovs-vswitchd.8.in
> @@ -159,6 +159,11 @@ If \fIbridge\fR is not specified, then displays 
> detailed information about all
>  bridges with RSTP enabled.
>  .SS "BRIDGE COMMANDS"
>  These commands manage bridges.
> +.IP "\fBfdb/add\fR \fIbridge\fR \fIport\fR \fIvlan\fR \fImac\fR"
> +.IQ "\fBfdb/del\fR \fIbridge\fR \fIport\fR \fIvlan\fR \fImac\fR"
> +Adds/deletes \fImac\fR address on a \fIbridge\fR to/from \fIport\fR 
> and
> +\fIvlan\fR. This utility is can be used to pre-populate fdb table 
> without
> +relying on dynamic mac learning.
>  .IP "\fBfdb/flush\fR [\fIbridge\fR]"
>  Flushes \fIbridge\fR MAC address learning table, or all learning 
> tables
>  if no \fIbridge\fR is given.
> -- 
> 2.29.2


More information about the dev mailing list