| Message ID | 2022010921445851196091@chinatelecom.cn |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | fix dpif backer revalidation | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 9 Jan 2022, at 14:44, lic121 wrote: > If lldp didn't change, we are not supposed to trigger backer > revalidation. > > Signed-off-by: lic121 <lic121@chinatelecom.cn> > --- > ofproto/ofproto-dpif.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 1cdef18..1bc9ec7 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -2461,10 +2461,10 @@ set_lldp(struct ofport *ofport_, > struct ofport_dpif *ofport = ofport_dpif_cast(ofport_); > struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto); > int error = 0; > + struct lldp *old_lldp = ofport->lldp; > > if (cfg) { > if (!ofport->lldp) { > - ofproto->backer->need_revalidate = REV_RECONFIGURE; > ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg); > } > > @@ -2476,6 +2476,8 @@ set_lldp(struct ofport *ofport_, > } else if (ofport->lldp) { > lldp_unref(ofport->lldp); > ofport->lldp = NULL; > + } > + if (old_lldp != ofport->lldp) { Same as for your first patch in the series. Here you only check if the pointer changes, so if you enable and then disable LLDP it will not trigger a REV_RECONFIGURE? Maybe something like this will work (not tested): diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c index 162311fa4..0f374df67 100644 --- a/lib/ovs-lldp.c +++ b/lib/ovs-lldp.c @@ -761,6 +761,14 @@ lldp_configure(struct lldp *lldp, const struct smap *cfg) OVS_EXCLUDED(mutex) return true; } +/* Is LLDP enabled? + */ +bool +lldp_is_enabled(struct lldp *lldp) +{ + return lldp ? lldp->enabled : false; +} + /* Create an LLDP stack instance. At the moment there is one per bridge port. */ struct lldp * diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h index 0e536e8c2..661ac4e18 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 1bc9ec7e4..edfe7e123 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2461,7 +2461,7 @@ 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; - struct lldp *old_lldp = ofport->lldp; if (cfg) { if (!ofport->lldp) { @@ -2477,7 +2477,8 @@ set_lldp(struct ofport *ofport_, lldp_unref(ofport->lldp); ofport->lldp = NULL; } - if (old_lldp != ofport->lldp) { + + if (lldp_is_enabled(ofport->lldp) != old_enable) { ofproto->backer->need_revalidate = REV_RECONFIGURE; } Maybe you could also add some test cases to make sure this works? > ofproto->backer->need_revalidate = REV_RECONFIGURE; > } > > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >On 9 Jan 2022, at 14:44, lic121 wrote: > >> If lldp didn't change, we are not supposed to trigger backer >> revalidation. >> >> Signed-off-by: lic121 <lic121@chinatelecom.cn> >> --- >> ofproto/ofproto-dpif.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index 1cdef18..1bc9ec7 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -2461,10 +2461,10 @@ set_lldp(struct ofport *ofport_, >> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_); >> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto); >> int error = 0; >> + struct lldp *old_lldp = ofport->lldp; >> >> if (cfg) { >> if (!ofport->lldp) { >> - ofproto->backer->need_revalidate = REV_RECONFIGURE; >> ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg); >> } >> >> @@ -2476,6 +2476,8 @@ set_lldp(struct ofport *ofport_, >> } else if (ofport->lldp) { >> lldp_unref(ofport->lldp); >> ofport->lldp = NULL; >> + } >> + if (old_lldp != ofport->lldp) { > >Same as for your first patch in the series. Here you only check if the pointer changes, so if you enable and then disable LLDP it will not trigger a REV_RECONFIGURE? Good catch, lldp_configure returns true even the cfg has eabled=false Will address this in the next version >Maybe something like this will work (not tested): > >diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c >index 162311fa4..0f374df67 100644 >--- a/lib/ovs-lldp.c >+++ b/lib/ovs-lldp.c >@@ -761,6 +761,14 @@ lldp_configure(struct lldp *lldp, const struct smap *cfg) OVS_EXCLUDED(mutex) > return true; > } > >+/* Is LLDP enabled? >+ */ >+bool >+lldp_is_enabled(struct lldp *lldp) >+{ >+ return lldp ? lldp->enabled : false; >+} >+ > /* Create an LLDP stack instance. At the moment there is one per bridge port. > */ > struct lldp * >diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h >index 0e536e8c2..661ac4e18 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 1bc9ec7e4..edfe7e123 100644 >--- a/ofproto/ofproto-dpif.c >+++ b/ofproto/ofproto-dpif.c >@@ -2461,7 +2461,7 @@ 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; >- struct lldp *old_lldp = ofport->lldp; > > if (cfg) { > if (!ofport->lldp) { >@@ -2477,7 +2477,8 @@ set_lldp(struct ofport *ofport_, > lldp_unref(ofport->lldp); > ofport->lldp = NULL; > } >- if (old_lldp != ofport->lldp) { >+ >+ if (lldp_is_enabled(ofport->lldp) != old_enable) { > ofproto->backer->need_revalidate = REV_RECONFIGURE; > } > > >Maybe you could also add some test cases to make sure this works? > >> ofproto->backer->need_revalidate = REV_RECONFIGURE; >> } >> >> -- >> 1.8.3.1 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 1cdef18..1bc9ec7 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -2461,10 +2461,10 @@ set_lldp(struct ofport *ofport_, struct ofport_dpif *ofport = ofport_dpif_cast(ofport_); struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto); int error = 0; + struct lldp *old_lldp = ofport->lldp; if (cfg) { if (!ofport->lldp) { - ofproto->backer->need_revalidate = REV_RECONFIGURE; ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg); } @@ -2476,6 +2476,8 @@ set_lldp(struct ofport *ofport_, } else if (ofport->lldp) { lldp_unref(ofport->lldp); ofport->lldp = NULL; + } + if (old_lldp != ofport->lldp) { ofproto->backer->need_revalidate = REV_RECONFIGURE; }
If lldp didn't change, we are not supposed to trigger backer revalidation. Signed-off-by: lic121 <lic121@chinatelecom.cn> --- ofproto/ofproto-dpif.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) -- 1.8.3.1