Thank you Eelco for a patient review. I have addressed all your comments, also fixed comments in test files as well.
Here is the Patch v9 <https://mail.openvswitch.org/pipermail/ovs-dev/2021-June/384417.html>. Thanks -Vasu *Vasu Dasari* On Thu, Jun 24, 2021 at 8:16 AM Eelco Chaudron <[email protected]> wrote: > Thanks Vasu for the quick turnaround! I found some very small issues. If > you could fix those, I can ack the patch! > > Cheers, > > Eelco > > On 24 Jun 2021, at 12:52, Vasu Dasari wrote: > > > Currently there is an option to add/flush/show ARP/ND neighbor. This > covers L3 > > side. For L2 side, there is only fdb show command. This patch gives an > option > > to add/del an fdb entry via ovs-appctl. > > > > CLI command looks like: > > > > To add: > > ovs-appctl fdb/add <bridge> <port> <vlan> <Mac> > > ovs-appctl fdb/add br0 p1 0 50:54:00:00:00:05 > > > > To del: > > ovs-appctl fdb/del <bridge> <vlan> <Mac> > > ovs-appctl fdb/del br0 0 50:54:00:00:00:05 > > > > Added two new APIs to provide convenient interface to add and delete > static-macs. > > bool xlate_add_static_mac_entry(const struct ofproto_dpif *, ofp_port_t > in_port, > > struct eth_addr dl_src, int vlan); > > bool xlate_delete_static_mac_entry(const struct ofproto_dpif *, > > struct eth_addr dl_src, int vlan); > > > > 1. Static entry should not age. To indicate that entry being programmed > is a static entry, > > 'expires' field in 'struct mac_entry' will be set to a > MAC_ENTRY_AGE_STATIC_ENTRY. A > > check for this value is made while deleting mac entry as part of > regular aging process. > > 2. Another change to of mac-update logic, when a packet with same dl_src > as that of a > > static-mac entry arrives on any port, the logic will not modify the > expires field. > > 3. While flushing fdb entries, made sure static ones are not evicted. > > 4. Updated "ovs-appctl fdb/stats-show br0" to display numberof static > entries in switch > > > > Added following tests: > > ofproto-dpif - static-mac add/del/flush > > ofproto-dpif - static-mac mac moves > > > > Signed-off-by: Vasu Dasari <[email protected]> > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048894.html > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1597752 > > Tested-by: Eelco Chaudron <[email protected]> > > --- > > v1: > > - Fixed 0-day robot warnings > > v2: > > - Fix valgrind error in the modified code in mac_learning_insert() > where a read is > > is performed on e->expires which is not initialized > > v3: > > - Addressed code review comments > > - Added more documentation > > - Fixed mac_entry_age() and is_mac_learning_update_needed() to have > common > > understanding of return values when mac_entry is a static one. > > - Added NEWS item > > v4: > > - Addressed code review comments > > - Static entries will not be purged when fdb/flush is performed. > > - Static entries will not be overwritten when packet with same dl_src > arrives on > > any port of switch > > - Provided bit more detail while doing fdb/add, to indicate if > static-mac is > > overriding already present entry > > - Separated test cases for a bit more clarity > > v5: > > - Addressed code review comments > > - Added new total_static counter to count number of static entries. > > - Removed mac_entry_set_idle_time() > > - Added mac_learning_add_static_entry() and > mac_learning_del_static_entry() > > - Modified APIs xlate_add_static_mac_entry() and > xlate_delete_static_mac_entry() > > return 0 on success else a failure code > > v6: > > - Fixed a probable bug with Eelco's code review comments in > > is_mac_learning_update_needed() > > v7: > > - Added a ovs-vswitchd.8 man page entry for fdb add/del commands > > v8: > > - Updaed with code review comments from Eelco. > > - Renamed total_static to static_entries > > - Added coverage counter mac_learning_static_none_move > > - Fixed a possible bug with static_entries getting cleared via > > fdb/stats-clear command > > - Initialize static_entries in mac_learning_create() > > - Modified fdb/del command by removing option to specify port-name > > - Breakup ofproto_unixctl_fdb_update into ofproto_unixctl_fdb_add > > and ofproto_unixctl_fdb_delete > > - Updated test "static-mac add/del/flush" to have interleaved mac > > entries before fdb/flush > > - Updated test "static-mac mac move" to check for newly added > > coverage counter mac_learning_static_none_move > > --- > > NEWS | 4 + > > lib/mac-learning.c | 155 +++++++++++++++++++++++++++++++---- > > lib/mac-learning.h | 17 ++++ > > ofproto/ofproto-dpif-xlate.c | 48 +++++++++-- > > ofproto/ofproto-dpif-xlate.h | 5 ++ > > ofproto/ofproto-dpif.c | 114 +++++++++++++++++++++++++- > > tests/ofproto-dpif.at | 99 ++++++++++++++++++++++ > > vswitchd/ovs-vswitchd.8.in | 5 ++ > > 8 files changed, 418 insertions(+), 29 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index db3faf4cc..12fb40054 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -20,6 +20,10 @@ Post-v2.15.0 > > - ovsdb-tool: > > * New option '--election-timer' to the 'create-cluster' command to > set the > > leader election timer during cluster creation. > > + - ovs-appctl: > > + * Added ability to add and delete static mac entries using: > > + 'ovs-appctl fdb/add <bridge> <port> <vlan> <mac>' > > + 'ovs-appctl fdb/del <bridge> <vlan> <mac>' > > > > > > v2.15.0 - 15 Feb 2021 > > diff --git a/lib/mac-learning.c b/lib/mac-learning.c > > index 3d5293d3b..234ac7d33 100644 > > --- a/lib/mac-learning.c > > +++ b/lib/mac-learning.c > > @@ -34,13 +34,25 @@ COVERAGE_DEFINE(mac_learning_learned); > > COVERAGE_DEFINE(mac_learning_expired); > > COVERAGE_DEFINE(mac_learning_evicted); > > COVERAGE_DEFINE(mac_learning_moved); > > +COVERAGE_DEFINE(mac_learning_static_none_move); > > > > -/* Returns the number of seconds since 'e' (within 'ml') was last > learned. */ > > +/* > > + * This function will return age of mac entry in the fdb. > > + * It will return either one of the following: > > + * 1. Number of seconds since 'e' (within 'ml') was last learned. > > + * 2. If the mac entry is a static entry, it returns > > + * MAC_ENTRY_AGE_STATIC_ENTRY > > Add . at the end of the sentence for comments. There are a couple, as I > did forget to check it earlier. I should have remembered as Ilya keep on > bugging me about it ;) > > > + */ > > 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 */ > > Add . at the end of the sentence for comments. > > > + if (MAC_ENTRY_AGE_STATIC_ENTRY == e->expires) { > > + return MAC_ENTRY_AGE_STATIC_ENTRY; > > + } else { > > + time_t remaining = e->expires - time_now(); > > + return ml->idle_time - remaining; > > + } > > } > > > > static uint32_t > > @@ -214,6 +226,7 @@ mac_learning_create(unsigned int idle_time) > > ovs_refcount_init(&ml->ref_cnt); > > ovs_rwlock_init(&ml->rwlock); > > mac_learning_clear_statistics(ml); > > + ml->static_entries = 0; > > return ml; > > } > > > > @@ -309,16 +322,19 @@ mac_learning_may_learn(const struct mac_learning > *ml, > > } > > > > /* Searches 'ml' for and returns a MAC learning entry for 'src_mac' in > 'vlan', > > - * inserting a new entry if necessary. The caller must have already > verified, > > - * by calling mac_learning_may_learn(), that 'src_mac' and 'vlan' are > > - * learnable. > > + * inserting a new entry if necessary. If entry being added is a > > + * 1. cache entry: caller must have already verified, by calling > > + * mac_learning_may_learn(), that 'src_mac' and 'vlan' are > learnable. > > + * 2. static entry: new mac static fdb entry will be created or if one > > + * exists already, converts that entry to a static fdb type. > > * > > * If the returned MAC entry is new (that is, if it has a NULL > client-provided > > * port, as returned by mac_entry_get_port()), then the caller must > initialize > > * the new entry's port to a nonnull value with mac_entry_set_port(). */ > > -struct mac_entry * > > -mac_learning_insert(struct mac_learning *ml, > > - const struct eth_addr src_mac, uint16_t vlan) > > +static struct mac_entry * > > +mac_learning_insert__(struct mac_learning *ml, const struct eth_addr > src_mac, > > + uint16_t vlan, bool is_static) > > + OVS_REQ_WRLOCK(ml->rwlock) > > { > > struct mac_entry *e; > > > > @@ -336,8 +352,11 @@ mac_learning_insert(struct mac_learning *ml, > > e->vlan = vlan; > > e->grat_arp_lock = TIME_MIN; > > e->mlport = NULL; > > - COVERAGE_INC(mac_learning_learned); > > - ml->total_learned++; > > + e->expires = 0; > > + if (!is_static) { > > + COVERAGE_INC(mac_learning_learned); > > + ml->total_learned++; > > + } > > } else { > > ovs_list_remove(&e->lru_node); > > } > > @@ -348,11 +367,78 @@ mac_learning_insert(struct mac_learning *ml, > > ovs_list_remove(&e->port_lru_node); > > ovs_list_push_back(&e->mlport->port_lrus, &e->port_lru_node); > > } > > - e->expires = time_now() + ml->idle_time; > > + > > + /* Update 'expires' for mac entry */ > > Add . at the end of the sentence for comments. > > + if (is_static) { > > + /* Increment static_entries only if entry is a new one or entry > is > > + * is converted from cache to static type */ > > > Missing . and double "is". > > > + if (e->expires != MAC_ENTRY_AGE_STATIC_ENTRY) { > > + ml->static_entries++; > > + } > > + e->expires = MAC_ENTRY_AGE_STATIC_ENTRY; > > + } else { > > + e->expires = time_now() + ml->idle_time; > > + } > > > > return e; > > } > > > > +/* Adds a new dynamic mac entry to fdb */ > > Add . at the end of the sentence for comments. > > > +struct mac_entry * > > +mac_learning_insert(struct mac_learning *ml, > > + const struct eth_addr src_mac, uint16_t vlan) > > +{ > > + return mac_learning_insert__(ml, src_mac, vlan, false); > > +} > > + > > +/* Adds a new static mac entry to fdb. It returns pointer to mac_entry > > + * pointing to the fdb entry > > Add . at the end of the sentence for comments. > > > + * > > + * Returns 'true' if mac entry is inserted, 'false' otherwise > > Add . at the end of the sentence for comments. > > + */ > > +bool > > +mac_learning_add_static_entry(struct mac_learning *ml, > > + const struct eth_addr src_mac, uint16_t > vlan, > > + void *in_port) > > + OVS_EXCLUDED(ml->rwlock) > > +{ > > + struct mac_entry *mac = NULL; > > + bool inserted = false; > > + > > + ovs_rwlock_wrlock(&ml->rwlock); > > + mac = mac_learning_insert__(ml, src_mac, vlan, true); > > + if (mac) { > > + mac_entry_set_port(ml, mac, in_port); > > + inserted = true; > > + } > > + ovs_rwlock_unlock(&ml->rwlock); > > + > > + return inserted; > > +} > > + > > +/* Delete a static mac entry from fdb if it exists. > > + * > > + * Returns 'true' if mac entry is found, 'false' otherwise > > Add . at the end of the sentence for comments. > > > + */ > > +bool > > +mac_learning_del_static_entry(struct mac_learning *ml, > > + const struct eth_addr dl_src, uint16_t > vlan) > > +{ > > + struct mac_entry *mac = NULL; > > + bool deleted = false; > > + > > + ovs_rwlock_wrlock(&ml->rwlock); > > + mac = mac_learning_lookup(ml, dl_src, vlan); > > + if (mac && mac_entry_age(ml, mac) == MAC_ENTRY_AGE_STATIC_ENTRY) { > > + mac_learning_expire(ml, mac); > > + ml->static_entries--; > > + deleted = true; > > + } > > + ovs_rwlock_unlock(&ml->rwlock); > > + > > + return deleted; > > +} > > + > > /* Checks whether a MAC learning update is necessary for MAC learning > table > > * 'ml' given that a packet matching 'src' was received on 'in_port' in > 'vlan', > > * and given that the packet was gratuitous ARP if 'is_gratuitous_arp' > is > > @@ -372,13 +458,32 @@ is_mac_learning_update_needed(const struct > mac_learning *ml, > > OVS_REQ_RDLOCK(ml->rwlock) > > { > > struct mac_entry *mac; > > + int age; > > > > if (!mac_learning_may_learn(ml, src, vlan)) { > > return false; > > } > > > > mac = mac_learning_lookup(ml, src, vlan); > > - if (!mac || mac_entry_age(ml, mac)) { > > + /* If mac entry is missing it needs to be added to fdb */ > > Add . at the end of the sentence for comments. > > > + if (!mac) { > > + return true; > > + } > > + > > + age = mac_entry_age(ml, mac); > > + /* if mac is a static entry, then there is no need to update */ > > Add . at the end of the sentence for comments. > > > + if (age == MAC_ENTRY_AGE_STATIC_ENTRY) { > > + /* coverage counter to increment when a packet with same > > + * static-mac appears on a different port */ > > Add . at the end of the sentence for comments. > > > + if (mac_entry_get_port(ml, mac) != in_port) { > > + COVERAGE_INC(mac_learning_static_none_move); > > + } > > + return false; > > + } > > + > > + /* If entry is still alive, just update the mac_entry so, that > expires > > + * gets updated */ > > Add . at the end of the sentence for comments. > > > + if (age > 0) { > > return true; > > } > > > > @@ -513,13 +618,29 @@ mac_learning_expire(struct mac_learning *ml, > struct mac_entry *e) > > free(e); > > } > > > > -/* Expires all the mac-learning entries in 'ml'. */ > > +/* Expires all the dynamic mac-learning entries in 'ml'. */ > > void > > mac_learning_flush(struct mac_learning *ml) > > { > > - struct mac_entry *e; > > - while (get_lru(ml, &e)){ > > - mac_learning_expire(ml, e); > > + struct mac_entry *e, *first_static_mac = NULL; > > + > > + while (get_lru(ml, &e) && (e != first_static_mac)) { > > + > > + /* static-mac should not be evicted */ > > Add . at the end of the sentence for comments. > > > + 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() > */ > > Add . at the end of the sentence for comments. > > > + if (!first_static_mac) { > > + first_static_mac = e; > > + } > > + > > + /* Remove from lru head and append it to tail */ > > Add . at the end of the sentence for comments. > > > + ovs_list_remove(&e->lru_node); > > + ovs_list_push_back(&ml->lrus, &e->lru_node); > > + } else { > > + mac_learning_expire(ml, e); > > + } > > } > > hmap_shrink(&ml->table); > > } > > diff --git a/lib/mac-learning.h b/lib/mac-learning.h > > index 0ddab06cb..f391a8bb2 100644 > > --- a/lib/mac-learning.h > > +++ b/lib/mac-learning.h > > @@ -57,6 +57,11 @@ > > * list starting from the LRU end, deleting each entry that has been > idle too > > * long. > > * > > + * Fourth, a mac entry can be configured statically via API or appctl > commands. > > + * Static entries are programmed to have an age of > MAC_ENTRY_AGE_STATIC_ENTRY. > > + * Age of static entries will not be updated by a receiving packet as > part of > > + * regular packet processing. > > + * > > * Finally, the number of MAC learning table entries has a configurable > maximum > > * size to prevent memory exhaustion. When a new entry must be > inserted but > > * the table is already full, the implementation uses an eviction > strategy > > @@ -94,6 +99,9 @@ struct mac_learning; > > /* Time, in seconds, before expiring a mac_entry due to inactivity. */ > > #define MAC_ENTRY_DEFAULT_IDLE_TIME 300 > > > > +/* Age value to represent a static entry */ > > Add . at the end of the sentence for comments. > > > +#define MAC_ENTRY_AGE_STATIC_ENTRY INT_MAX > > + > > /* Time, in seconds, to lock an entry updated by a gratuitous ARP to > avoid > > * relearning based on a reflection from a bond member. */ > > #define MAC_GRAT_ARP_LOCK_TIME 5 > > @@ -156,6 +164,7 @@ struct mac_learning { > > unsigned long *flood_vlans; /* Bitmap of learning disabled VLANs. */ > > unsigned int idle_time; /* Max age before deleting an entry. */ > > size_t max_entries; /* Max number of learned MACs. */ > > + size_t static_entries; /* Current number of static MAC > entries. */ > > struct ovs_refcount ref_cnt; > > struct ovs_rwlock rwlock; > > bool need_revalidate; > > @@ -218,6 +227,14 @@ bool mac_learning_update(struct mac_learning *ml, > struct eth_addr src, > > int vlan, bool is_gratuitous_arp, bool is_bond, > > void *in_port) > > OVS_EXCLUDED(ml->rwlock); > > +bool mac_learning_add_static_entry(struct mac_learning *ml, > > + const struct eth_addr src, > > + uint16_t vlan, void *in_port) > > + OVS_EXCLUDED(ml->rwlock); > > +bool mac_learning_del_static_entry(struct mac_learning *ml, > > + const struct eth_addr src, > > + uint16_t vlan) > > + OVS_EXCLUDED(ml->rwlock); > > > > /* Lookup. */ > > struct mac_entry *mac_learning_lookup(const struct mac_learning *ml, > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > > index a6f4ea334..6f744bb25 100644 > > --- a/ofproto/ofproto-dpif-xlate.c > > +++ b/ofproto/ofproto-dpif-xlate.c > > @@ -7991,26 +7991,58 @@ xlate_send_packet(const struct ofport_dpif > *ofport, bool oam, > > ofpacts.data, ofpacts.size, > packet); > > } > > > > -void > > -xlate_mac_learning_update(const struct ofproto_dpif *ofproto, > > - ofp_port_t in_port, struct eth_addr dl_src, > > - int vlan, bool is_grat_arp) > > +/* Get xbundle for a ofp_port in a ofproto datapath */ > > Add . at the end of the sentence for comments. > > > +static struct xbundle* > > +ofp_port_to_xbundle(const struct ofproto_dpif *ofproto, ofp_port_t > ofp_port) > > { > > struct xlate_cfg *xcfg = ovsrcu_get(struct xlate_cfg *, &xcfgp); > > struct xbridge *xbridge; > > - struct xbundle *xbundle; > > > > xbridge = xbridge_lookup(xcfg, ofproto); > > if (!xbridge) { > > - return; > > + return NULL; > > } > > > > - xbundle = lookup_input_bundle__(xbridge, in_port, NULL); > > + return lookup_input_bundle__(xbridge, ofp_port, NULL); > > +} > > + > > +void > > +xlate_mac_learning_update(const struct ofproto_dpif *ofproto, > > + ofp_port_t in_port, struct eth_addr dl_src, > > + int vlan, bool is_grat_arp) > > +{ > > + struct xbundle *xbundle = NULL; > > + > > + xbundle = ofp_port_to_xbundle(ofproto, in_port); > > if (!xbundle) { > > return; > > } > > > > - update_learning_table__(xbridge, xbundle, dl_src, vlan, > is_grat_arp); > > + update_learning_table__(xbundle->xbridge, > > + xbundle, dl_src, vlan, is_grat_arp); > > +} > > + > > +bool > > +xlate_add_static_mac_entry(const struct ofproto_dpif *ofproto, > > + ofp_port_t in_port, > > + struct eth_addr dl_src, int vlan) > > +{ > > + struct xbundle *xbundle = ofp_port_to_xbundle(ofproto, in_port); > > + > > + /* Return here if xbundle */ > > Add . at the end of the sentence for comments. > > > + if (!xbundle || (xbundle == &ofpp_none_bundle)) { > > + return false; > > + } > > + > > + return mac_learning_add_static_entry(ofproto->ml, dl_src, vlan, > > + xbundle->ofbundle); > > +} > > + > > +bool > > +xlate_delete_static_mac_entry(const struct ofproto_dpif *ofproto, > > + struct eth_addr dl_src, int vlan) > > +{ > > + return mac_learning_del_static_entry(ofproto->ml, dl_src, vlan); > > } > > > > void > > diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h > > index 3426a27b2..851088d79 100644 > > --- a/ofproto/ofproto-dpif-xlate.h > > +++ b/ofproto/ofproto-dpif-xlate.h > > @@ -225,6 +225,11 @@ int xlate_send_packet(const struct ofport_dpif *, > bool oam, struct dp_packet *); > > void xlate_mac_learning_update(const struct ofproto_dpif *ofproto, > > ofp_port_t in_port, struct eth_addr > dl_src, > > int vlan, bool is_grat_arp); > > +bool xlate_add_static_mac_entry(const struct ofproto_dpif *, > > + ofp_port_t in_port, > > + struct eth_addr dl_src, int vlan); > > +bool xlate_delete_static_mac_entry(const struct ofproto_dpif *, > > + struct eth_addr dl_src, int vlan); > > > > void xlate_set_support(const struct ofproto_dpif *, > > const struct dpif_backer_support *); > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 47db9bb57..52ba72df0 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -5854,15 +5854,114 @@ ofproto_unixctl_fdb_show(struct unixctl_conn > *conn, int argc OVS_UNUSED, > > LIST_FOR_EACH (e, lru_node, &ofproto->ml->lrus) { > > struct ofbundle *bundle = mac_entry_get_port(ofproto->ml, e); > > char name[OFP_MAX_PORT_NAME_LEN]; > > + int age = mac_entry_age(ofproto->ml, e); > > > > ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port, > > - NULL, name, sizeof name); > > - ds_put_format(&ds, "%5s %4d "ETH_ADDR_FMT" %3d\n", > > - name, e->vlan, ETH_ADDR_ARGS(e->mac), > > - mac_entry_age(ofproto->ml, e)); > > + NULL, name, sizeof name); > > + ds_put_format(&ds, "%5s %4d "ETH_ADDR_FMT" ", > > + name, e->vlan, ETH_ADDR_ARGS(e->mac)); > > + if (MAC_ENTRY_AGE_STATIC_ENTRY == age) { > > + ds_put_format(&ds, "static\n"); > > + } else { > > + ds_put_format(&ds, "%3d\n", age); > > + } > > + } > > + ovs_rwlock_unlock(&ofproto->ml->rwlock); > > + unixctl_command_reply(conn, ds_cstr(&ds)); > > + ds_destroy(&ds); > > +} > > + > > +static void > > +ofproto_unixctl_fdb_add(struct unixctl_conn *conn, int argc OVS_UNUSED, > > + const char *argv[], void *aux OVS_UNUSED) > > +{ > > + const char *br_name = argv[1]; > > + const char *port_name = argv[2]; > > + struct eth_addr mac; > > + uint16_t vlan = atoi(argv[3]); > > + const struct ofproto_dpif *ofproto; > > + ofp_port_t in_port = OFPP_NONE; > > + struct ofproto_port ofproto_port; > > + struct ds ds = DS_EMPTY_INITIALIZER; > > + const struct mac_entry *mac_entry; > > + const struct ofbundle *bundle = NULL; > > + int age; > > + > > + ofproto = ofproto_dpif_lookup_by_name(br_name); > > + if (!ofproto) { > > + unixctl_command_reply_error(conn, "no such bridge"); > > + return; > > + } > > + > > + if (!eth_addr_from_string(argv[4], &mac)) { > > + unixctl_command_reply_error(conn, "bad MAC address"); > > + return; > > + } > > + > > + if (ofproto_port_query_by_name(&ofproto->up, port_name, > &ofproto_port)) { > > + unixctl_command_reply_error(conn, > > + "software error, odp port is present but no ofp port"); > > + return; > > + } > > + in_port = ofproto_port.ofp_port; > > + ofproto_port_destroy(&ofproto_port); > > + > > + /* Give a bit more information if the entry being added is > overriding > > + * an existing entry */ > > Add . at the end of the sentence for comments. > > + > > + 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; > > mac_entry is none NULL so no need to check it again. > > > + age = mac_entry->expires; > > } > > ovs_rwlock_unlock(&ofproto->ml->rwlock); > > + > > + if (bundle && ((strcmp(bundle->name, port_name)) || > > + (age != MAC_ENTRY_AGE_STATIC_ENTRY))) { > > Indentation is off by 3 spaces. > > > + char old_port_name[OFP_MAX_PORT_NAME_LEN]; > > + > > + ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port, > > + NULL, old_port_name, sizeof old_port_name); > > + ds_put_format(&ds, "Overriding already existing %s entry on > %s\n", > > + (age == MAC_ENTRY_AGE_STATIC_ENTRY) ? "static" : > "dynamic", > > + old_port_name); > > + } > > + > > + if (!xlate_add_static_mac_entry(ofproto, in_port, mac, vlan)) { > > + ds_put_format(&ds, "could not add static mac entry\n"); > > Why no using unixctl_command_reply_error() like for the other failures? > > > + } > > unixctl_command_reply(conn, ds_cstr(&ds)); > > + > > + ds_destroy(&ds); > > +} > > + > > +static void > > +ofproto_unixctl_fdb_delete(struct unixctl_conn *conn, int argc > OVS_UNUSED, > > + const char *argv[], void *aux OVS_UNUSED) > > +{ > > + const char *br_name = argv[1]; > > + struct eth_addr mac; > > + uint16_t vlan = atoi(argv[2]); > > + const struct ofproto_dpif *ofproto; > > + struct ds ds = DS_EMPTY_INITIALIZER; > > + > > + ofproto = ofproto_dpif_lookup_by_name(br_name); > > + if (!ofproto) { > > + unixctl_command_reply_error(conn, "no such bridge"); > > + return; > > + } > > + > > + if (!eth_addr_from_string(argv[3], &mac)) { > > + unixctl_command_reply_error(conn, "bad MAC address"); > > + return; > > + } > > + if (!xlate_delete_static_mac_entry(ofproto, mac, vlan)) { > > + ds_put_format(&ds, "could not find static mac entry\n"); > > Why no using unixctl_command_reply_error() like for the other failures? > If you do, you can also remove the ds variable and just call > unixctl_command_reply() below with NULL. > > > + } > > + unixctl_command_reply(conn, ds_cstr(&ds)); > > + > > ds_destroy(&ds); > > } > > > > @@ -5914,6 +6013,9 @@ ofproto_unixctl_fdb_stats_show(struct unixctl_conn > *conn, int argc OVS_UNUSED, > > ds_put_format(&ds, > > " Total number of learned MAC entries : > %"PRIu64"\n", > > ofproto->ml->total_learned); > > + ds_put_format(&ds, > > + " Current static MAC entries in the table : > %"PRIuSIZE"\n", > > + ofproto->ml->static_entries); > > > Can you move this up, as the output now looks like this: > > Current/maximum MAC entries in the table: 17/8192 > Total number of learned MAC entries : 18 > Current static MAC entries in the table : 17 > Total number of expired MAC entries : 0 > Total number of evicted MAC entries : 0 > Total number of port moved MAC entries : 0 > > I think it should look like this: > > Current/maximum MAC entries in the table: 17/8192 > Current static MAC entries in the table : 17 > Total number of learned MAC entries : 18 > Total number of expired MAC entries : 0 > Total number of evicted MAC entries : 0 > Total number of port moved MAC entries : 0 > > > ds_put_format(&ds, > > " Total number of expired MAC entries : > %"PRIu64"\n", > > ofproto->ml->total_expired); > > @@ -6417,6 +6519,10 @@ ofproto_unixctl_init(void) > > } > > registered = true; > > > > + unixctl_command_register("fdb/add", "[bridge port vlan mac]", 4, 4, > > + ofproto_unixctl_fdb_add, NULL); > > + unixctl_command_register("fdb/del", "[bridge vlan mac]", 3, 3, > > + ofproto_unixctl_fdb_delete, NULL); > > unixctl_command_register("fdb/flush", "[bridge]", 0, 1, > > ofproto_unixctl_fdb_flush, NULL); > > unixctl_command_register("fdb/show", "bridge", 1, 1, > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > > index 31064ed95..bc91fc9a8 100644 > > --- a/tests/ofproto-dpif.at > > +++ b/tests/ofproto-dpif.at > > @@ -6753,6 +6753,105 @@ PORTNAME > > portName=p2 > > ])]) > > > > +AT_SETUP([ofproto-dpif - static-mac add/del/flush]) > > +OVS_VSWITCHD_START([set bridge br0 fail-mode=standalone]) > > +add_of_ports br0 1 2 > > + > > +dnl Generate some dynamic fdb entries on some ports > > +OFPROTO_TRACE([ovs-dummy], [in_port(1),eth(src=50:54:00:00:00:01)], > [-generate], [100,2]) > > +OFPROTO_TRACE([ovs-dummy], [in_port(2),eth(src=50:54:00:00:00:02)], > [-generate], [100,1]) > > + > > +dnl Add some static mac entries > > +AT_CHECK([ovs-appctl fdb/add br0 p1 0 50:54:00:00:01:01]) > > +AT_CHECK([ovs-appctl fdb/add br0 p2 0 50:54:00:00:02:02]) > > + > > +dnl Check initial fdb > > +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ *[[0-9]]\{1,\}$//' > | grep -v port | sort], [0], [dnl > > + 1 0 50:54:00:00:00:01 > > + 1 0 50:54:00:00:01:01 static > > + 2 0 50:54:00:00:00:02 > > + 2 0 50:54:00:00:02:02 static > > +]) > > + > > +dnl Remove static mac entry > > +AT_CHECK([ovs-appctl fdb/del br0 0 50:54:00:00:01:01]) > > + > > +dnl Check that entry is removed > > +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | grep "50:54:00:00:01:01"], > [1], [dnl > > +]) > > + > > +# Add some more cache and static entries, to test out flush operation > > +for i in 0 1 2 3 4 5 6 7 8 9 a b c d e f; do > > + ovs-appctl ofproto/trace ovs-dummy > "in_port(1),eth(src=50:54:00:00:0$i:ff)" -generate > > + ovs-appctl fdb/add br0 p2 0 50:54:00:0$i:00:ff > > +done > > + > > +for i in 0 1 2 3 4 5 6 7 8 9 a b c d e f; do > > + ovs-appctl fdb/add br0 p2 0 50:54:00:0$i:00:ff > > +done > > + > > +dnl Flush mac entries, only dynamic ones should be evicted. > > +AT_CHECK([ovs-appctl fdb/flush br0], [0], [dnl > > +table successfully flushed > > +]) > > + > > +dnl Count number of static entries remaining > > +AT_CHECK_UNQUOTED([ovs-appctl fdb/stats-show br0 | grep static], [0], > [dnl > > + Current static MAC entries in the table : 17 > > +]) > > + > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > + > > +AT_SETUP([ofproto-dpif - static-mac mac moves]) > > +OVS_VSWITCHD_START([set bridge br0 fail-mode=standalone]) > > +add_of_ports br0 1 2 > > + > > +dnl Generate a dynamic entry > > +OFPROTO_TRACE([ovs-dummy], [in_port(1),eth(src=50:54:00:00:00:00)], > [-generate], [100,2]) > > + > > +dnl Convert dynamically learnt dl_src to a static-mac > > +AT_CHECK([ovs-appctl fdb/add br0 p1 0 50:54:00:00:00:00], [0], [dnl > > +Overriding already existing dynamic entry on 1 > > +]) > > + > > +dnl Check fdb > > +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ *[[0-9]]\{1,\}$//' > | grep -v port | sort], [0], [dnl > > + 1 0 50:54:00:00:00:00 static > > +]) > > + > > +dnl Move the static mac to different port > > +AT_CHECK([ovs-appctl fdb/add br0 p2 0 50:54:00:00:00:00], [0], [dnl > > +Overriding already existing static entry on 1 > > +]) > > + > > +dnl Check fdb > > +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ *[[0-9]]\{1,\}$//' > | grep -v port | sort], [0], [dnl > > + 2 0 50:54:00:00:00:00 static > > +]) > > + > > +dnl static-mac should not be converted to a dynamic one when a packet > with same dl_src > > +dnl arrives on any port of the switch > > +dnl Packet arriving on p1 > > +OFPROTO_TRACE([ovs-dummy], [in_port(1),eth(src=50:54:00:00:00:00)], > [-generate], [100,2]) > > +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ *[[0-9]]\{1,\}$//' > | grep -v port | sort], [0], [dnl > > + 2 0 50:54:00:00:00:00 static > > +]) > > + > > +dnl Packet arriving on p2 > > +OFPROTO_TRACE([ovs-dummy], [in_port(2),eth(src=50:54:00:00:00:00)], > [-generate], [100,1]) > > +AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed 's/ *[[0-9]]\{1,\}$//' > | grep -v port | sort], [0], [dnl > > + 2 0 50:54:00:00:00:00 static > > +]) > > + > > +dnl Check mac_move coverage counter mac_learning_static_none_move > > +AT_CHECK([ovs-appctl coverage/read-counter > mac_learning_static_none_move], [0], [dnl > > +1 > > +]) > > + > > +OVS_VSWITCHD_STOP > > +AT_CLEANUP > > + > > AT_SETUP([ofproto-dpif - basic truncate action]) > > OVS_VSWITCHD_START > > add_of_ports br0 1 2 3 4 5 > > diff --git a/vswitchd/ovs-vswitchd.8.in b/vswitchd/ovs-vswitchd.8.in > > index 50dad7208..08920600e 100644 > > --- a/vswitchd/ovs-vswitchd.8.in > > +++ b/vswitchd/ovs-vswitchd.8.in > > @@ -159,6 +159,11 @@ If \fIbridge\fR is not specified, then displays > detailed information about all > > bridges with RSTP enabled. > > .SS "BRIDGE COMMANDS" > > These commands manage bridges. > > +.IP "\fBfdb/add\fR \fIbridge\fR \fIport\fR \fIvlan\fR \fImac\fR" > > +.IQ "\fBfdb/del\fR \fIbridge\fR \fIport\fR \fIvlan\fR \fImac\fR" > > +Adds/deletes \fImac\fR address on a \fIbridge\fR to/from \fIport\fR and > > +\fIvlan\fR. This utility is can be used to pre-populate fdb table > without > > +relying on dynamic mac learning. > > .IP "\fBfdb/flush\fR [\fIbridge\fR]" > > Flushes \fIbridge\fR MAC address learning table, or all learning tables > > if no \fIbridge\fR is given. > > -- > > 2.29.2 > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
