>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:
Hi Eelco, very appreciate your careful review of this patch. About the test, I
am very sure I had run the test and passed at my local environment.
But after my local test before I upload the test, I made a rebase on the
upstream master. I will update the patch to make sure it works.
>
>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
