[ovs-dev] [PATCH v8] ofproto-dpif: APIs and CLI option to add/delete static fdb entry
Vasu Dasari
vdasari at gmail.com
Thu Jun 24 16:00:38 UTC 2021
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 <echaudro at redhat.com> 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 <vdasari at gmail.com>
> > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2019-June/048894.html
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1597752
> > Tested-by: Eelco Chaudron <echaudro at redhat.com>
> > ---
> > v1:
> > - Fixed 0-day robot warnings
> > v2:
> > - Fix valgrind error in the modified code in mac_learning_insert()
> where a read is
> > is performed on e->expires which is not initialized
> > v3:
> > - Addressed code review comments
> > - Added more documentation
> > - Fixed mac_entry_age() and is_mac_learning_update_needed() to have
> common
> > understanding of return values when mac_entry is a static one.
> > - Added NEWS item
> > v4:
> > - Addressed code review comments
> > - Static entries will not be purged when fdb/flush is performed.
> > - Static entries will not be overwritten when packet with same dl_src
> arrives on
> > any port of switch
> > - Provided bit more detail while doing fdb/add, to indicate if
> static-mac is
> > overriding already present entry
> > - Separated test cases for a bit more clarity
> > 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
>
>
More information about the dev
mailing list