Eelco Chaudron <echau...@redhat.com> writes:
> When TC parses the flow to install, it assumes that the datalink type
> mask is set. However, this may not always be the case, for example,
> when multiple VLANs exist but only one is enabled (vlan-limit).
>
> This patch will only process the dl_type if the mask is set. It also
> includes a unit test to verify that the TC rules are offloaded in this
> case.
>
> Fixes: 1be33d52af77 ("netdev-tc-offloads: Don't offload header modification
> on ip fragments.")
> Signed-off-by: Eelco Chaudron <echau...@redhat.com>
>
> ---
> v3: - Fixed all Ilya's style comments
> v2: - Adding is_ip_any() function that got deleted somehow
> - Fixed alignment
> ---
> lib/netdev-offload-tc.c | 11 +++++++----
> tests/system-offloads-traffic.at | 32 +++++++++++++++++++++++++++++++-
> 2 files changed, 38 insertions(+), 5 deletions(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 9e163c2a6..38ea95bd4 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -2293,6 +2293,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> const struct flow_tnl *tnl = &match->flow.tunnel;
> struct flow_tnl *tnl_mask = &mask->tunnel;
> struct dpif_flow_stats adjust_stats;
> + bool exact_match_on_dl_type;
> bool recirc_act = false;
> uint32_t block_id = 0;
> struct tcf_id id;
> @@ -2310,6 +2311,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
>
> memset(&flower, 0, sizeof flower);
>
> + exact_match_on_dl_type = mask->dl_type == htons(0xffff);
I guess the only thing you could consider changing is putting this
assignment right at the point that we declare exact_match_on_dl_type. I
don't think it is too big a deal either way - consider it a very minor
nit.
Either way LGTM -
Acked-by: Aaron Conole <acon...@redhat.com>
> chain = key->recirc_id;
> mask->recirc_id = 0;
>
> @@ -2503,7 +2505,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> mask->dl_type = 0;
> mask->in_port.odp_port = 0;
>
> - if (key->dl_type == htons(ETH_P_ARP)) {
> + if (exact_match_on_dl_type && key->dl_type == htons(ETH_P_ARP)) {
> flower.key.arp.spa = key->nw_src;
> flower.key.arp.tpa = key->nw_dst;
> flower.key.arp.sha = key->arp_sha;
> @@ -2522,7 +2524,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> memset(&mask->arp_tha, 0, sizeof mask->arp_tha);
> }
>
> - if (is_ip_any(key) && !is_ipv6_fragment_and_masked(key, mask)) {
> + if (exact_match_on_dl_type && is_ip_any(key)
> + && !is_ipv6_fragment_and_masked(key, mask)) {
> flower.key.ip_proto = key->nw_proto;
> flower.mask.ip_proto = mask->nw_proto;
> mask->nw_proto = 0;
> @@ -2552,9 +2555,9 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> } else {
> /* This scenario should not occur. Currently, all installed IP
> DP
> * flows perform a fully masked match on the fragmentation bits.
> - * However, since TC depends on this behavior, we return ENOTSUPP
> + * However, since TC depends on this behavior, we return
> EOPNOTSUPP
> * for now in case this behavior changes in the future. */
> - return EOPNOTSUPP;
> + return EOPNOTSUPP;
> }
>
> if (key->nw_proto == IPPROTO_TCP) {
> diff --git a/tests/system-offloads-traffic.at
> b/tests/system-offloads-traffic.at
> index 78c6f5d7e..32c0d2f2a 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -1016,4 +1016,34 @@ AT_CHECK(
> stdout])
>
> OVS_TRAFFIC_VSWITCHD_STOP
> -AT_CLEANUP
> \ No newline at end of file
> +AT_CLEANUP
> +
> +AT_SETUP([offloads - 802.1ad should be offloaded])
> +OVS_TRAFFIC_VSWITCHD_START(
> + [], [], [-- set Open_vSwitch . other_config:hw-offload=true])
> +OVS_CHECK_8021AD()
> +
> +ADD_NAMESPACES(at_ns0, at_ns1)
> +
> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24")
> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24")
> +
> +ADD_SVLAN(p0, at_ns0, 4094, "10.255.2.1/24")
> +ADD_SVLAN(p1, at_ns1, 4094, "10.255.2.2/24")
> +
> +ADD_CVLAN(p0.4094, at_ns0, 100, "10.2.2.1/24")
> +ADD_CVLAN(p1.4094, at_ns1, 100, "10.2.2.2/24")
> +
> +AT_CHECK([ovs-ofctl add-flow br0 "priority=1 action=normal"])
> +
> +OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.2.2.2])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows type=tc,offloaded | grep
> "eth_type(0x0800)" | DUMP_CLEAN_SORTED], [0], [dnl
> +in_port(2),eth(macs),eth_type(0x88a8),vlan(vid=4094,pcp=0),encap(eth_type(0x0800)),
> packets:0, bytes:0, used:0.001s, actions:output
> +in_port(3),eth(macs),eth_type(0x88a8),vlan(vid=4094,pcp=0),encap(eth_type(0x0800)),
> packets:0, bytes:0, used:0.001s, actions:output
> +])
> +
> +AT_CHECK([ovs-appctl dpctl/dump-flows type=ovs | grep "eth_type(0x0800)" |
> DUMP_CLEAN_SORTED], [0], [])
> +
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
