On 16 Oct 2023, at 9:09, Roi Dayan wrote:
> On 09/10/2023 15:05, Roi Dayan wrote: >> The cited commit fixed missing mirror packets by reset mirror when >> packets are modified but setting geneve options was also treated as >> a modified packet but should be treated as a part of set_tunnel >> which doesn't reset mirror. >> >> Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are >> modified.") >> Signed-off-by: Roi Dayan <r...@nvidia.com> >> Acked-by: Simon Horman <ho...@ovn.org> >> Acked-by: Eelco Chaudron <echau...@redhat.com> >> --- >> >> Notes: >> v2: >> - user correct sha in fixes line. >> >> ofproto/ofproto-dpif-xlate.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index be4bd6657688..e243773307b7 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -7097,7 +7097,7 @@ reset_mirror_ctx(struct xlate_ctx *ctx, const struct >> flow *flow, >> >> set_field = ofpact_get_SET_FIELD(a); >> mf = set_field->field; >> - if (mf_are_prereqs_ok(mf, flow, NULL)) { >> + if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_tun_metadata(mf)) { >> ctx->mirrors = 0; >> } >> return; > > > Hi, > > I would like to consult another related issue to the original commit. > > feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.") > > Now with geneve options the redundant mirror is removed but if there will be > a real modification there will still be a mirror output but in an incorrect > place. > > For example adding dec_ttl, the action list will be like this: > > actions:enp8s0f0_1,set(tunnel(tun_id=0x2a,src=7.7.7.7,dst=7.7.7.8,ttl=64,tp_dst=6081,geneve({class=0xffff,type=0x80,len=4,0x1234}),flags(df|key))),set(ipv4(ttl=63)),genev_sys_6081,enp8s0f0_1 > > A mirror output enp8s0f0_1 added at the end but the second mirror is with the > tunnel header already. > > When not using tunnels the mirror is fine and the action list will look like > this: > > actions:port1,dec_ttl,port2,port1 > > So with tunnel the second mirror shouldn't have been somehow with the dec_ttl > action but without the tunnel header? > > Should the actions list somehow be like this > > actions:enp8s0f0_1,set(ipv4(ttl=63),enp8s0f0_1,set(tunnel(...),genev_sys_6081 Not sure I follow you, but do you think the dect ttl and set tunnel should have been swapped? I guess this would depend on your OpenFlow rule. Can you show an ofproto trace on your example, which might help clarify OVS’s reasoning? //Eelco > > Am I looking at this wrong? What do you think? > > Thanks, > Roi _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev