> -----Original Message-----
> From: Aaron Conole <[email protected]>
> Sent: Thursday 19 May 2022 21:53
> To: Ferriter, Cian <[email protected]>
> Cc: [email protected]; Flavio Leitner <[email protected]>; Ilya Maximets
> <[email protected]>; Rashid
> Khan <[email protected]>
> Subject: Re: [ovs-dev] [PATCH] classifier: adjust segment boundary to execute
> prerequisite processing
>
> "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.
>
That makes sense, I agree.
> >> 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.
>
Thanks Aaron.
> >> 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
