On 28 Jan 2022, at 7:41, 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 <[email protected]>
> ---
> 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(-)
The patch itself looks good to me, however the selftest is failing each time on
the second case:
./ofproto-dpif.at:37: ovs-appctl coverage/read-counter rev_reconfigure
./ofproto-dpif.at:42: ovs-vsctl set interface p1 lldp:enable=yes
./ofproto-dpif.at:44: ovs-appctl revalidator/wait
./ofproto-dpif.at:45: ovs-appctl coverage/read-counter rev_reconfigure
--- - 2022-02-04 13:12:54.116010047 +0100
+++
/home/echaudron/Documents/review/ovs_lic_trigger_revalidation/tests/testsuite.dir/at-groups/1036/stdout
2022-02-04 13:12:54.114089445 +0100
@@ -1,2 +1,2 @@
-2
+1
I even tried to add a 5-second delay, but same problem:
dnl enable lldp
AT_CHECK([ovs-vsctl set interface p1 lldp:enable=yes])
sleep 5
AT_CHECK([ovs-appctl revalidator/wait])
AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
2
])
So it made me wonder if you ever tested your patch? Especially as I figured out
the script was wrong.
Here is the diff to make it work:
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 1ba0aac85..7de27fe7c 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -39,14 +39,14 @@ AT_CHECK([ovs-appctl coverage/read-counter
rev_reconfigure], [0], [dnl
])
dnl enable lldp
-AT_CHECK([ovs-vsctl set interface p1 lldp:enable=yes])
+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=no])
+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
Guess you need a v3 of this.
>
> 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 5737615..c3c1922 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2461,11 +2461,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);
> }
>
> @@ -2477,6 +2477,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 1660b08..1ba0aac 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=yes])
> +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=no])
> +AT_CHECK([ovs-appctl revalidator/wait])
> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
> +3
> +])
> +
> +dnl remove lldp
> +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
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
