Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: remove mirror assert

[lic121]

On Wed, Apr 27, 2022 at 01:36:48PM +0200, Adrian Moreno wrote:
> 
> 
> On 4/7/22 15:53, lic121 wrote:
> >During the revalidation, mirror could be removed. Instead of crash
> >the process, we can simply skip the deleted mirror.
> >
> >Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.")
> >Signed-off-by: lic121 
> >---
> >  ofproto/ofproto-dpif-xlate.c | 10 +++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> >diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> >index 5a770f1..33e56c2 100644
> >--- a/ofproto/ofproto-dpif-xlate.c
> >+++ b/ofproto/ofproto-dpif-xlate.c
> >@@ -2142,9 +2142,13 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle 
> >*xbundle,
> >  int snaplen;
> >  /* Get the details of the mirror represented by the rightmost 
> > 1-bit. */
> >-ovs_assert(mirror_get(xbridge->mbridge, raw_ctz(mirrors),
> >-  , _mirrors,
> >-  , , _vlan));
> >+if (!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
> >+   , _mirrors,
> >+   , , _vlan)) {
> >+/* Mirror could be deleted during revalidation */
> >+mirrors = zero_rightmost_1bit(mirrors);
> >+continue;
> >+}
> >  /* If this mirror selects on the basis of VLAN, and it does not 
> > select
> 
> I have reproduced the assert and verified this patch fixes it.
> 
> The patch looks good to me. My only nit is about the comment, it's a
> bit confusing because this code is executed also in upcall handling,
> not only on revalidation. Something on the lines of the following
> would be a bit more clear IMHO:
> 
> /* The mirror got reconfigured before we got to read it's configuration. */
> 
> Also, maybe add OVS_UNLIKELY?

Thanks for the review, will address the comments in the next version.

> 
> Thanks.
> -- 
> Adrián Moreno
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: remove mirror assert

[Adrian Moreno]

On 4/7/22 15:53, lic121 wrote:

During the revalidation, mirror could be removed. Instead of crash
the process, we can simply skip the deleted mirror.

Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.")
Signed-off-by: lic121 
---
  ofproto/ofproto-dpif-xlate.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 5a770f1..33e56c2 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2142,9 +2142,13 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle 
*xbundle,
  int snaplen;
  
  /* Get the details of the mirror represented by the rightmost 1-bit. */

-ovs_assert(mirror_get(xbridge->mbridge, raw_ctz(mirrors),
-  , _mirrors,
-  , , _vlan));
+if (!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
+   , _mirrors,
+   , , _vlan)) {
+/* Mirror could be deleted during revalidation */
+mirrors = zero_rightmost_1bit(mirrors);
+continue;
+}
  
  
  /* If this mirror selects on the basis of VLAN, and it does not select


I have reproduced the assert and verified this patch fixes it.

The patch looks good to me. My only nit is about the comment, it's a bit 
confusing because this code is executed also in upcall handling, not only on 
revalidation. Something on the lines of the following would be a bit more clear 
IMHO:


/* The mirror got reconfigured before we got to read it's configuration. */

Also, maybe add OVS_UNLIKELY?

Thanks.
--
Adrián Moreno

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] ofproto-dpif-xlate: remove mirror assert

[lic121]

During the revalidation, mirror could be removed. Instead of crash
the process, we can simply skip the deleted mirror.

Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.")
Signed-off-by: lic121 
---
 ofproto/ofproto-dpif-xlate.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 5a770f1..33e56c2 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2142,9 +2142,13 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle 
*xbundle,
 int snaplen;
 
 /* Get the details of the mirror represented by the rightmost 1-bit. */
-ovs_assert(mirror_get(xbridge->mbridge, raw_ctz(mirrors),
-  , _mirrors,
-  , , _vlan));
+if (!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
+   , _mirrors,
+   , , _vlan)) {
+/* Mirror could be deleted during revalidation */
+mirrors = zero_rightmost_1bit(mirrors);
+continue;
+}
 
 
 /* If this mirror selects on the basis of VLAN, and it does not select
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev