diff mbox series

[ovs-dev,v3,2/2] ofproto-dpif: avoid unneccesary backer revalidation

Message ID 2022020510581518971432@chinatelecom.cn
State Superseded
Headers show
Series fix dpif backer revalidation | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test fail github build: failed

Commit Message

Cheng Li Feb. 5, 2022, 2:58 a.m. UTC 
If lldp didn't change, we are not supposed to trigger backer
revalidation.

Without this patch, bridge_reconfigure() always trigger udpif
revalidator because of lldp.

Signed-off-by: lic121 <lic121@chinatelecom.cn>
Co-authored-by: Eelco Chaudron <echaudro@redhat.com>
---
 lib/ovs-lldp.c         |  8 ++++++++
 lib/ovs-lldp.h         |  1 +
 ofproto/ofproto-dpif.c |  5 ++++-
 tests/ofproto-dpif.at  | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 1 deletion(-)

--
1.8.3.1

Comments

Cheng Li Feb. 6, 2022, 3:58 a.m. UTC | #1 
>If lldp didn't change, we are not supposed to trigger backer
>revalidation.
>
>Without this patch, bridge_reconfigure() always trigger udpif
>revalidator because of lldp.
>
>Signed-off-by: lic121 <lic121@chinatelecom.cn>
>Co-authored-by: Eelco Chaudron <echaudro@redhat.com>
>---
> lib/ovs-lldp.c         |  8 ++++++++
> lib/ovs-lldp.h         |  1 +
> ofproto/ofproto-dpif.c |  5 ++++-
> tests/ofproto-dpif.at  | 33 +++++++++++++++++++++++++++++++++
> 4 files changed, 46 insertions(+), 1 deletion(-)
>
>diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
>index 162311f..bfc69a3 100644
>--- a/lib/ovs-lldp.c
>+++ b/lib/ovs-lldp.c
>@@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
>     ovs_mutex_unlock(&mutex);
> }
>
>+/* Is LLDP enabled?
>+ */
>+bool
>+lldp_is_enabled(struct lldp *lldp)
>+{
>+    return lldp ? lldp->enabled : false;
>+}
>+
> /* Configures the LLDP stack.
>  */
> bool
>diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
>index 0e536e8..661ac4e 100644
>--- a/lib/ovs-lldp.h
>+++ b/lib/ovs-lldp.h
>@@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
> bool lldp_should_send_packet(struct lldp *cfg);
> bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
> bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
>+bool lldp_is_enabled(struct lldp *lldp);
> void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
> void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
>                      const struct eth_addr eth_src);
>diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>index ced67b0..c2f5b12 100644
>--- a/ofproto/ofproto-dpif.c
>+++ b/ofproto/ofproto-dpif.c
>@@ -2499,11 +2499,11 @@ set_lldp(struct ofport *ofport_,
> {
>     struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
>     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
>+    bool old_enable = lldp_is_enabled(ofport->lldp);
>     int error = 0;
>
>     if (cfg) {
>         if (!ofport->lldp) {
>-            ofproto->backer->need_revalidate = REV_RECONFIGURE;
>             ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
>         }
>
>@@ -2515,6 +2515,9 @@ set_lldp(struct ofport *ofport_,
>     } else if (ofport->lldp) {
>         lldp_unref(ofport->lldp);
>         ofport->lldp = NULL;
>+    }
>+
>+    if (lldp_is_enabled(ofport->lldp) != old_enable) {
>         ofproto->backer->need_revalidate = REV_RECONFIGURE;
>     }
>
>diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>index 7f273e5..edb6aa1 100644
>--- a/tests/ofproto-dpif.at
>+++ b/tests/ofproto-dpif.at
>@@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
>+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
>+OVS_VSWITCHD_START(
>+    [add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
>+)
>+dnl first revalidation triggered by add interface
>+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>+1
>+])
>+
>+dnl enable lldp
>+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])

CI test shows that setting lldp cause memory leaks, will try to find out the places.

>+AT_CHECK([ovs-appctl revalidator/wait])
>+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>+2
>+])
>+
>+dnl disable lldp
>+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
>+AT_CHECK([ovs-appctl revalidator/wait])
>+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>+3
>+])
>+
>+dnl remove lldp, no revalidation as lldp was disabled
>+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
>+AT_CHECK([ovs-appctl revalidator/wait])
>+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>+3
>+])
>+
>+OVS_VSWITCHD_STOP
>+AT_CLEANUP
>+
> AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
>
> dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
>--
>1.8.3.1
>
Eelco Chaudron Feb. 7, 2022, 10:30 a.m. UTC | #2 
On 6 Feb 2022, at 4:58, lic121 wrote:

>> If lldp didn't change, we are not supposed to trigger backer
>> revalidation.
>>
>> Without this patch, bridge_reconfigure() always trigger udpif
>> revalidator because of lldp.
>>
>> Signed-off-by: lic121 <lic121@chinatelecom.cn>
>> Co-authored-by: Eelco Chaudron <echaudro@redhat.com>
>> ---
>> lib/ovs-lldp.c         |  8 ++++++++
>> lib/ovs-lldp.h         |  1 +
>> ofproto/ofproto-dpif.c |  5 ++++-
>> tests/ofproto-dpif.at  | 33 +++++++++++++++++++++++++++++++++
>> 4 files changed, 46 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
>> index 162311f..bfc69a3 100644
>> --- a/lib/ovs-lldp.c
>> +++ b/lib/ovs-lldp.c
>> @@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
>>     ovs_mutex_unlock(&mutex);
>> }
>>
>> +/* Is LLDP enabled?
>> + */
>> +bool
>> +lldp_is_enabled(struct lldp *lldp)
>> +{
>> +    return lldp ? lldp->enabled : false;
>> +}
>> +
>> /* Configures the LLDP stack.
>>  */
>> bool
>> diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
>> index 0e536e8..661ac4e 100644
>> --- a/lib/ovs-lldp.h
>> +++ b/lib/ovs-lldp.h
>> @@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
>> bool lldp_should_send_packet(struct lldp *cfg);
>> bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
>> bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
>> +bool lldp_is_enabled(struct lldp *lldp);
>> void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
>> void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
>>                      const struct eth_addr eth_src);
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index ced67b0..c2f5b12 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -2499,11 +2499,11 @@ set_lldp(struct ofport *ofport_,
>> {
>>     struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
>>     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
>> +    bool old_enable = lldp_is_enabled(ofport->lldp);
>>     int error = 0;
>>
>>     if (cfg) {
>>         if (!ofport->lldp) {
>> -            ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>             ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
>>         }
>>
>> @@ -2515,6 +2515,9 @@ set_lldp(struct ofport *ofport_,
>>     } else if (ofport->lldp) {
>>         lldp_unref(ofport->lldp);
>>         ofport->lldp = NULL;
>> +    }
>> +
>> +    if (lldp_is_enabled(ofport->lldp) != old_enable) {
>>         ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>     }
>>
>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>> index 7f273e5..edb6aa1 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
>> OVS_VSWITCHD_STOP
>> AT_CLEANUP
>>
>> +AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
>> +OVS_VSWITCHD_START(
>> +    [add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
>> +)
>> +dnl first revalidation triggered by add interface
>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>> +1
>> +])
>> +
>> +dnl enable lldp
>> +AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
>
> CI test shows that setting lldp cause memory leaks, will try to find out the places.


Ack, I’ll wait for reviewing once you found/fix it.

PS: You can add me as a co-author and sign off in your next iteration for the changes I made.

>> +AT_CHECK([ovs-appctl revalidator/wait])
>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>> +2
>> +])
>> +
>> +dnl disable lldp
>> +AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
>> +AT_CHECK([ovs-appctl revalidator/wait])
>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>> +3
>> +])
>> +
>> +dnl remove lldp, no revalidation as lldp was disabled
>> +AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
>> +AT_CHECK([ovs-appctl revalidator/wait])
>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>> +3
>> +])
>> +
>> +OVS_VSWITCHD_STOP
>> +AT_CLEANUP
>> +
>> AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
>>
>> dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
>> --
>> 1.8.3.1
>>
Cheng Li Feb. 14, 2022, 12:05 p.m. UTC | #3 
>On 6 Feb 2022, at 4:58, lic121 wrote:
>
>>> If lldp didn't change, we are not supposed to trigger backer
>>> revalidation.
>>>
>>> Without this patch, bridge_reconfigure() always trigger udpif
>>> revalidator because of lldp.
>>>
>>> Signed-off-by: lic121 <lic121@chinatelecom.cn>
>>> Co-authored-by: Eelco Chaudron <echaudro@redhat.com>
>>> ---
>>> lib/ovs-lldp.c         |  8 ++++++++
>>> lib/ovs-lldp.h         |  1 +
>>> ofproto/ofproto-dpif.c |  5 ++++-
>>> tests/ofproto-dpif.at  | 33 +++++++++++++++++++++++++++++++++
>>> 4 files changed, 46 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
>>> index 162311f..bfc69a3 100644
>>> --- a/lib/ovs-lldp.c
>>> +++ b/lib/ovs-lldp.c
>>> @@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
>>>     ovs_mutex_unlock(&mutex);
>>> }
>>>
>>> +/* Is LLDP enabled?
>>> + */
>>> +bool
>>> +lldp_is_enabled(struct lldp *lldp)
>>> +{
>>> +    return lldp ? lldp->enabled : false;
>>> +}
>>> +
>>> /* Configures the LLDP stack.
>>>  */
>>> bool
>>> diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
>>> index 0e536e8..661ac4e 100644
>>> --- a/lib/ovs-lldp.h
>>> +++ b/lib/ovs-lldp.h
>>> @@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
>>> bool lldp_should_send_packet(struct lldp *cfg);
>>> bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
>>> bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
>>> +bool lldp_is_enabled(struct lldp *lldp);
>>> void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
>>> void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
>>>                      const struct eth_addr eth_src);
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index ced67b0..c2f5b12 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -2499,11 +2499,11 @@ set_lldp(struct ofport *ofport_,
>>> {
>>>     struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
>>>     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
>>> +    bool old_enable = lldp_is_enabled(ofport->lldp);
>>>     int error = 0;
>>>
>>>     if (cfg) {
>>>         if (!ofport->lldp) {
>>> -            ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>>             ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
>>>         }
>>>
>>> @@ -2515,6 +2515,9 @@ set_lldp(struct ofport *ofport_,
>>>     } else if (ofport->lldp) {
>>>         lldp_unref(ofport->lldp);
>>>         ofport->lldp = NULL;
>>> +    }
>>> +
>>> +    if (lldp_is_enabled(ofport->lldp) != old_enable) {
>>>         ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>>     }
>>>
>>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>>> index 7f273e5..edb6aa1 100644
>>> --- a/tests/ofproto-dpif.at
>>> +++ b/tests/ofproto-dpif.at
>>> @@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
>>> OVS_VSWITCHD_STOP
>>> AT_CLEANUP
>>>
>>> +AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
>>> +OVS_VSWITCHD_START(
>>> +    [add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
>>> +)
>>> +dnl first revalidation triggered by add interface
>>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>>> +1
>>> +])
>>> +
>>> +dnl enable lldp
>>> +AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
>>
>> CI test shows that setting lldp cause memory leaks, will try to find out the places.
>
>
>Ack, I’ll wait for reviewing once you found/fix it.

v4 is uploaded

>
>PS: You can add me as a co-author and sign off in your next iteration for the changes I made.
>

Sure, please let me know if I haven't done it in the right way.

>>> +AT_CHECK([ovs-appctl revalidator/wait])
>>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>>> +2
>>> +])
>>> +
>>> +dnl disable lldp
>>> +AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
>>> +AT_CHECK([ovs-appctl revalidator/wait])
>>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>>> +3
>>> +])
>>> +
>>> +dnl remove lldp, no revalidation as lldp was disabled
>>> +AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
>>> +AT_CHECK([ovs-appctl revalidator/wait])
>>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>>> +3
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP
>>> +AT_CLEANUP
>>> +
>>> AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
>>>
>>> dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
>>> --
>>> 1.8.3.1
>>>
>
>
diff mbox series

Patch

diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index 162311f..bfc69a3 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -738,6 +738,14 @@  lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
     ovs_mutex_unlock(&mutex);
 }

+/* Is LLDP enabled?
+ */
+bool
+lldp_is_enabled(struct lldp *lldp)
+{
+    return lldp ? lldp->enabled : false;
+}
+
 /* Configures the LLDP stack.
  */
 bool
diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
index 0e536e8..661ac4e 100644
--- a/lib/ovs-lldp.h
+++ b/lib/ovs-lldp.h
@@ -86,6 +86,7 @@  void lldp_run(struct lldpd *cfg);
 bool lldp_should_send_packet(struct lldp *cfg);
 bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
 bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
+bool lldp_is_enabled(struct lldp *lldp);
 void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
 void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
                      const struct eth_addr eth_src);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ced67b0..c2f5b12 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2499,11 +2499,11 @@  set_lldp(struct ofport *ofport_,
 {
     struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
+    bool old_enable = lldp_is_enabled(ofport->lldp);
     int error = 0;

     if (cfg) {
         if (!ofport->lldp) {
-            ofproto->backer->need_revalidate = REV_RECONFIGURE;
             ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
         }

@@ -2515,6 +2515,9 @@  set_lldp(struct ofport *ofport_,
     } else if (ofport->lldp) {
         lldp_unref(ofport->lldp);
         ofport->lldp = NULL;
+    }
+
+    if (lldp_is_enabled(ofport->lldp) != old_enable) {
         ofproto->backer->need_revalidate = REV_RECONFIGURE;
     }

diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 7f273e5..edb6aa1 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -29,6 +29,39 @@  AT_CHECK([ovs-appctl revalidator/wait])
 OVS_VSWITCHD_STOP
 AT_CLEANUP

+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
+OVS_VSWITCHD_START(
+    [add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
+)
+dnl first revalidation triggered by add interface
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+1
+])
+
+dnl enable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+2
+])
+
+dnl disable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+dnl remove lldp, no revalidation as lldp was disabled
+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])

 dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and