Re: [ovs-dev] [PATCH v1 2/2] ofproto-dpif: avoid unneccesary backer revalidation
>
>
>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
>> ---
>> 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
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 2/2] ofproto-dpif: avoid unneccesary backer revalidation
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
> ---
> 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1 2/2] ofproto-dpif: avoid unneccesary backer revalidation
If lldp didn't change, we are not supposed to trigger backer
revalidation.
Signed-off-by: lic121
---
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) {
ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
