Hi Eelco,
Thanks for reviewing.
The latest patch will be update soon.
> -----Original Message-----
> From: Eelco Chaudron <echau...@redhat.com>
> Sent: Wednesday, October 19, 2022 8:06 PM
> To: mit...@outlook.com
> Cc: ovs-dev@openvswitch.org; i.maxim...@ovn.org; vdas...@gmail.com; Lin
> Huang <linhu...@ruijie.com.cn>; Zhang Yuhuang
> <zhangyuhu...@ruijie.com.cn>
> Subject: Re: [PATCH] mac-learning: Fix learned fdb entries are not age out
> issue.
>
>
>
> On 29 Jul 2022, at 13:16, mit...@outlook.com wrote:
>
> > From: Lin Huang <linhu...@ruijie.com.cn>
> >
> > After user add a static fdb entry, the get_lru() function will always
> > return the
> > static fdb entry. That's normal fdb entries will not age out through
> mac_learning_run().
> >
> > Fix the issue by modify the get_lru() function to check the entry->expires
> field and
> > not return the entry which entry->expires is
> MAC_ENTRY_AGE_STATIC_ENTRY.
> >
> > Adding a unit test for this.
>
> Sorry for the late response, but it was dropped off of my review list :(
>
> One small nit, and a suggested test case change.
>
> The rest looks good!
>
> //Eelco
>
>
> > Fixes: ccc24fc88d59 ("ofproto-dpif: APIs and CLI option to add/delete static
> fdb entry.")
> > Tested-by: Zhang Yuhuang <zhangyuhu...@ruijie.com.cn>
> > Signed-off-by: Lin Huang <linhu...@ruijie.com.cn>
> > ---
> > lib/mac-learning.c | 37 ++++++++++++++-----------------------
> > tests/ofproto-dpif.at | 23 +++++++++++++++++++++++
> > 2 files changed, 37 insertions(+), 23 deletions(-)
> >
> > diff --git a/lib/mac-learning.c b/lib/mac-learning.c
> > index a60794fb2..713f81e63 100644
> > --- a/lib/mac-learning.c
> > +++ b/lib/mac-learning.c
> > @@ -175,13 +175,19 @@ static bool
> > get_lru(struct mac_learning *ml, struct mac_entry **e)
> > OVS_REQ_RDLOCK(ml->rwlock)
> > {
> > + struct mac_entry *entry;
>
> nit: the entry declaration can be moved inside the if statement.
>
> > +
> > if (!ovs_list_is_empty(&ml->lrus)) {
> > - *e = mac_entry_from_lru_node(ml->lrus.next);
> > - return true;
> > - } else {
> > - *e = NULL;
> > - return false;
> > + LIST_FOR_EACH (entry, lru_node, &ml->lrus) {
> > + if (entry->expires != MAC_ENTRY_AGE_STATIC_ENTRY) {
> > + *e = entry;
> > + return true;
> > + }
> > + }
> > }
> > +
> > + *e = NULL;
> > + return false;
> > }
> >
> > static unsigned int
> > @@ -618,25 +624,10 @@ mac_learning_expire(struct mac_learning *ml,
> struct mac_entry *e)
> > void
> > mac_learning_flush(struct mac_learning *ml)
> > {
> > - 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;
> > - }
> > + struct mac_entry *e;
> >
> > - /* Remove from lru head and append it to tail. */
> > - ovs_list_remove(&e->lru_node);
> > - ovs_list_push_back(&ml->lrus, &e->lru_node);
> > - } else {
> > - mac_learning_expire(ml, e);
> > - }
> > + while (get_lru(ml, &e)) {
> > + mac_learning_expire(ml, e);
> > }
> > hmap_shrink(&ml->table);
> > }
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index 2e91ae1a1..4b455315d 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -7195,6 +7195,29 @@ AT_CHECK([ovs-appctl coverage/read-counter
> mac_learning_static_none_move], [0],
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +AT_SETUP([ofproto-dpif - static-mac learned mac age out])
> > +OVS_VSWITCHD_START([set bridge br0 fail-mode=standalone -- set bridge
> br0 other_config:mac-aging-time=5])
>
> 15 is the minimum value, so you might as well use it here. With the warp
> command below, you can even leave it at the default.
>
> > +add_of_ports br0 1 2
> > +
> > +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 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 Waiting for aging out.
> > +sleep 16
>
> Don’t wait 16 seconds, this will only cause the test run to take longer. Use
> the
> “ovs-appctl time/warp 20000” command here.
>
> > +dnl Count number of static entries remaining.
> > +AT_CHECK_UNQUOTED([ovs-appctl fdb/stats-show br0 | grep expired], [0],
> [dnl
> > + Total number of expired MAC entries : 2
> > +])
> > +
> > +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.32.0.windows.2
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
