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

Eelco Chaudron echaudro at redhat.com
Fri Jun 11 14:35:40 UTC 2021


Hi Vasu,

Will try to look at it in more details next week, but found two (actually one is a nit) not being fixed?

Any reason, see below…

//Eelco


On 10 Jun 2021, at 13:09, Vasu Dasari wrote:

> Hi Eelco,
>
> I addressed your comments and also added a counter to track number of
> static entries in the switch.
>
> Here is the new Patch v5
> <https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/383731.html> .
> Please take a look.
>
> Thanks
> -Vasu
>
> *Vasu Dasari*
>
>
> On Wed, May 26, 2021 at 4:52 AM Eelco Chaudron <echaudro at redhat.com> wrote:
>
>>
>>
>> On 25 May 2021, at 20:11, Vasu Dasari wrote:
>>
>>> Eelco, Thank you for the detailed review. My comments inline and will
>> have
>>> a new pull-request shortly:
>>
>> Thanks, take your time. I’m rather busy this week and early next week.
>>
>> //Eelco
>>
>>
>>> *Vasu Dasari*
>>>
>>>
>>> On Tue, May 25, 2021 at 10:32 AM Eelco Chaudron <echaudro at redhat.com>
>> wrote:
>>>
>>>> I did an initial code review. See comments inline below.
>>>>
>>>> //Eelco
>>>>
>>>>
>>>> On 22 May 2021, at 20:12, Vasu Dasari wrote:
>>>>
>>>>> Currently there is an option to add/flush/show ARP/ND neighbor. This
>>>> covers L3
>>>>> side.  For L2 side, there is only fdb show command. This patch gives an
>>>> option
>>>>> to add/del an fdb entry via ovs-appctl.
>>>>>
>>>>> CLI command looks like:
>>>>>
>>>>> To add:
>>>>>     ovs-appctl fdb/add <bridge> <port> <vlan> <Mac>
>>>>>     ovs-appctl fdb/add br0 p1 0 50:54:00:00:00:05
>>>>>
>>>>> To del:
>>>>>     ovs-appctl fdb/del <bridge> <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.
>>>>> void xlate_add_static_mac_entry(const struct ofproto_dpif *, ofp_port_t
>>>> in_port,
>>>>>                                struct eth_addr dl_src, int vlan);
>>>>> void 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.
>>>>>
>>>>> Added following tests:
>>>>>   ofproto-dpif - static-mac add/del/flush
>>>>>   ofproto-dpif - static-mac mac moves
>>>>>
>>>>> Signed-off-by: Vasu Dasari <vdasari at gmail.com>
>>>>> Reported-at:
>>>>
>> https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048894.html
>>>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1597752
>>>>> Tested-by: Eelco Chaudron <echaudro at redhat.com>
>>>>> ---
>>>>> v1:
>>>>>  - Fixed 0-day robot warnings
>>>>> v2:
>>>>>  - Fix valgrind error in the modified code in mac_learning_insert()
>>>> where a read is
>>>>>    is performed on e->expires which is not initialized
>>>>> v3:
>>>>>  - Addressed code review comments
>>>>>  - Added more documentation
>>>>>  - Fixed mac_entry_age() and is_mac_learning_update_needed() to have
>>>> common
>>>>>    understanding of return values when mac_entry is a static one.
>>>>>  - Added NEWS item
>>>>> v4:
>>>>>  - Addressed code review comments
>>>>>  - Static entries will not be purged when fdb/flush is performed.
>>>>>  - Static entries will not be overwritten when packet with same dl_src
>>>> arrives on
>>>>>    any port of switch
>>>>>  - Provided bit more detail while doing fdb/add, to indicate if
>>>> static-mac is
>>>>>    overriding already present entry
>>>>>  - Separated test cases for a bit more clarity
>>>>> ---
>>>>>  NEWS                         |  2 +
>>>>>  lib/mac-learning.c           | 61 +++++++++++++++++++++----
>>>>>  lib/mac-learning.h           | 11 +++++
>>>>>  ofproto/ofproto-dpif-xlate.c | 28 ++++++++++++
>>>>>  ofproto/ofproto-dpif-xlate.h |  5 ++
>>>>>  ofproto/ofproto-dpif.c       | 88 ++++++++++++++++++++++++++++++++++--
>>>>>  tests/ofproto-dpif.at        | 84 ++++++++++++++++++++++++++++++++++
>>>>>  7 files changed, 266 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/NEWS b/NEWS
>>>>> index 402ce5969..61ab61462 100644
>>>>> --- a/NEWS
>>>>> +++ b/NEWS
>>>>> @@ -5,6 +5,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>'
>>>>>     - 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..ab58e2ab6 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
>>>>> + * will return either one of the following:
>>>>> + *  1. Number of seconds since 'e' (within 'ml') was last learned.
>>>>> + *  2. If the mac entry is a static entry, it returns
>>>>> + *  MAC_ENTRY_AGE_STATIC_ENTRY
>>>>> + */
>>>>>  int
>>>>>  mac_entry_age(const struct mac_learning *ml, const struct mac_entry
>> *e)
>>>>>  {
>>>>> -    time_t remaining = e->expires - time_now();
>>>>> -    return ml->idle_time - remaining;
>>>>> +    /* For static fdb entries, expires would be
>>>> MAC_ENTRY_AGE_STATIC_ENTRY */
>>>>> +    if (MAC_ENTRY_AGE_STATIC_ENTRY == e->expires) {
>>>>> +        return e->expires;
>>>>
>>>> My preference here would be to do “return MAC_ENTRY_AGE_STATIC_ENTRY;”
>> but
>>>> I guess this is personal.
>>>>
>>>
>>> I do not have any preference either. I think it would be more readable,
>> so
>>> I will change it to MAC_ENTRY_AGE_STATIC_ENTRY.

NIT: You did not change this to “return MAC_ENTRY_AGE_STATIC_ENTRY” in v5.

>>>
>>>>
>>>>> +    } else {
>>>>> +        time_t remaining = e->expires - time_now();
>>>>> +        return ml->idle_time - remaining;
>>>>> +    }
>>>>>  }
>>>>>
>>>>>  static uint32_t
>>>>> @@ -282,6 +293,18 @@ mac_learning_set_idle_time(struct mac_learning
>> *ml,
>>>> unsigned int idle_time)
>>>>>      }
>>>>>  }
>>>>>
>>>>> +/* Changes the MAC aging timeout of a mac_entry to 'idle_time'
>> seconds.
>>>> */
>>>>> +void
>>>>> +mac_entry_set_idle_time(struct mac_learning *ml, struct eth_addr mac,
>>>>
>>>> mac_learning_xxx is the prefix for all mac_learning functions, so this
>>>> should be something like mac_learning_set_entry_idle_time().
>>>>
>>> This API is setting idle time on a mac_entry. I see the following APIs
>>> acting on mac_entry using prefix mac_entry_XXX. And hence took liberty of
>>> using this prefix.
>>>
>>> mac_entry_get_port()
>>> mac_entry_set_port()
>>> mac_entry_age()
>>> mac_entry_is_grat_arp_locked()
>>>
>>>>
>>>>> +        int vlan, unsigned int idle_time)
>>>>> +{
>>>>> +    struct mac_entry *e;
>>>>
>>>> We need to do some form of normalize_idle_time() here. Maybe update the
>>>> normalize_idle_time() function to allow MAC_ENTRY_AGE_STATIC_ENTR?
>>>>
>>>
>>> Will remove this API in place of mac_learning_add_static_entry()
>>>
>>>
>>>>
>>>>> +    e = mac_entry_lookup(ml, mac, vlan);
>>>>> +    if (e) {
>>>>> +        e->expires = idle_time;
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>  /* Sets the maximum number of entries in 'ml' to 'max_entries',
>>>> adjusting it
>>>>>   * to be within a reasonable range. */
>>>>>  void
>>>>> @@ -336,6 +359,7 @@ mac_learning_insert(struct mac_learning *ml,
>>>>>          e->vlan = vlan;
>>>>>          e->grat_arp_lock = TIME_MIN;
>>>>>          e->mlport = NULL;
>>>>> +        e->expires = 0;
>>>>>          COVERAGE_INC(mac_learning_learned);
>>>>>          ml->total_learned++;
>>>>>      } else {
>>>>> @@ -348,7 +372,10 @@ 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;
>>>>> +    /* Do not update 'expires' for static mac entry */
>>>>> +    if (e->expires != MAC_ENTRY_AGE_STATIC_ENTRY) {
>>>>> +        e->expires = time_now() + ml->idle_time;
>>>>> +    }
>>>>>
>>>>>      return e;
>>>>>  }
>>>>> @@ -378,10 +405,16 @@ is_mac_learning_update_needed(const struct
>>>> mac_learning *ml,
>>>>>      }
>>>>>
>>>>>      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;
>>>>>      }
>>>>
>>>> What happened here to the “if mac_entry_age(ml, mac) return true” case?
>>>> Was this a previous existing error, or did you remove it for another
>> reason?
>>>>
>>>> Trying to understand the use case in the existing code, I think it was
>>>> there to make sure the table gets updated if not yet timed out so that
>> the
>>>> e->expires gets updated.
>>>> So you probably need to add a check like (after the ==
>>>> MAC_ENTRY_AGE_STATIC_ENTRY below):
>>>>
>>>> age = mac_entry_age(ml, mac);
>>>> if (age > 0) {
>>>>   return true;
>>>> }
>>>>
>>>
>>> Yes. You are right. Will fix this.

This part has not been taken care of in your v5. Which I think should be fixed.
Something like:

>>>>> +    /* if mac is a static entry, then there is no need to update */

+    age = mac_entry_age(ml, mac);
+    if (age == MAC_ENTRY_AGE_STATIC_ENTRY) {
>>>>> +        return false;
>>>>> +    }
>>>>> +
+    if (age > 0) {
+        return true;
+    }
>>>>>      if (is_gratuitous_arp) {
>>>>>          /* We don't want to learn from gratuitous ARP packets that are
>>>>>           * reflected back over bond members so we lock the learning
>>>> table.  For
>>>>> @@ -513,13 +546,23 @@ 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;
>>>>> +            }
>>>>
>>>> Do not think this will work, the list is sorted on most recently used.
>>>> If you hit the first static entry, you will stop flushing?
>>>>
>>>
>>> I have added the "fdb/flush" unit test to test this logic. If the entry
>>> being removed is a static entry entry and if first_static_mac is not set,
>>> then set it to point to the the entry. If it is a cache entry it will be
>>> deleted. During the loop processing all cache entries will be deleted
>>> leaving out all static entries, in the same order they were present
>> before.
>>> We just need to break out from processing the static-macs list in a loop
>>> again. And hence the first_static_mac variable is introduced to track the
>>> first static-entry the while loop sees.
>>>
>>>
>>>>> +        } else {
>>>>> +            mac_learning_expire(ml, e);
>>>>> +        }
>>>>>      }
>>>>>      hmap_shrink(&ml->table);
>>>>>  }
>>>>> diff --git a/lib/mac-learning.h b/lib/mac-learning.h
>>>>> index 0ddab06cb..d8ff3172b 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 a age of
>>>> MAC_ENTRY_AGE_STATIC_ENTRY.
>>>>
>>>> Should be “an age”
>>>>
>>> Yes.
>>>
>>>>
>>>>> + * 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
>>>>> @@ -202,6 +210,9 @@ bool mac_learning_set_flood_vlans(struct
>>>> mac_learning *ml,
>>>>>  void mac_learning_set_idle_time(struct mac_learning *ml,
>>>>>                                  unsigned int idle_time)
>>>>>      OVS_REQ_WRLOCK(ml->rwlock);
>>>>> +void mac_entry_set_idle_time(struct mac_learning *ml, struct eth_addr
>>>> src,
>>>>> +                             int vlan, unsigned int idle_time)
>>>>> +    OVS_REQ_WRLOCK(ml->rwlock);
>>>>
>>>> Rather than have a mac_entry_set_idle_time() API, would it not be better
>>>> to have an API that hides the internal implementation of a setting time
>> to
>>>> INT_MAX?
>>>> We should have something like
>>>>
>>>> mac_learning_add_static_entry() to hide all this?
>>>>
>>>> I had some design choices to chose from. Either one involved adding an
>> API.
>>> 1. Add mac_entry_set_idle_time() API to modify the expires field, where I
>>> could existing functions to get the job done with minute tweaks.
>>> 2. Add mac_learning_add_static_entry(), where some code duplication will
>> be
>>> there for insert operation. It might not be a bad idea to take this
>>> approach. I can generate a pull request with this approach as well.
>>>
>>>>
>>>>>  void mac_learning_set_max_entries(struct mac_learning *ml, size_t
>>>> max_entries)
>>>>>      OVS_REQ_WRLOCK(ml->rwlock);
>>>>>
>>>>> diff --git a/ofproto/ofproto-dpif-xlate.c
>> b/ofproto/ofproto-dpif-xlate.c
>>>>> index 7108c8a30..4392a38f4 100644
>>>>> --- a/ofproto/ofproto-dpif-xlate.c
>>>>> +++ b/ofproto/ofproto-dpif-xlate.c
>>>>> @@ -8011,6 +8011,34 @@ xlate_mac_learning_update(const struct
>>>> ofproto_dpif *ofproto,
>>>>>      update_learning_table__(xbridge, xbundle, dl_src, vlan,
>>>> is_grat_arp);
>>>>>  }
>>>>>
>>>>> +void
>>>>> +xlate_add_static_mac_entry(const struct ofproto_dpif *ofproto,
>>>>> +                           ofp_port_t in_port,
>>>>> +                           struct eth_addr dl_src, int vlan)
>>>>> +{
>>>>> +    xlate_delete_static_mac_entry(ofproto, dl_src, vlan);
>>>>> +
>>>>> +    xlate_mac_learning_update(ofproto, in_port, dl_src, vlan, false);
>>>>
>>>> So what happens here if the entry got added by the call above, but due
>> to
>>>> us not having a lock, the source port could have changed, and now we
>> make
>>>> it static!? Guess a new API should solve this (see above).
>>>>
>>>>> +    ovs_rwlock_wrlock(&ofproto->ml->rwlock);
>>>>> +    mac_entry_set_idle_time(ofproto->ml, dl_src, vlan, INT_MAX);
>>>>
>>>> Why INT_MAX, there should be a #define for this.
>>>>
>>> This code will go away with mac_learning_add_static_entry() approach
>>>
>>>>
>>>>> +    ovs_rwlock_unlock(&ofproto->ml->rwlock);
>>>>> +}
>>>>> +
>>>>> +void
>>>>> +xlate_delete_static_mac_entry(const struct ofproto_dpif *ofproto,
>>>>> +                              struct eth_addr dl_src, int vlan)
>>>>> +{
>>>>> +    struct mac_entry *e = NULL;
>>>>> +
>>>>> +    ovs_rwlock_wrlock(&ofproto->ml->rwlock);
>>>>> +    e = mac_learning_lookup(ofproto->ml, dl_src, vlan);
>>>>> +    if (e) {
>>>>> +        mac_learning_expire(ofproto->ml, e);
>>>>> +    }
>>>>> +    ovs_rwlock_unlock(&ofproto->ml->rwlock);
>>>>> +}
>>>>> +
>>>>>  void
>>>>>  xlate_set_support(const struct ofproto_dpif *ofproto,
>>>>>                      const struct dpif_backer_support *support)
>>>>> diff --git a/ofproto/ofproto-dpif-xlate.h
>> b/ofproto/ofproto-dpif-xlate.h
>>>>> index 3426a27b2..9e6e95756 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);
>>>>> +void xlate_add_static_mac_entry(const struct ofproto_dpif *,
>>>>> +                                ofp_port_t in_port,
>>>>> +                                struct eth_addr dl_src, int vlan);
>>>>> +void 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 fd0b2fdea..97f2ac475 100644
>>>>> --- a/ofproto/ofproto-dpif.c
>>>>> +++ b/ofproto/ofproto-dpif.c
>>>>> @@ -5852,18 +5852,94 @@ 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);
>>>>> +        }
>>>>> +
>>>>> +        xlate_add_static_mac_entry(ofproto, in_port, mac, vlan);
>>>>
>>>> Is it impossible for xlate_add_static_mac_entry() to fail?
>>>>
>>>>> +        unixctl_command_reply(conn, ds_cstr(&ds));
>>>>> +    } else if (!strcmp(op, "del")) {
>>>>> +        xlate_delete_static_mac_entry(ofproto, mac, vlan);
>>>>
>>>> Maybe an error if we tried to delete a non existing entry?
>>>>
>>>>> +        unixctl_command_reply(conn, NULL);
>>>>> +    } else {
>>>>> +        unixctl_command_reply_error(conn, "software error, unknown
>> op");
>>>>
>>>> Guess “op” might be to short would just spell it out, operation.
>>>>> +    }
>>>>> +    ds_destroy(&ds);
>>>>> +}
>>>>> +
>>>>>  static void
>>>>>  ofproto_unixctl_fdb_stats_clear(struct unixctl_conn *conn, int argc,
>>>>>                                  const char *argv[], void *aux
>>>> OVS_UNUSED)
>>>>> @@ -6415,6 +6491,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..a6105df1d 100644
>>>>> --- a/tests/ofproto-dpif.at
>>>>> +++ b/tests/ofproto-dpif.at
>>>>> @@ -6753,6 +6753,90 @@ 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
>>>>> +])
>>>>> +
>>>>> +dnl Flush mac entries, only dynamic ones should be evicted.
>>>>> +AT_CHECK([ovs-appctl fdb/flush br0], [0], [dnl
>>>>> +table successfully flushed
>>>>> +])
>>>>> +
>>>>> +dnl Check that entry is removed
>>>>> +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:02:02  static
>>>>> +])
>>>>> +
>>>>> +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
>>>>
>>>>
>>
>>



More information about the dev mailing list