Thanks, Eelco! The patch looks good to me technically, but see some
minor comments and nits below.
On 1/28/25 10:23, Eelco Chaudron wrote:
> 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.
Nit: It's better to limit commit message lines to 72, as they are extra
indented while printing. Just to be consistent withing the git log.
>
> Fixes: 1be33d52af77 ("netdev-tc-offloads: Don't offload header modification
> on ip fragments.")
> Signed-off-by: Eelco Chaudron <echau...@redhat.com>
>
> ---
> v2: - Adding is_ip_any() function that got deleted somehow
> - Fixed alignment
> ---
> lib/netdev-offload-tc.c | 9 ++++++---
> tests/system-offloads-traffic.at | 30 ++++++++++++++++++++++++++++++
> 2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 9e163c2a6..732acfc97 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -2294,6 +2294,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> struct flow_tnl *tnl_mask = &mask->tunnel;
> struct dpif_flow_stats adjust_stats;
> bool recirc_act = false;
> + bool match_on_dl_type;
> uint32_t block_id = 0;
> struct tcf_id id;
> uint32_t chain;
> @@ -2310,6 +2311,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
>
> memset(&flower, 0, sizeof flower);
>
> + match_on_dl_type = mask->dl_type == htons(0xffff);
I'd add an 'exact_' prefix to the variable to make it reflect what
it actually means. The code below seems to allow masked matches
on the dl_type, so the name is a little confusing without the prefix.
> 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 (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 (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;
> @@ -2554,7 +2557,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> * flows perform a fully masked match on the fragmentation bits.
> * However, since TC depends on this behavior, we return ENOTSUPP
> * for now in case this behavior changes in the future. */
> - return EOPNOTSUPP;
> + return EOPNOTSUPP;
Nit: If you want to fix the indentation here, might as well fix the error
code in the comment (missing 'OP').
> }
>
> if (key->nw_proto == IPPROTO_TCP) {
> diff --git a/tests/system-offloads-traffic.at
> b/tests/system-offloads-traffic.at
> index 78c6f5d7e..479126398 100644
> --- a/tests/system-offloads-traffic.at
> +++ b/tests/system-offloads-traffic.at
> @@ -1015,5 +1015,35 @@ AT_CHECK(
> [grep -q -F "set(tunnel(dst=172.31.1.1,ttl=64,tp_dst=4789,flags(csum)))" \
> stdout])
>
> +OVS_TRAFFIC_VSWITCHD_STOP
> +AT_CLEANUP
> +
> +AT_SETUP([offloads - 802.1ad should be offloaded])
> +OVS_TRAFFIC_VSWITCHD_START([], [], [-- set Open_vSwitch . \
> + other_config:hw-offload=true])
Nit: Might be better to just move the arguments to a new line indented
by 2 spaces, instead of splitting the arg.
> +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
> \ No newline at end of file
Nit: Since, you're adding a new test to the end of the file, could you,
please, add the new line character to the end?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
