| Message ID | 20210522181211.48976-1-vdasari@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v4] ofproto-dpif: APIs and CLI option to add/delete static fdb entry | expand |
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@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 > --- > 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. > + } 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(). > + 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? > + 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; } > + /* if mac is a static entry, then there is no need to update */ > + if (mac_entry_age(ml, mac) == MAC_ENTRY_AGE_STATIC_ENTRY) { > + return false; > + } > + > 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? > + } 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” > + * 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? > 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. > + 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
Eelco, Thank you for the detailed review. My comments inline and will have a new pull-request shortly: *Vasu Dasari* On Tue, May 25, 2021 at 10:32 AM Eelco Chaudron <echaudro@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@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 > > --- > > 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. > > > + } 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. > > > + /* if mac is a static entry, then there is no need to update */ > > + if (mac_entry_age(ml, mac) == MAC_ENTRY_AGE_STATIC_ENTRY) { > > + return false; > > + } > > + > > 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 > >
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@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@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 >>> --- >>> 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. > > >> >>> + } 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. > >> >>> + /* if mac is a static entry, then there is no need to update */ >>> + if (mac_entry_age(ml, mac) == MAC_ENTRY_AGE_STATIC_ENTRY) { >>> + return false; >>> + } >>> + >>> 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 >> >>
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@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@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@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 > >>> --- > >>> 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. > > > > > >> > >>> + } 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. > > > >> > >>> + /* if mac is a static entry, then there is no need to update */ > >>> + if (mac_entry_age(ml, mac) == MAC_ENTRY_AGE_STATIC_ENTRY) { > >>> + return false; > >>> + } > >>> + > >>> 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 > >> > >> > >
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@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@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@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 >>>>> --- >>>>> 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 >>>> >>>> >> >>
You are right, Eelco. Sorry that I missed two of your comments. I am addressing them in Patch v6 <https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/383884.html>. Thanks -Vasu *Vasu Dasari* On Fri, Jun 11, 2021 at 10:35 AM Eelco Chaudron <echaudro@redhat.com> wrote: > 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@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@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@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 > --- > 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 > > >
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; + } 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, + int vlan, unsigned int idle_time) +{ + struct mac_entry *e; + 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; } + /* if mac is a static entry, then there is no need to update */ + if (mac_entry_age(ml, mac) == MAC_ENTRY_AGE_STATIC_ENTRY) { + return false; + } + 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; + } + } 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. + * 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); 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); + + ovs_rwlock_wrlock(&ofproto->ml->rwlock); + mac_entry_set_idle_time(ofproto->ml, dl_src, vlan, INT_MAX); + 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); + unixctl_command_reply(conn, ds_cstr(&ds)); + } else if (!strcmp(op, "del")) { + xlate_delete_static_mac_entry(ofproto, mac, vlan); + unixctl_command_reply(conn, NULL); + } else { + unixctl_command_reply_error(conn, "software error, unknown op"); + } + 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
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@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 --- 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(-)