On 28 Jan 2025, at 21:12, Ilya Maximets wrote:
> Thanks, Eelco! The patch looks good to me technically, but see some
> minor comments and nits below.
Thanks for the review Ilya! I fixed all the nits, and sent out a v3. Please
take a look.
Cheers,
Eelco
> 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 <[email protected]>
>>
>> ---
>> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
