| 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. | expand |
> 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
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
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.
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?
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
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(-)