diff mbox series

[ovs-dev] mac-learning: Fix learned fdb entries are not age out issue.

Message ID MEYP282MB349994926F722F2E892FC084CD999@MEYP282MB3499.AUSP282.PROD.OUTLOOK.COM
State Changes Requested
Headers show
Series [ovs-dev] mac-learning: Fix learned fdb entries are not age out issue. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

miter July 29, 2022, 11:16 a.m. UTC
From: Lin Huang <linhuang@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.

Fixes: ccc24fc88d59 ("ofproto-dpif: APIs and CLI option to add/delete static fdb entry.")
Tested-by: Zhang Yuhuang <zhangyuhuang@ruijie.com.cn>
Signed-off-by: Lin Huang <linhuang@ruijie.com.cn>
---
 lib/mac-learning.c    | 37 ++++++++++++++-----------------------
 tests/ofproto-dpif.at | 23 +++++++++++++++++++++++
 2 files changed, 37 insertions(+), 23 deletions(-)

Comments

miter Aug. 3, 2022, 1:52 p.m. UTC | #1
Hi Vasu & Eelco,

Pls review my patch.

Thanks a lot.

> -----Original Message-----
> From: lin huang <miterv@outlook.com>
> Sent: Friday, July 29, 2022 7:17 PM
> To: ovs-dev@openvswitch.org; echaudro@redhat.com; i.maximets@ovn.org;
> vdasari@gmail.com
> Cc: Lin Huang <linhuang@ruijie.com.cn>; Zhang Yuhuang
> <zhangyuhuang@ruijie.com.cn>
> Subject: [PATCH] mac-learning: Fix learned fdb entries are not age out issue.
> 
> From: Lin Huang <linhuang@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.
> 
> Fixes: ccc24fc88d59 ("ofproto-dpif: APIs and CLI option to add/delete static
> fdb entry.")
> Tested-by: Zhang Yuhuang <zhangyuhuang@ruijie.com.cn>
> Signed-off-by: Lin Huang <linhuang@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;
> +
>      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])
> +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
> +
> +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
Eelco Chaudron Aug. 23, 2022, 8:19 a.m. UTC | #2
On 3 Aug 2022, at 15:52, lin huang wrote:

> Hi Vasu & Eelco,
>
> Pls review my patch.

Just returned from PTO, will try to add this to my next week’s list of things to review.

//Eelco


>> -----Original Message-----
>> From: lin huang <miterv@outlook.com>
>> Sent: Friday, July 29, 2022 7:17 PM
>> To: ovs-dev@openvswitch.org; echaudro@redhat.com; i.maximets@ovn.org;
>> vdasari@gmail.com
>> Cc: Lin Huang <linhuang@ruijie.com.cn>; Zhang Yuhuang
>> <zhangyuhuang@ruijie.com.cn>
>> Subject: [PATCH] mac-learning: Fix learned fdb entries are not age out issue.
>>
>> From: Lin Huang <linhuang@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.
>>
>> Fixes: ccc24fc88d59 ("ofproto-dpif: APIs and CLI option to add/delete static
>> fdb entry.")
>> Tested-by: Zhang Yuhuang <zhangyuhuang@ruijie.com.cn>
>> Signed-off-by: Lin Huang <linhuang@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;
>> +
>>      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])
>> +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
>> +
>> +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
miter Sept. 25, 2022, 10:48 a.m. UTC | #3
Hi Vasu,

Pls review my patch.

Thanks a lot.

> -----Original Message-----
> From: lin huang <miterv@outlook.com>
> Sent: Friday, July 29, 2022 7:17 PM
> To: ovs-dev@openvswitch.org; echaudro@redhat.com; i.maximets@ovn.org;
> vdasari@gmail.com
> Cc: Lin Huang <linhuang@ruijie.com.cn>; Zhang Yuhuang
> <zhangyuhuang@ruijie.com.cn>
> Subject: [PATCH] mac-learning: Fix learned fdb entries are not age out issue.
> 
> From: Lin Huang <linhuang@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.
> 
> Fixes: ccc24fc88d59 ("ofproto-dpif: APIs and CLI option to add/delete static
> fdb entry.")
> Tested-by: Zhang Yuhuang <zhangyuhuang@ruijie.com.cn>
> Signed-off-by: Lin Huang <linhuang@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;
> +
>      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])
> +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
> +
> +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
Eelco Chaudron Oct. 19, 2022, 12:06 p.m. UTC | #4
On 29 Jul 2022, at 13:16, miterv@outlook.com wrote:

> From: Lin Huang <linhuang@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 <zhangyuhuang@ruijie.com.cn>
> Signed-off-by: Lin Huang <linhuang@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
miter Oct. 20, 2022, 3:03 p.m. UTC | #5
Hi Eelco,

Thanks for reviewing.

The latest patch will be update soon.

> -----Original Message-----
> From: Eelco Chaudron <echaudro@redhat.com>
> Sent: Wednesday, October 19, 2022 8:06 PM
> To: miterv@outlook.com
> Cc: ovs-dev@openvswitch.org; i.maximets@ovn.org; vdasari@gmail.com; Lin
> Huang <linhuang@ruijie.com.cn>; Zhang Yuhuang
> <zhangyuhuang@ruijie.com.cn>
> Subject: Re: [PATCH] mac-learning: Fix learned fdb entries are not age out
> issue.
> 
> 
> 
> On 29 Jul 2022, at 13:16, miterv@outlook.com wrote:
> 
> > From: Lin Huang <linhuang@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 <zhangyuhuang@ruijie.com.cn>
> > Signed-off-by: Lin Huang <linhuang@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
diff mbox series

Patch

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;
+
     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])
+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
+
+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