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

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> <port> <vlan> <Mac>
    ovs-appctl fdb/del br0 p1 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@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@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()
---
 NEWS                         |   2 +
 lib/mac-learning.c           | 149 +++++++++++++++++++++++++++++++----
 lib/mac-learning.h           |  17 ++++
 ofproto/ofproto-dpif-xlate.c |  48 +++++++++--
 ofproto/ofproto-dpif-xlate.h |   5 ++
 ofproto/ofproto-dpif.c       |  95 +++++++++++++++++++++-
 tests/ofproto-dpif.at        |  93 ++++++++++++++++++++++
 7 files changed, 380 insertions(+), 29 deletions(-)

On Fri, Jun 11, 2021 at 10:09:58PM -0400, 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.

I didn't really review this patch, but I noticed that there is no
documentation for the new commands.  Please add some in
vswitchd/ovs-vswitchd.8.in, near the documentation for the existing fdb
commands.

Ok Ben, Will do that.

Thanks
-Vasu

*Vasu Dasari*


On Mon, Jun 14, 2021 at 11:48 AM Ben Pfaff <blp@ovn.org> wrote:

> On Fri, Jun 11, 2021 at 10:09:58PM -0400, 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.
>
> I didn't really review this patch, but I noticed that there is no
> documentation for the new commands.  Please add some in
> vswitchd/ovs-vswitchd.8.in, near the documentation for the existing fdb
> commands.
>

> On 12 Jun 2021, at 4:09, Vasu Dasari wrote:

See my inline comments below.

Cheers,

Eelco


> 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> <port> <vlan> <Mac>
>     ovs-appctl fdb/del br0 p1 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@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@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()
> ---
>  NEWS                         |   2 +
>  lib/mac-learning.c           | 149 +++++++++++++++++++++++++++++++----
>  lib/mac-learning.h           |  17 ++++
>  ofproto/ofproto-dpif-xlate.c |  48 +++++++++--
>  ofproto/ofproto-dpif-xlate.h |   5 ++
>  ofproto/ofproto-dpif.c       |  95 +++++++++++++++++++++-
>  tests/ofproto-dpif.at        |  93 ++++++++++++++++++++++
>  7 files changed, 380 insertions(+), 29 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index ebba17b22..501b26cee 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,6 +9,8 @@ Post-v2.15.0
>     - Userspace datapath:
>       * Auto load balancing of PMDs now partially supports cross-NUMA polling
>         cases, e.g if all PMD threads are running on the same NUMA node.
> +     * Added ability to add and delete static mac entries using:
> +       'ovs-appctl fdb/{add,del} <bridge> <port> <vlan> <mac>'

I think this needs its own section, not being part of  "- Userspace datapath:". So something like:

    - Added ability to add and delete static mac entries using:
       'ovs-appctl fdb/{add,del} <bridge> <port> <vlan> <mac>'

>     - ovs-ctl:
>       * New option '--no-record-hostname' to disable hostname configuration
>         in ovsdb on startup.
> diff --git a/lib/mac-learning.c b/lib/mac-learning.c
> index 3d5293d3b..2f51a6553 100644
> --- a/lib/mac-learning.c
> +++ b/lib/mac-learning.c
> @@ -35,12 +35,23 @@ COVERAGE_DEFINE(mac_learning_expired);
>  COVERAGE_DEFINE(mac_learning_evicted);
>  COVERAGE_DEFINE(mac_learning_moved);
>
> -/* Returns the number of seconds since 'e' (within 'ml') was last learned. */
> +/*
> + * This function will return age of mac entry in the fdb. It

NIT: If you break the line before 80, I would also move the above "It" to the line below.

> + * 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

NIT: I would indent MAC_ENTRY_AGE_STATIC_ENTRY with the "If" above.

> + */
>  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
> @@ -187,6 +198,7 @@ mac_learning_clear_statistics(struct mac_learning *ml)
>  {
>      if (ml != NULL) {
>          ml->total_learned = 0;
> +        ml->total_static = 0;

Do not clear this here, as it would reset the static count on clear stats, which would cause negative numbers, etc.
This is not really a statistics/total counter, but more a current number of static entries. Maybe it should be named as such, i.e. current_static?

Please add a ml->total_static = 0 to the mac_learning_create() function.

>          ml->total_expired = 0;
>          ml->total_evicted = 0;
>          ml->total_moved = 0;
> @@ -309,16 +321,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 +351,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 +366,80 @@ 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 total_static only if entry is a new one or entry is
> +         * is converted from cache to static type */
> +        if (e->expires != MAC_ENTRY_AGE_STATIC_ENTRY) {
> +            ml->total_static++;
> +        }
> +        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) {
> +        if (mac_entry_get_port(ml, mac) != in_port) {

First thing mac_entry_set_port() does is "if (mac_entry_get_port(ml, e) != port)", so I think we can remove the check above and always set the port.

> +            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) {

Guess this needs to be "if (mac && mac_entry_age(ml, mac) == MAC_ENTRY_AGE_STATIC_ENTRY) to make sure we only remove static entries.

> +        mac_learning_expire(ml, mac);
> +        ml->total_static--;
> +        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 +459,27 @@ 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) {

For debugging, we need some coverage counter when the update comes from a different port, i.e., where the MAC is seen on another port than the static one.

    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 +614,27 @@ 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..07fb6331a 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
> @@ -162,6 +170,7 @@ struct mac_learning {
>
>      /* Statistics */
>      uint64_t total_learned;
> +    uint64_t total_static;

I think this is not a statistic value, just the current number of static entries in the table (see other comments above).
So I would move it up, below max_entries, and make it looks something like this:

        size_t max_entries;         /* Max number of learned MACs. */
+       size_t static_entries;      /* Current number of static MAC entries. */

>      uint64_t total_expired;
>      uint64_t total_evicted;
>      uint64_t total_moved;
> @@ -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 7108c8a30..b74255fc7 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -7989,26 +7989,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 */
> +    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..75843fc0e 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5854,18 +5854,98 @@ 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_update(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 char *op = (const char *) aux;
> +    const struct ofproto_dpif *ofproto;
> +    ofp_port_t    in_port = OFPP_NONE;
> +    struct ofproto_port ofproto_port;
> +    struct ds ds = DS_EMPTY_INITIALIZER;
> +
> +    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);
> +
> +    if (!strcmp(op, "add")) {
> +        /* Give a bit more information if the entry being added is overriding
> +         * an existing entry */
> +        const struct mac_entry *mac_entry;
> +        const struct ofbundle *bundle = NULL;
> +        int age;
> +
> +        ovs_rwlock_rdlock(&ofproto->ml->rwlock);
> +        mac_entry = mac_learning_lookup(ofproto->ml, mac, vlan);
> +        if (mac_entry) {
> +            bundle = mac_entry ?
> +                mac_entry_get_port(ofproto->ml, mac_entry) : NULL;
> +            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(conn, ds_cstr(&ds));
> +    } else if (!strcmp(op, "del")) {
> +        if (!xlate_delete_static_mac_entry(ofproto, mac, vlan)) {
> +            ds_put_format(&ds, "could not find static mac entry\n");
> +        }

As the port number is not requered for deleting, I think it should be deleted from the command, so it would become:

   ovs-appctl fdb/del <bridge> <vlan> <mac>

> +        unixctl_command_reply(conn, ds_cstr(&ds));
> +    } else {
> +        unixctl_command_reply_error(conn, "software error, unknown operation");
> +    }
> +    ds_destroy(&ds);
> +}
> +
>  static void
>  ofproto_unixctl_fdb_stats_clear(struct unixctl_conn *conn, int argc,
>                                  const char *argv[], void *aux OVS_UNUSED)
> @@ -5914,6 +5994,9 @@ ofproto_unixctl_fdb_stats_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>      ds_put_format(&ds,
>                    "  Total number of learned MAC entries     : %"PRIu64"\n",
>                    ofproto->ml->total_learned);
> +    ds_put_format(&ds,
> +                  "  Total number of static  MAC entries     : %"PRIu64"\n",
> +                  ofproto->ml->total_static);

Maybe this should reflect more the value, like "Current static MAC entries in the table"?

And move it up, for example:

   Current/maximum MAC entries in the table: 17/8192
+  Current static MAC entries in the table : 17
   Total number of learned MAC entries     : 18
-  Total number of static  MAC entries     : 17
   Total number of expired MAC entries     : 0
   Total number of evicted MAC entries     : 0
   Total number of port moved MAC entries  : 0

>      ds_put_format(&ds,
>                    "  Total number of expired MAC entries     : %"PRIu64"\n",
>                    ofproto->ml->total_expired);
> @@ -6417,6 +6500,10 @@ ofproto_unixctl_init(void)
>      }
>      registered = true;
>
> +    unixctl_command_register("fdb/add", "[bridge port vlan mac]", 4, 4,
> +                             ofproto_unixctl_fdb_update, "add");
> +    unixctl_command_register("fdb/del", "[bridge port vlan mac]", 4, 4,
> +                             ofproto_unixctl_fdb_update, "del");
>      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..b2c5734d2 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -6753,6 +6753,99 @@ 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 p1 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
> +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

I noticed you fixed the potential issue I pointed out with the flush. To make sure we would catch it, I think you should have some interleaved entries. Maybe change it to the following?

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

> +
> +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
> +  Total number of static  MAC entries     : 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
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
> +
>  AT_SETUP([ofproto-dpif - basic truncate action])
>  OVS_VSWITCHD_START
>  add_of_ports br0 1 2 3 4 5
> -- 
> 2.29.2

Vasu, I reviewed your v7, but added my comments in the v6 email :(

As only the documentation updated in v7, it should reflect the same code areas.

Cheers,

Eelco


On 23 Jun 2021, at 16:41, Eelco Chaudron wrote:

>> On 12 Jun 2021, at 4:09, Vasu Dasari wrote:
>
> See my inline comments below.
>
> Cheers,
>
> Eelco
>
>
>> 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> <port> <vlan> <Mac>
>>     ovs-appctl fdb/del br0 p1 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@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@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()
>> ---
>>  NEWS                         |   2 +
>>  lib/mac-learning.c           | 149 +++++++++++++++++++++++++++++++----
>>  lib/mac-learning.h           |  17 ++++
>>  ofproto/ofproto-dpif-xlate.c |  48 +++++++++--
>>  ofproto/ofproto-dpif-xlate.h |   5 ++
>>  ofproto/ofproto-dpif.c       |  95 +++++++++++++++++++++-
>>  tests/ofproto-dpif.at        |  93 ++++++++++++++++++++++
>>  7 files changed, 380 insertions(+), 29 deletions(-)
>>
>> diff --git a/NEWS b/NEWS
>> index ebba17b22..501b26cee 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -9,6 +9,8 @@ Post-v2.15.0
>>     - Userspace datapath:
>>       * Auto load balancing of PMDs now partially supports cross-NUMA polling
>>         cases, e.g if all PMD threads are running on the same NUMA node.
>> +     * Added ability to add and delete static mac entries using:
>> +       'ovs-appctl fdb/{add,del} <bridge> <port> <vlan> <mac>'
>
> I think this needs its own section, not being part of  "- Userspace datapath:". So something like:
>
>     - Added ability to add and delete static mac entries using:
>        'ovs-appctl fdb/{add,del} <bridge> <port> <vlan> <mac>'
>
>>     - ovs-ctl:
>>       * New option '--no-record-hostname' to disable hostname configuration
>>         in ovsdb on startup.
>> diff --git a/lib/mac-learning.c b/lib/mac-learning.c
>> index 3d5293d3b..2f51a6553 100644
>> --- a/lib/mac-learning.c
>> +++ b/lib/mac-learning.c
>> @@ -35,12 +35,23 @@ COVERAGE_DEFINE(mac_learning_expired);
>>  COVERAGE_DEFINE(mac_learning_evicted);
>>  COVERAGE_DEFINE(mac_learning_moved);
>>
>> -/* Returns the number of seconds since 'e' (within 'ml') was last learned. */
>> +/*
>> + * This function will return age of mac entry in the fdb. It
>
> NIT: If you break the line before 80, I would also move the above "It" to the line below.
>
>> + * 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
>
> NIT: I would indent MAC_ENTRY_AGE_STATIC_ENTRY with the "If" above.
>
>> + */
>>  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
>> @@ -187,6 +198,7 @@ mac_learning_clear_statistics(struct mac_learning *ml)
>>  {
>>      if (ml != NULL) {
>>          ml->total_learned = 0;
>> +        ml->total_static = 0;
>
> Do not clear this here, as it would reset the static count on clear stats, which would cause negative numbers, etc.
> This is not really a statistics/total counter, but more a current number of static entries. Maybe it should be named as such, i.e. current_static?
>
> Please add a ml->total_static = 0 to the mac_learning_create() function.
>
>>          ml->total_expired = 0;
>>          ml->total_evicted = 0;
>>          ml->total_moved = 0;
>> @@ -309,16 +321,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 +351,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 +366,80 @@ 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 total_static only if entry is a new one or entry is
>> +         * is converted from cache to static type */
>> +        if (e->expires != MAC_ENTRY_AGE_STATIC_ENTRY) {
>> +            ml->total_static++;
>> +        }
>> +        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) {
>> +        if (mac_entry_get_port(ml, mac) != in_port) {
>
> First thing mac_entry_set_port() does is "if (mac_entry_get_port(ml, e) != port)", so I think we can remove the check above and always set the port.
>
>> +            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) {
>
> Guess this needs to be "if (mac && mac_entry_age(ml, mac) == MAC_ENTRY_AGE_STATIC_ENTRY) to make sure we only remove static entries.
>
>> +        mac_learning_expire(ml, mac);
>> +        ml->total_static--;
>> +        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 +459,27 @@ 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) {
>
> For debugging, we need some coverage counter when the update comes from a different port, i.e., where the MAC is seen on another port than the static one.
>
>     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 +614,27 @@ 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..07fb6331a 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
>> @@ -162,6 +170,7 @@ struct mac_learning {
>>
>>      /* Statistics */
>>      uint64_t total_learned;
>> +    uint64_t total_static;
>
> I think this is not a statistic value, just the current number of static entries in the table (see other comments above).
> So I would move it up, below max_entries, and make it looks something like this:
>
>         size_t max_entries;         /* Max number of learned MACs. */
> +       size_t static_entries;      /* Current number of static MAC entries. */
>
>>      uint64_t total_expired;
>>      uint64_t total_evicted;
>>      uint64_t total_moved;
>> @@ -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 7108c8a30..b74255fc7 100644
>> --- a/ofproto/ofproto-dpif-xlate.c
>> +++ b/ofproto/ofproto-dpif-xlate.c
>> @@ -7989,26 +7989,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 */
>> +    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..75843fc0e 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -5854,18 +5854,98 @@ 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_update(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 char *op = (const char *) aux;
>> +    const struct ofproto_dpif *ofproto;
>> +    ofp_port_t    in_port = OFPP_NONE;
>> +    struct ofproto_port ofproto_port;
>> +    struct ds ds = DS_EMPTY_INITIALIZER;
>> +
>> +    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);
>> +
>> +    if (!strcmp(op, "add")) {
>> +        /* Give a bit more information if the entry being added is overriding
>> +         * an existing entry */
>> +        const struct mac_entry *mac_entry;
>> +        const struct ofbundle *bundle = NULL;
>> +        int age;
>> +
>> +        ovs_rwlock_rdlock(&ofproto->ml->rwlock);
>> +        mac_entry = mac_learning_lookup(ofproto->ml, mac, vlan);
>> +        if (mac_entry) {
>> +            bundle = mac_entry ?
>> +                mac_entry_get_port(ofproto->ml, mac_entry) : NULL;
>> +            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(conn, ds_cstr(&ds));
>> +    } else if (!strcmp(op, "del")) {
>> +        if (!xlate_delete_static_mac_entry(ofproto, mac, vlan)) {
>> +            ds_put_format(&ds, "could not find static mac entry\n");
>> +        }
>
> As the port number is not requered for deleting, I think it should be deleted from the command, so it would become:
>
>    ovs-appctl fdb/del <bridge> <vlan> <mac>
>
>> +        unixctl_command_reply(conn, ds_cstr(&ds));
>> +    } else {
>> +        unixctl_command_reply_error(conn, "software error, unknown operation");
>> +    }
>> +    ds_destroy(&ds);
>> +}
>> +
>>  static void
>>  ofproto_unixctl_fdb_stats_clear(struct unixctl_conn *conn, int argc,
>>                                  const char *argv[], void *aux OVS_UNUSED)
>> @@ -5914,6 +5994,9 @@ ofproto_unixctl_fdb_stats_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>      ds_put_format(&ds,
>>                    "  Total number of learned MAC entries     : %"PRIu64"\n",
>>                    ofproto->ml->total_learned);
>> +    ds_put_format(&ds,
>> +                  "  Total number of static  MAC entries     : %"PRIu64"\n",
>> +                  ofproto->ml->total_static);
>
> Maybe this should reflect more the value, like "Current static MAC entries in the table"?
>
> And move it up, for example:
>
>    Current/maximum MAC entries in the table: 17/8192
> +  Current static MAC entries in the table : 17
>    Total number of learned MAC entries     : 18
> -  Total number of static  MAC entries     : 17
>    Total number of expired MAC entries     : 0
>    Total number of evicted MAC entries     : 0
>    Total number of port moved MAC entries  : 0
>
>>      ds_put_format(&ds,
>>                    "  Total number of expired MAC entries     : %"PRIu64"\n",
>>                    ofproto->ml->total_expired);
>> @@ -6417,6 +6500,10 @@ ofproto_unixctl_init(void)
>>      }
>>      registered = true;
>>
>> +    unixctl_command_register("fdb/add", "[bridge port vlan mac]", 4, 4,
>> +                             ofproto_unixctl_fdb_update, "add");
>> +    unixctl_command_register("fdb/del", "[bridge port vlan mac]", 4, 4,
>> +                             ofproto_unixctl_fdb_update, "del");
>>      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..b2c5734d2 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -6753,6 +6753,99 @@ 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 p1 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
>> +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
>
> I noticed you fixed the potential issue I pointed out with the flush. To make sure we would catch it, I think you should have some interleaved entries. Maybe change it to the following?
>
> 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
>
>> +
>> +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
>> +  Total number of static  MAC entries     : 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
>> +])
>> +
>> +OVS_VSWITCHD_STOP
>> +AT_CLEANUP
>> +
>>  AT_SETUP([ofproto-dpif - basic truncate action])
>>  OVS_VSWITCHD_START
>>  add_of_ports br0 1 2 3 4 5
>> -- 
>> 2.29.2

Hi Eelco,

Thank you for a thorough review. I have Ack'd all comments and created a
patch at Patch v8
<https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/384363.html>.

 My comments inline: (Mostly all Acks)

Thanks
-Vasu

*Vasu Dasari*


On Wed, Jun 23, 2021 at 10:41 AM Eelco Chaudron <echaudro@redhat.com> wrote:

> > On 12 Jun 2021, at 4:09, Vasu Dasari wrote:
>
> See my inline comments below.
>
> Cheers,
>
> Eelco
>
>
> > 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> <port> <vlan> <Mac>
> >     ovs-appctl fdb/del br0 p1 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@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@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()
> > ---
> >  NEWS                         |   2 +
> >  lib/mac-learning.c           | 149 +++++++++++++++++++++++++++++++----
> >  lib/mac-learning.h           |  17 ++++
> >  ofproto/ofproto-dpif-xlate.c |  48 +++++++++--
> >  ofproto/ofproto-dpif-xlate.h |   5 ++
> >  ofproto/ofproto-dpif.c       |  95 +++++++++++++++++++++-
> >  tests/ofproto-dpif.at        |  93 ++++++++++++++++++++++
> >  7 files changed, 380 insertions(+), 29 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index ebba17b22..501b26cee 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -9,6 +9,8 @@ Post-v2.15.0
> >     - Userspace datapath:
> >       * Auto load balancing of PMDs now partially supports cross-NUMA
> polling
> >         cases, e.g if all PMD threads are running on the same NUMA node.
> > +     * Added ability to add and delete static mac entries using:
> > +       'ovs-appctl fdb/{add,del} <bridge> <port> <vlan> <mac>'
>
> I think this needs its own section, not being part of  "- Userspace
> datapath:". So something like:
>
>     - Added ability to add and delete static mac entries using:
>        'ovs-appctl fdb/{add,del} <bridge> <port> <vlan> <mac>'
>
> Ack

> >     - ovs-ctl:
> >       * New option '--no-record-hostname' to disable hostname
> configuration
> >         in ovsdb on startup.
> > diff --git a/lib/mac-learning.c b/lib/mac-learning.c
> > index 3d5293d3b..2f51a6553 100644
> > --- a/lib/mac-learning.c
> > +++ b/lib/mac-learning.c
> > @@ -35,12 +35,23 @@ COVERAGE_DEFINE(mac_learning_expired);
> >  COVERAGE_DEFINE(mac_learning_evicted);
> >  COVERAGE_DEFINE(mac_learning_moved);
> >
> > -/* Returns the number of seconds since 'e' (within 'ml') was last
> learned. */
> > +/*
> > + * This function will return age of mac entry in the fdb. It
>
> NIT: If you break the line before 80, I would also move the above "It" to
> the line below.
>
Ack

>
> > + * 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
>
> NIT: I would indent MAC_ENTRY_AGE_STATIC_ENTRY with the "If" above.
>
Ack

>
> > + */
> >  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
> > @@ -187,6 +198,7 @@ mac_learning_clear_statistics(struct mac_learning
> *ml)
> >  {
> >      if (ml != NULL) {
> >          ml->total_learned = 0;
> > +        ml->total_static = 0;
>
> Do not clear this here, as it would reset the static count on clear stats,
> which would cause negative numbers, etc.
> This is not really a statistics/total counter, but more a current number
> of static entries. Maybe it should be named as such, i.e. current_static?
>
> Please add a ml->total_static = 0 to the mac_learning_create() function.
>
Ack

>
> >          ml->total_expired = 0;
> >          ml->total_evicted = 0;
> >          ml->total_moved = 0;
> > @@ -309,16 +321,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 +351,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 +366,80 @@ 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 total_static only if entry is a new one or entry is
> > +         * is converted from cache to static type */
> > +        if (e->expires != MAC_ENTRY_AGE_STATIC_ENTRY) {
> > +            ml->total_static++;
> > +        }
> > +        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) {
> > +        if (mac_entry_get_port(ml, mac) != in_port) {
>
> First thing mac_entry_set_port() does is "if (mac_entry_get_port(ml, e) !=
> port)", so I think we can remove the check above and always set the port.
>
> > +            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) {
>
> Guess this needs to be "if (mac && mac_entry_age(ml, mac) ==
> MAC_ENTRY_AGE_STATIC_ENTRY) to make sure we only remove static entries.
>
> > +        mac_learning_expire(ml, mac);
> > +        ml->total_static--;
> > +        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 +459,27 @@ 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) {
>
> For debugging, we need some coverage counter when the update comes from a
> different port, i.e., where the MAC is seen on another port than the static
> one.
>
>     if (mac_entry_get_port(ml, mac) != in_port) {
>         COVERAGE_INC(mac_learning_static_none_move);
>     }
>
> Ack

> > +        return false;
> > +    }
> > +
> > +    /* If entry is still alive, just update the mac_entry so, that
> expires
> > +     * gets updated */
> > +    if (age > 0) {
> >          return true;
> >      }
> >
> > @@ -513,13 +614,27 @@ 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..07fb6331a 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
> > @@ -162,6 +170,7 @@ struct mac_learning {
> >
> >      /* Statistics */
> >      uint64_t total_learned;
> > +    uint64_t total_static;
>
> I think this is not a statistic value, just the current number of static
> entries in the table (see other comments above).
> So I would move it up, below max_entries, and make it looks something like
> this:
>
> Ack

>         size_t max_entries;         /* Max number of learned MACs. */
> +       size_t static_entries;      /* Current number of static MAC
> entries. */
>
> >      uint64_t total_expired;
> >      uint64_t total_evicted;
> >      uint64_t total_moved;
> > @@ -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 7108c8a30..b74255fc7 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -7989,26 +7989,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 */
> > +    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..75843fc0e 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -5854,18 +5854,98 @@ 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_update(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 char *op = (const char *) aux;
> > +    const struct ofproto_dpif *ofproto;
> > +    ofp_port_t    in_port = OFPP_NONE;
> > +    struct ofproto_port ofproto_port;
> > +    struct ds ds = DS_EMPTY_INITIALIZER;
> > +
> > +    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);
> > +
> > +    if (!strcmp(op, "add")) {
> > +        /* Give a bit more information if the entry being added is
> overriding
> > +         * an existing entry */
> > +        const struct mac_entry *mac_entry;
> > +        const struct ofbundle *bundle = NULL;
> > +        int age;
> > +
> > +        ovs_rwlock_rdlock(&ofproto->ml->rwlock);
> > +        mac_entry = mac_learning_lookup(ofproto->ml, mac, vlan);
> > +        if (mac_entry) {
> > +            bundle = mac_entry ?
> > +                mac_entry_get_port(ofproto->ml, mac_entry) : NULL;
> > +            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(conn, ds_cstr(&ds));
> > +    } else if (!strcmp(op, "del")) {
> > +        if (!xlate_delete_static_mac_entry(ofproto, mac, vlan)) {
> > +            ds_put_format(&ds, "could not find static mac entry\n");
> > +        }
>
> As the port number is not requered for deleting, I think it should be
> deleted from the command, so it would become:
>
>    ovs-appctl fdb/del <bridge> <vlan> <mac>
>
> Ack

> > +        unixctl_command_reply(conn, ds_cstr(&ds));
> > +    } else {
> > +        unixctl_command_reply_error(conn, "software error, unknown
> operation");
> > +    }
> > +    ds_destroy(&ds);
> > +}
> > +
> >  static void
> >  ofproto_unixctl_fdb_stats_clear(struct unixctl_conn *conn, int argc,
> >                                  const char *argv[], void *aux
> OVS_UNUSED)
> > @@ -5914,6 +5994,9 @@ ofproto_unixctl_fdb_stats_show(struct unixctl_conn
> *conn, int argc OVS_UNUSED,
> >      ds_put_format(&ds,
> >                    "  Total number of learned MAC entries     :
> %"PRIu64"\n",
> >                    ofproto->ml->total_learned);
> > +    ds_put_format(&ds,
> > +                  "  Total number of static  MAC entries     :
> %"PRIu64"\n",
> > +                  ofproto->ml->total_static);
>
> Maybe this should reflect more the value, like "Current static MAC entries
> in the table"?
>
> And move it up, for example:
>
>    Current/maximum MAC entries in the table: 17/8192
> +  Current static MAC entries in the table : 17
>    Total number of learned MAC entries     : 18
> -  Total number of static  MAC entries     : 17
>    Total number of expired MAC entries     : 0
>    Total number of evicted MAC entries     : 0
>    Total number of port moved MAC entries  : 0
>
> >      ds_put_format(&ds,
> >                    "  Total number of expired MAC entries     :
> %"PRIu64"\n",
> >                    ofproto->ml->total_expired);
> > @@ -6417,6 +6500,10 @@ ofproto_unixctl_init(void)
> >      }
> >      registered = true;
> >
> > +    unixctl_command_register("fdb/add", "[bridge port vlan mac]", 4, 4,
> > +                             ofproto_unixctl_fdb_update, "add");
> > +    unixctl_command_register("fdb/del", "[bridge port vlan mac]", 4, 4,
> > +                             ofproto_unixctl_fdb_update, "del");
> >      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..b2c5734d2 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -6753,6 +6753,99 @@ 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 p1 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
> > +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
>
> I noticed you fixed the potential issue I pointed out with the flush. To
> make sure we would catch it, I think you should have some interleaved
> entries. Maybe change it to the following?
>
> Yes, I did fix the issue you identified. My earlier test was not catching
the bug. Once I improved the test, I caught the bug you identified. Will
make following change as well.

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
>
> Ack

> > +
> > +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
> > +  Total number of static  MAC entries     : 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
> > +])
> > +
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> >  AT_SETUP([ofproto-dpif - basic truncate action])
> >  OVS_VSWITCHD_START
> >  add_of_ports br0 1 2 3 4 5
> > --
> > 2.29.2
>
>