Re: [ovs-dev] [PATCH] classifier: adjust segment boundary to execute prerequisite processing
> -Original Message-
> From: Aaron Conole
> Sent: Thursday 19 May 2022 21:53
> To: Ferriter, Cian
> Cc: d...@openvswitch.org; Flavio Leitner ; Ilya Maximets
> ; Rashid
> Khan
> Subject: Re: [ovs-dev] [PATCH] classifier: adjust segment boundary to execute
> prerequisite processing
>
> "Ferriter, Cian" writes:
>
> > Hi Aaron,
>
> Hi Cian,
>
> >
> >
> >> ---
> >> 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 -
&g
Re: [ovs-dev] [PATCH] classifier: adjust segment boundary to execute prerequisite processing
"Ferriter, Cian" writes:
> Hi Aaron,
Hi Cian,
>
>
>> ---
>> 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-
Re: [ovs-dev] [PATCH] classifier: adjust segment boundary to execute prerequisite processing
On Thu, May 19, 2022 at 9:25 AM Ferriter, Cian wrote:
>
> Hi Aaron,
>
>
>
> > ---
> > 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
> > +
> > +
Re: [ovs-dev] [PATCH] classifier: adjust segment boundary to execute prerequisite processing
Hi Aaron,
> ---
> 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 tes
Re: [ovs-dev] [PATCH] classifier: adjust segment boundary to execute prerequisite processing
On 16 May 2022, at 22:04, Aaron Conole wrote:
> During flow processing, the flow wildcards are checked as a series of
> stages, and these stages are intended to carry dependencies in a single
> direction. But when the neighbor discovery processing, for example, was
> executed there is an incorrect dependency chain - we need fields from
> stage 4 to determine whether we need fields from stage 3.
>
> We can build a set of flow rules to demonstrate this:
> 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_sll=10:de:ad:be:ef:10
> actions=NORMAL
> table=2,priority=100,tcp actions=NORMAL
> table=2,priority=100,icmp6 actions=NORMAL
> table=2,priority=0 actions=NORMAL
>
> With this set of flows, any IPv6 packet that executes through this pipeline
> will have the corresponding nd_sll field flagged as required match for
> classification even if that field doesn't make sense in such a context
> (for example, TCP packets). When the corresponding flow is installed into
> the kernel datapath, this field is not reflected when the revalidator
> executes the dump stage (see net/openvswitch/flow_netlink.c for more details).
>
> During the sweep stage, revalidator will compare the dumped WC with a
> generated WC - these will mismatch because the generated WC will match on
> the Neighbor Discovery fields, while the datapath WC will not match on
> these fields. We will then invalidate the flow and as a side effect
> force an upcall.
>
> By redefining the boundary, we shift these fields to the l4 subtable, and
> cause masks to be generated matching just the requisite fields. The list
> of fields being shifted:
>
> struct in6_addr nd_target;
> struct eth_addr arp_sha;
> struct eth_addr arp_tha;
> ovs_be16 tcp_flags;
> ovs_be16 pad2;
> struct ovs_key_nsh nsh;
>
> A standout field would be tcp_flags moving from l3 subtable matches to
> the l4 subtable matches. This reverts a partial performance optimization
> in the case of stateless firewalling. The tcp_flags field might have
> been a good candidate to retain in the l3 segment, but it got overloaded
> with ICMPv6 ND matching, and therefore we can't preserve this kind of
> optimization.
>
> Two other approaches were considered - moving the nd_target field alone
> and collapsing the l3/l4 segments into a single subtable for matching.
> Moving any field individually introduces ABI mismatch, and doesn't
> completely address the problems with other neighbor discovery related
> fields (such as nd_sll/nd_tll). Collapsing the two subtables creates
> an issue with datapath flow explosion, since the l3 and l4 fields will
> be unwildcarded together (this can be seen with some of the existing
> classifier tests).
>
> A simple test is added to showcase the behavior.
>
> Fixes: 476f36e83bc5 ("Classifier: Staged subtable matching.")
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2081773
> Reported-by: Numan Siddique
> Suggested-by: Ilya Maximets
> Signed-off-by: Aaron Conole
> ---
Bit later response (was planning on replying to the RFC patch), but I needed a
bit more time (and experimenting) with this area of the code :)
The change looks good, and thanks for adding so many details in the commit
message!
Acked-by: Eelco Chaudron
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] classifier: adjust segment boundary to execute prerequisite processing
During flow processing, the flow wildcards are checked as a series of
stages, and these stages are intended to carry dependencies in a single
direction. But when the neighbor discovery processing, for example, was
executed there is an incorrect dependency chain - we need fields from
stage 4 to determine whether we need fields from stage 3.
We can build a set of flow rules to demonstrate this:
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_sll=10:de:ad:be:ef:10
actions=NORMAL
table=2,priority=100,tcp actions=NORMAL
table=2,priority=100,icmp6 actions=NORMAL
table=2,priority=0 actions=NORMAL
With this set of flows, any IPv6 packet that executes through this pipeline
will have the corresponding nd_sll field flagged as required match for
classification even if that field doesn't make sense in such a context
(for example, TCP packets). When the corresponding flow is installed into
the kernel datapath, this field is not reflected when the revalidator
executes the dump stage (see net/openvswitch/flow_netlink.c for more details).
During the sweep stage, revalidator will compare the dumped WC with a
generated WC - these will mismatch because the generated WC will match on
the Neighbor Discovery fields, while the datapath WC will not match on
these fields. We will then invalidate the flow and as a side effect
force an upcall.
By redefining the boundary, we shift these fields to the l4 subtable, and
cause masks to be generated matching just the requisite fields. The list
of fields being shifted:
struct in6_addr nd_target;
struct eth_addr arp_sha;
struct eth_addr arp_tha;
ovs_be16 tcp_flags;
ovs_be16 pad2;
struct ovs_key_nsh nsh;
A standout field would be tcp_flags moving from l3 subtable matches to
the l4 subtable matches. This reverts a partial performance optimization
in the case of stateless firewalling. The tcp_flags field might have
been a good candidate to retain in the l3 segment, but it got overloaded
with ICMPv6 ND matching, and therefore we can't preserve this kind of
optimization.
Two other approaches were considered - moving the nd_target field alone
and collapsing the l3/l4 segments into a single subtable for matching.
Moving any field individually introduces ABI mismatch, and doesn't
completely address the problems with other neighbor discovery related
fields (such as nd_sll/nd_tll). Collapsing the two subtables creates
an issue with datapath flow explosion, since the l3 and l4 fields will
be unwildcarded together (this can be seen with some of the existing
classifier tests).
A simple test is added to showcase the behavior.
Fixes: 476f36e83bc5 ("Classifier: Staged subtable matching.")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2081773
Reported-by: Numan Siddique
Suggested-by: Ilya Maximets
Signed-off-by: Aaron Conole
---
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)
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
