Message ID | 1649339584-8250-1-git-send-email-lic121@chinatelecom.cn |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | [ovs-dev] ofproto-dpif-xlate: remove mirror assert | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | success | test: success |
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 <lic121@chinatelecom.cn> > --- > 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), > - &vlans, &dup_mirrors, > - &out, &snaplen, &out_vlan)); > + if (!mirror_get(xbridge->mbridge, raw_ctz(mirrors), > + &vlans, &dup_mirrors, > + &out, &snaplen, &out_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.
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 <lic121@chinatelecom.cn> > >--- > > 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), > >- &vlans, &dup_mirrors, > >- &out, &snaplen, &out_vlan)); > >+ if (!mirror_get(xbridge->mbridge, raw_ctz(mirrors), > >+ &vlans, &dup_mirrors, > >+ &out, &snaplen, &out_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 >
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), - &vlans, &dup_mirrors, - &out, &snaplen, &out_vlan)); + if (!mirror_get(xbridge->mbridge, raw_ctz(mirrors), + &vlans, &dup_mirrors, + &out, &snaplen, &out_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
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 <lic121@chinatelecom.cn> --- ofproto/ofproto-dpif-xlate.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)