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