"Ferriter, Cian" <[email protected]> writes:
> Hi Aaron,
Hi Cian,
> <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.
I decided not to increment it because it's really a subtable
configuration for the classifier rather than a flow structure related
change. In ofproto/ofpcoto.c we initialize the classifier with the map
stored in flow.c that holds the various segments. I consider it more of
a classifier configuration, in that sense. As you note, it doesn't
modify the layout or size of flow struct. Maybe we can have a more
precise comment around that area.
>> 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.
Yes - it's 448 bits or 7 u64 words. This is a bit lucky for us -
otherwise we might have had to get more creative :)
>> 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?
Hrrm.. not sure how that got in. I can spin a v2 with this cleaned up.
>> 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.
That's good to know - it's an area I'm concerned about.
> 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 <[email protected]>
Thanks!
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
