[ovs-dev] ofproto-dpif-xlate: Account mirrored packets only if the VLAN matches.

Message ID 20181227214155.24265-1-blp@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev] ofproto-dpif-xlate: Account mirrored packets only if the VLAN matches.
Related show

Commit Message

Ben Pfaff Dec. 27, 2018, 9:41 p.m.
Until now, OVS has accounted packets to mirrors even if the VLAN selection
criteria did not match.  This fixes the problem.

Reported-by: Shweta Seth <shwseth@cisco.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-December/047931.html
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ofproto/ofproto-dpif-xlate.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

Comments

Justin Pettit Jan. 17, 2019, 9:56 p.m. | #1
> On Dec 27, 2018, at 1:41 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 839fddd99fbe..8d17151a057e 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2058,21 +2058,9 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
> 
>     /* 'mirrors' is a bit-mask of candidates for mirroring.  Iterate as long as
>      * some candidates remain.  */
> +    mirror_mask_t used_mirrors = 0;

This is very minor, but the comment combined with the similarly declared variable seems like it could lead to confusion on a quick reading.  Maybe move the declaration up?  I know that would slightly break the preferred style, so feel free to ignore this.

Acked-by: Justin Pettit <jpettit@ovn.org>

--Justin
Jann Horn via dev Jan. 17, 2019, 11:18 p.m. | #2
Thanks,

I have tried the patch sent earlier and it solved the issue I was facing. 

Do you think this iteration is a required must have? 

Thanks,
Shweta
Ben Pfaff Jan. 18, 2019, 12:28 a.m. | #3
On Thu, Jan 17, 2019 at 01:56:43PM -0800, Justin Pettit wrote:
> 
> > On Dec 27, 2018, at 1:41 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 839fddd99fbe..8d17151a057e 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -2058,21 +2058,9 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
> > 
> >     /* 'mirrors' is a bit-mask of candidates for mirroring.  Iterate as long as
> >      * some candidates remain.  */
> > +    mirror_mask_t used_mirrors = 0;
> 
> This is very minor, but the comment combined with the similarly declared variable seems like it could lead to confusion on a quick reading.  Maybe move the declaration up?  I know that would slightly break the preferred style, so feel free to ignore this.
> 
> Acked-by: Justin Pettit <jpettit@ovn.org>

Thanks for the feedback!  I clarified the comment and applied it to
master and backported as far as branch-2.7.
Ben Pfaff Jan. 18, 2019, 12:29 a.m. | #4
On Thu, Jan 17, 2019 at 11:18:39PM +0000, Shweta Seth (shwseth) wrote:
> Thanks,
> 
> I have tried the patch sent earlier and it solved the issue I was facing. 

Thanks for testing!  I applied this to relevant branches.

> Do you think this iteration is a required must have? 

I don't understand the question.  Can you ask it a different way?

Patch

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 839fddd99fbe..8d17151a057e 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2058,21 +2058,9 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
         return;
     }
 
-    if (ctx->xin->resubmit_stats) {
-        mirror_update_stats(xbridge->mbridge, mirrors,
-                            ctx->xin->resubmit_stats->n_packets,
-                            ctx->xin->resubmit_stats->n_bytes);
-    }
-    if (ctx->xin->xcache) {
-        struct xc_entry *entry;
-
-        entry = xlate_cache_add_entry(ctx->xin->xcache, XC_MIRROR);
-        entry->mirror.mbridge = mbridge_ref(xbridge->mbridge);
-        entry->mirror.mirrors = mirrors;
-    }
-
     /* 'mirrors' is a bit-mask of candidates for mirroring.  Iterate as long as
      * some candidates remain.  */
+    mirror_mask_t used_mirrors = 0;
     while (mirrors) {
         const unsigned long *vlans;
         mirror_mask_t dup_mirrors;
@@ -2096,6 +2084,9 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
             continue;
         }
 
+        /* We sent a packet to this mirror. */
+        used_mirrors |= rightmost_1bit(mirrors);
+
         /* Record the mirror, and the mirrors that output to the same
          * destination, so that we don't mirror to them again.  This must be
          * done now to ensure that output_normal(), below, doesn't recursively
@@ -2129,6 +2120,21 @@  mirror_packet(struct xlate_ctx *ctx, struct xbundle *xbundle,
         mirrors &= ~ctx->mirrors;
         ctx->mirror_snaplen = 0;
     }
+
+    if (used_mirrors) {
+        if (ctx->xin->resubmit_stats) {
+            mirror_update_stats(xbridge->mbridge, used_mirrors,
+                                ctx->xin->resubmit_stats->n_packets,
+                                ctx->xin->resubmit_stats->n_bytes);
+        }
+        if (ctx->xin->xcache) {
+            struct xc_entry *entry;
+
+            entry = xlate_cache_add_entry(ctx->xin->xcache, XC_MIRROR);
+            entry->mirror.mbridge = mbridge_ref(xbridge->mbridge);
+            entry->mirror.mirrors = used_mirrors;
+        }
+    }
 }
 
 static void