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
