[ovs-dev] bond/mirror: fix duplicate output when mix bond and mirror
diff mbox series

Message ID SG2PR03MB3960E18B05B95507A1C2D700B6510@SG2PR03MB3960.apcprd03.prod.outlook.com
State Superseded
Headers show
Series
  • [ovs-dev] bond/mirror: fix duplicate output when mix bond and mirror
Related show

Commit Message

? ? April 5, 2019, 1:26 p.m. UTC
When we configured bond that use recirc and configure mirror, we
observed that mirror destination is duplicated output.  This is
because bond's frozen_state is not updated to reflect the mirror
already composed the output, and when the companion recirc flow
need to decide its action, the mirror again take effects, causing
duplicate output.

Signed-off-by: Shuang Huang <kevinhuangs@hotmail.com>
---
 ofproto/ofproto-dpif-xlate.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

0-day Robot April 5, 2019, 2:50 p.m. UTC | #1
Bleep bloop.  Greetings ? ?, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author ? ? <kevinhuangs@hotmail.com> needs to sign off.
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Shuang Huang <kevinhuangs@hotmail.com>
Lines checked: 42, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot
Ben Pfaff April 8, 2019, 8:17 p.m. UTC | #2
On Fri, Apr 05, 2019 at 01:26:29PM +0000, ? ? wrote:
> When we configured bond that use recirc and configure mirror, we
> observed that mirror destination is duplicated output.  This is
> because bond's frozen_state is not updated to reflect the mirror
> already composed the output, and when the companion recirc flow
> need to decide its action, the mirror again take effects, causing
> duplicate output.
> 
> Signed-off-by: Shuang Huang <kevinhuangs@hotmail.com>

Thanks for identifying that there is a problem.

I don't think that bonds that use recirculation involve frozen states.
In OVS, recirculation for bonds was added a long time before frozen
state, and I'm not sure that they're really related.  Output to a bond,
even when recirculation is involved, should not cause state to be
frozen.

Assuming I'm missing something, though, I do not think that this is a
good solution.  A frozen_state is not supposed to be modifiable.
Nothing else in the code base modifies one after creating it.  I do not
think that we should do it this way.

It seems to me that a better solution would be to disable mirroring for
flow translation of the flows that implement the bond recirculation.

Patch
diff mbox series

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index c4014d7..d4f10b7 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2453,6 +2453,16 @@  output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle,                                                                                            
     compose_output_action(ctx, xport->ofp_port, use_recirc ? &xr : NULL,
                           false, false);
     memcpy(&ctx->xin->flow.vlans, &old_vlans, sizeof(old_vlans));
+    /* Store mirrors for bond that use recirc to avoid duplicate output */
+    if (use_recirc) {
+        struct recirc_id_node *node = CONST_CAST(
+            struct recirc_id_node *, recirc_id_node_find(xr.recirc_id));
+        if (node) {
+            struct frozen_state *state =
+                CONST_CAST(struct frozen_state *, &node->state);
+            state->mirrors = ctx->mirrors;
+        }
+    }
 }

 /* A VM broadcasts a gratuitous ARP to indicate that it has resumed after