Message ID | 2022020510581518971432@chinatelecom.cn |
---|---|
State | Superseded |
Headers | show |
Series | fix dpif backer revalidation | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | warning | apply and check: warning |
ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
>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 >
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 >>
>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 --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
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