Hi Aaron,
<snip commit message away>
> ---
> include/openvswitch/flow.h | 7 +++----
> tests/classifier.at | 27 +++++++++++++++++++++++++++
> 2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h
> index 3054015d93..df10cf579e 100644
> --- a/include/openvswitch/flow.h
> +++ b/include/openvswitch/flow.h
> @@ -141,15 +141,14 @@ struct flow {
> uint8_t nw_tos; /* IP ToS (including DSCP and ECN). */
> uint8_t nw_ttl; /* IP TTL/Hop Limit. */
> uint8_t nw_proto; /* IP protocol or low 8 bits of ARP opcode.
> */
> + /* L4 (64-bit aligned) */
> struct in6_addr nd_target; /* IPv6 neighbor discovery (ND) target. */
> struct eth_addr arp_sha; /* ARP/ND source hardware address. */
> struct eth_addr arp_tha; /* ARP/ND target hardware address. */
> - ovs_be16 tcp_flags; /* TCP flags/ICMPv6 ND options type.
> - * With L3 to avoid matching L4. */
> + ovs_be16 tcp_flags; /* TCP flags/ICMPv6 ND options type. */
> ovs_be16 pad2; /* Pad to 64 bits. */
> struct ovs_key_nsh nsh; /* Network Service Header keys */
>
> - /* L4 (64-bit aligned) */
> ovs_be16 tp_src; /* TCP/UDP/SCTP source port/ICMP type. */
> ovs_be16 tp_dst; /* TCP/UDP/SCTP destination port/ICMP code.
> */
> ovs_be16 ct_tp_src; /* CT original tuple source port/ICMP type.
> */
> @@ -179,7 +178,7 @@ BUILD_ASSERT_DECL(offsetof(struct flow, igmp_group_ip4) +
> sizeof(uint32_t)
About the following BUILD_ASSRT_DECL which is in include/openvswitch/flow.h
directly below struct flow.h:
/* Remember to update FLOW_WC_SEQ when changing 'struct flow'. */
BUILD_ASSERT_DECL(offsetof(struct flow, igmp_group_ip4) + sizeof(uint32_t)
== sizeof(struct flow_tnl) + sizeof(struct ovs_key_nsh) + 300
&& FLOW_WC_SEQ == 42);
Should we increment the FLOW_WC_SEQ value? The comment reads:
/* This sequence number should be incremented whenever anything involving flows
* or the wildcarding of flows changes. This will cause build assertion
* failures in places which likely need to be updated. */
We haven't changed anything in the order or content of struct flow but we have
changed (fixed) how flows are wildcarded. It doesn't look like any of the code
asserting FLOW_WC_SEQ == 42 and relying on struct flow order and content needs
updating.
Just wondering your/others thoughts about whether to increment to 43 or not.
> enum {
> FLOW_SEGMENT_1_ENDS_AT = offsetof(struct flow, dl_dst),
> FLOW_SEGMENT_2_ENDS_AT = offsetof(struct flow, nw_src),
> - FLOW_SEGMENT_3_ENDS_AT = offsetof(struct flow, tp_src),
> + FLOW_SEGMENT_3_ENDS_AT = offsetof(struct flow, nd_target),
> };
> BUILD_ASSERT_DECL(FLOW_SEGMENT_1_ENDS_AT % sizeof(uint64_t) == 0);
> BUILD_ASSERT_DECL(FLOW_SEGMENT_2_ENDS_AT % sizeof(uint64_t) == 0);
IIUC, we are moving the L4 boundary 56B earlier in struct flow. This means
L3/L4 segment boundary is still at a U64 boundary. I know we have BUILD_ASSERTs
checking this, but I just thought to double check myself.
> diff --git a/tests/classifier.at b/tests/classifier.at
> index cdcd72c156..a0a8fe0359 100644
> --- a/tests/classifier.at
> +++ b/tests/classifier.at
> @@ -129,6 +129,33 @@ Datapath actions: 3
> OVS_VSWITCHD_STOP(["/'prefixes' with incompatible field: ipv6_label/d"])
> AT_CLEANUP
>
> +AT_SETUP([flow classifier - ipv6 ND dependancy])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2
> +AT_DATA([flows.txt], [dnl
> + table=0,priority=100,ipv6,ipv6_src=1000::/10 actions=resubmit(,1)
> + table=0,priority=0 actions=NORMAL
> + table=1,priority=110,ipv6,ipv6_dst=1000::3 actions=resubmit(,2)
> + table=1,priority=100,ipv6,ipv6_dst=1000::4 actions=resubmit(,2)
> + table=1,priority=0 actions=NORMAL
> +
> table=2,priority=120,icmp6,nw_ttl=255,icmp_type=135,icmp_code=0,nd_target=1000::1
> actions=NORMAL
> + table=2,priority=100,tcp actions=NORMAL
> + table=2,priority=100,icmp6 actions=NORMAL
> + table=2,priority=0 actions=NORMAL
> +])
> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> +
> +# test ICMPv6 echo request (which should have no nd_target field)
> +AT_CHECK([ovs-appctl ofproto/trace br0
> "in_port=1,eth_src=f6:d2:b0:19:5e:7b,eth_dst=d2:49:19:91:78:fe,dl_type=0x86dd,ipv6_src=1000::3,ipv6_ds
> t=1000::4,nw_proto=58,icmpv6_type=128,icmpv6_code=0"], [0], [stdout])
> +AT_CHECK([tail -2 stdout], [0],
> + [Megaflow:
> recirc_id=0,eth,icmp6,in_port=1,dl_src=f6:d2:b0:19:5e:7b,dl_dst=d2:49:19:91:78:fe,ipv6_src=1000::/10,i
> pv6_dst=1000::4,nw_ttl=0,nw_frag=no
> +Datapath actions: 100,2
> +])
> +
> +AT_CLEANUP
> +
> +
> +
Nit: The other tests in the file have one line after the "AT_CLEANUP", maybe
this should be the same instead of 3 lines?
> AT_BANNER([conjunctive match])
>
> AT_SETUP([single conjunctive match])
I've applied the patch to the latest master, run the unit test and it passes
for me.
I also ran the test after reverting just the C code changes in this patch and
correctly fails as expected. The Megaflow in the test has a ",nd_target=::" at
the end, which the test correctly picks up as a failure. The test is correctly
doing its job!
I have looked at the performance tests we run and none of them are affected by
moving tcp_flags from the l3 subtable match to l4.
Out of curiosity, I also ran the classifier benchmark tests (ovstest
test-classifier benchmark) and didn't see a performance difference before and
after the patch. Having a look at tests/test-classifier.c, I can see that the
tcp_flags field isn't tested, so it makes sense there's no difference in
performance there.
Regardless of the questions and comments above, I'm happy to Ack this patch in
its current state:
Acked-by: Cian Ferriter <cian.ferri...@intel.com>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
