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 <[email protected]> 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 <[email protected]> >> 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 <[email protected]> >>>>> 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 <[email protected]> >>>>> --- >>>>> 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 >>>> >>>> >> >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
