On 3/12/24 18:37, Mike Pattrick wrote: > Previously OVS reset the mirror contents when a packet is modified in > such a way that the packets contents changes. However, this change > incorrectly reset that mirror context when only metadata changes as > well. > > Now we check for all metadata fields, instead of just tunnel metadata, > before resetting the mirror context. > > Fixes: feed7f677505 ("ofproto-dpif-upcall: Mirror packets that are modified.") > Reported-by: Zhangweiwei <zhang.wei...@h3c.com> > Signed-off-by: Mike Pattrick <m...@redhat.com> > --- > include/openvswitch/meta-flow.h | 1 + > lib/meta-flow.c | 109 ++++++++++++++++++++++++++++++++ > ofproto/ofproto-dpif-xlate.c | 2 +- > tests/ofproto-dpif.at | 5 +- > 4 files changed, 114 insertions(+), 3 deletions(-)
Thanks, Mike! The change makes sense to me. See some comments inline. Best regards, Ilya Maixmets. > > diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h > index 3b0220aaa..96aad3933 100644 > --- a/include/openvswitch/meta-flow.h > +++ b/include/openvswitch/meta-flow.h > @@ -2305,6 +2305,7 @@ void mf_set_flow_value_masked(const struct mf_field *, > const union mf_value *mask, > struct flow *); > bool mf_is_tun_metadata(const struct mf_field *); > +bool mf_is_metadata(const struct mf_field *); Nit: maybe call it mf_is_any_metadata ? > bool mf_is_frozen_metadata(const struct mf_field *); > bool mf_is_pipeline_field(const struct mf_field *); > bool mf_is_set(const struct mf_field *, const struct flow *); > diff --git a/lib/meta-flow.c b/lib/meta-flow.c > index aa7cf1fcb..7ecec334e 100644 > --- a/lib/meta-flow.c > +++ b/lib/meta-flow.c > @@ -1788,6 +1788,115 @@ mf_is_tun_metadata(const struct mf_field *mf) > mf->id < MFF_TUN_METADATA0 + TUN_METADATA_NUM_OPTS; > } > > +bool > +mf_is_metadata(const struct mf_field *mf) > +{ > + switch (mf->id) { > + CASE_MFF_TUN_METADATA: > + case MFF_METADATA: > + case MFF_IN_PORT: > + case MFF_IN_PORT_OXM: > + CASE_MFF_REGS: > + CASE_MFF_XREGS: > + CASE_MFF_XXREGS: > + case MFF_PACKET_TYPE: > + case MFF_DP_HASH: > + case MFF_RECIRC_ID: > + case MFF_CONJ_ID: > + case MFF_ACTSET_OUTPUT: > + case MFF_SKB_PRIORITY: > + case MFF_PKT_MARK: > + case MFF_CT_STATE: > + case MFF_CT_ZONE: > + case MFF_CT_MARK: > + case MFF_CT_LABEL: > + case MFF_CT_NW_PROTO: > + case MFF_CT_NW_SRC: > + case MFF_CT_NW_DST: > + case MFF_CT_IPV6_SRC: > + case MFF_CT_IPV6_DST: > + case MFF_CT_TP_SRC: > + case MFF_CT_TP_DST: > + case MFF_N_IDS: The N_IDS should not be here. It probably goes to the 'default' case. > + return true; > + > + case MFF_TUN_ID: > + case MFF_TUN_SRC: > + case MFF_TUN_DST: > + case MFF_TUN_IPV6_SRC: > + case MFF_TUN_IPV6_DST: > + case MFF_TUN_FLAGS: > + case MFF_TUN_GBP_ID: > + case MFF_TUN_GBP_FLAGS: > + case MFF_TUN_ERSPAN_VER: > + case MFF_TUN_ERSPAN_IDX: > + case MFF_TUN_ERSPAN_DIR: > + case MFF_TUN_ERSPAN_HWID: > + case MFF_TUN_GTPU_FLAGS: > + case MFF_TUN_GTPU_MSGTYPE: > + case MFF_TUN_TTL: > + case MFF_TUN_TOS: > + case MFF_ETH_SRC: > + case MFF_ETH_DST: > + case MFF_ETH_TYPE: > + case MFF_VLAN_TCI: > + case MFF_DL_VLAN: > + case MFF_VLAN_VID: > + case MFF_DL_VLAN_PCP: > + case MFF_VLAN_PCP: > + case MFF_MPLS_LABEL: > + case MFF_MPLS_TC: > + case MFF_MPLS_BOS: > + case MFF_MPLS_TTL: > + case MFF_IPV4_SRC: > + case MFF_IPV4_DST: > + case MFF_IPV6_SRC: > + case MFF_IPV6_DST: > + case MFF_IPV6_LABEL: > + case MFF_IP_PROTO: > + case MFF_IP_DSCP: > + case MFF_IP_DSCP_SHIFTED: > + case MFF_IP_ECN: > + case MFF_IP_TTL: > + case MFF_IP_FRAG: > + case MFF_ARP_OP: > + case MFF_ARP_SPA: > + case MFF_ARP_TPA: > + case MFF_ARP_SHA: > + case MFF_ARP_THA: > + case MFF_TCP_SRC: > + case MFF_TCP_DST: > + case MFF_TCP_FLAGS: > + case MFF_UDP_SRC: > + case MFF_UDP_DST: > + case MFF_SCTP_SRC: > + case MFF_SCTP_DST: > + case MFF_ICMPV4_TYPE: > + case MFF_ICMPV4_CODE: > + case MFF_ICMPV6_TYPE: > + case MFF_ICMPV6_CODE: > + case MFF_ND_TARGET: > + case MFF_ND_SLL: > + case MFF_ND_TLL: > + case MFF_ND_RESERVED: > + case MFF_ND_OPTIONS_TYPE: > + case MFF_NSH_FLAGS: > + case MFF_NSH_TTL: > + case MFF_NSH_MDTYPE: > + case MFF_NSH_NP: > + case MFF_NSH_SPI: > + case MFF_NSH_SI: > + case MFF_NSH_C1: > + case MFF_NSH_C2: > + case MFF_NSH_C3: > + case MFF_NSH_C4: > + return false; In general, functions like this are trying to maintain a specific order of the cases. It's either alphabetical or the order in which they are defined in the enum. I think, most of the mf functions are trying to follwo the enum order. Order in this fucntion doesn't seem to match any other function. Could you re-order the cases in the enum order, please? > + > + default: > + OVS_NOT_REACHED(); > + } > +} > + > bool > mf_is_frozen_metadata(const struct mf_field *mf) > { > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 89f183182..faa364ec8 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -7141,7 +7141,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) && !mf_is_tun_metadata(mf)) { > + if (mf_are_prereqs_ok(mf, flow, NULL) && !mf_is_metadata(mf)) { > ctx->mirrors = 0; > } > return; > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index a1393f7f8..245e209c3 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -5443,7 +5443,8 @@ AT_CLEANUP > # This test verifies that mirror state is preserved across recirculation. > # > # Otherwise, post-recirculation the ingress and the output to port 4 > -# would cause the packet to be mirrored to port 3 a second time. > +# would cause the packet to be mirrored to port 3 a second time. A register > +# is also modified to verify that this doesn't reset the mirror context. I don't think we should mix the tests like that. Original test is checking a particular functionlity related to recirculation. We should have a separate test for metadata changes. > AT_SETUP([ofproto-dpif - mirroring with recirculation]) > AT_KEYWORDS([mirror mirrors mirroring]) > OVS_VSWITCHD_START > @@ -5454,7 +5455,7 @@ ovs-vsctl \ > --id=@m create Mirror name=mymirror select_all=true output_port=@p3 > > AT_DATA([flows.txt], [dnl > -in_port=1 actions=2,debug_recirc,4 > +in_port=1 actions=2,debug_recirc,set_field:1->reg4,4 > ]) > AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev