On 7/13/22 12:11, Roi Dayan wrote: > > > On 2022-07-13 12:01 PM, Roi Dayan wrote: >> >> >> On 2022-07-06 4:58 PM, Roi Dayan wrote: >>> >>> >>> On 2022-07-05 1:45 AM, Ilya Maximets wrote: >>>> Current offloading code supports only limited number of tunnel keys >>>> and silently ignores everything it doesn't understand. This is >>>> causing, for example, offloaded ERSPAN tunnels to not work, because >>>> flow is offloaded, but ERSPAN options are not provided to TC. >>>> >>>> There is a number of tunnel keys, which are supported by the userspace, >>>> but silently ignored during offloading: >>>> >>>> OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT >>>> OVS_TUNNEL_KEY_ATTR_OAM >>>> OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS >>>> OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS >>>> >>>> OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions >>>> and for some reason is set from the tunnel port instead of the >>>> provided action, and not currently supported for the tunnel key in >>>> the match. >>>> >>>> Addig a default case to fail offloading of unknown attributes. For >>>> now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag, >>>> otherwise we'll break all tunnel offloading by default. VXLAN and >>>> ERSPAN options has to fail offloading, because the tunnel will not >>>> work otherwise. OAM is not a default configurations, so failing it >>>> as well. The missing DONT_FRAGMENT flag though should, probably, >>>> cause frequent flow revalidation, but that is not new with this patch. >>>> >>>> Same for the 'match' key, only clearing masks that was actually >>>> consumed, except for the DONT_FRAGMENT and CSUM flags, which are >>>> explicitly allowed and highlighted as broken. >>>> >>>> Also, ttl and tos were incorrectly checked by the value instead of a >>>> mask during the flow key dump. Destination port as well as CSUM >>>> configuration for unknown reason was not taken from the actions list >>>> and were passed via HW offload info. >>>> >>>> Reported-at: >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395522.html >>>> Reported-by: Eelco Chaudron <[email protected]> >>>> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using >>>> tc interface") >>>> Signed-off-by: Ilya Maximets <[email protected]> >>>> --- >>>> lib/dpif-netlink.c | 14 +--------- >>>> lib/netdev-offload-tc.c | 57 ++++++++++++++++++++++++++++++++++------- >>>> lib/netdev-offload.h | 3 --- >>>> 3 files changed, 49 insertions(+), 25 deletions(-) >>>> >>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>>> index 06e1e8ca0..1e116b4ad 100644 >>>> --- a/lib/dpif-netlink.c >>>> +++ b/lib/dpif-netlink.c >>>> @@ -2237,8 +2237,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct >>>> dpif_flow_put *put) >>>> size_t left; >>>> struct netdev *dev; >>>> struct offload_info info; >>>> - ovs_be16 dst_port = 0; >>>> - uint8_t csum_on = false; >>>> int err; >>>> info.tc_modify_flow_deleted = false; >>>> @@ -2258,10 +2256,9 @@ parse_flow_put(struct dpif_netlink *dpif, struct >>>> dpif_flow_put *put) >>>> return EOPNOTSUPP; >>>> } >>>> - /* Get tunnel dst port */ >>>> + /* Check the output port for a tunnel. */ >>>> NL_ATTR_FOR_EACH(nla, left, put->actions, put->actions_len) { >>>> if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) { >>>> - const struct netdev_tunnel_config *tnl_cfg; >>>> struct netdev *outdev; >>>> odp_port_t out_port; >>>> @@ -2271,19 +2268,10 @@ parse_flow_put(struct dpif_netlink *dpif, struct >>>> dpif_flow_put *put) >>>> err = EOPNOTSUPP; >>>> goto out; >>>> } >>>> - tnl_cfg = netdev_get_tunnel_config(outdev); >>>> - if (tnl_cfg && tnl_cfg->dst_port != 0) { >>>> - dst_port = tnl_cfg->dst_port; >>>> - } >>>> - if (tnl_cfg) { >>>> - csum_on = tnl_cfg->csum; >>>> - } >>>> netdev_close(outdev); >>>> } >>>> } >>>> - info.tp_dst_port = dst_port; >>>> - info.tunnel_csum_on = csum_on; >>>> info.recirc_id_shared_with_tc = (dpif->user_features >>>> & OVS_DP_F_TC_RECIRC_SHARING); >>>> err = netdev_flow_put(dev, &match, >>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>>> index 262faf3c6..e62687c82 100644 >>>> --- a/lib/netdev-offload-tc.c >>>> +++ b/lib/netdev-offload-tc.c >>>> @@ -805,11 +805,11 @@ parse_tc_flower_to_match(struct tc_flower *flower, >>>> &flower->key.tunnel.ipv6.ipv6_src, >>>> &flower->mask.tunnel.ipv6.ipv6_src); >>>> } >>>> - if (flower->key.tunnel.tos) { >>>> + if (flower->mask.tunnel.tos) { >>>> match_set_tun_tos_masked(match, flower->key.tunnel.tos, >>>> flower->mask.tunnel.tos); >>>> } >>>> - if (flower->key.tunnel.ttl) { >>>> + if (flower->mask.tunnel.ttl) { >>>> match_set_tun_ttl_masked(match, flower->key.tunnel.ttl, >>>> flower->mask.tunnel.ttl); >>>> } >>>> @@ -1284,9 +1284,11 @@ static int >>>> parse_put_flow_set_action(struct tc_flower *flower, struct tc_action >>>> *action, >>>> const struct nlattr *set, size_t set_len) >>>> { >>>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); >>>> const struct nlattr *tunnel; >>>> const struct nlattr *tun_attr; >>>> size_t tun_left, tunnel_len; >>>> + bool attr_csum_present; >>>> if (nl_attr_type(set) == OVS_KEY_ATTR_MPLS) { >>>> return parse_mpls_set_action(flower, action, set); >>>> @@ -1300,6 +1302,7 @@ parse_put_flow_set_action(struct tc_flower *flower, >>>> struct tc_action *action, >>>> tunnel = nl_attr_get(set); >>>> tunnel_len = nl_attr_get_size(set); >>>> + attr_csum_present = false; >>>> action->type = TC_ACT_ENCAP; >>>> action->encap.id_present = false; >>>> flower->action_count++; >>>> @@ -1326,6 +1329,19 @@ parse_put_flow_set_action(struct tc_flower *flower, >>>> struct tc_action *action, >>>> action->encap.ttl = nl_attr_get_u8(tun_attr); >>>> } >>>> break; >>>> + case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT: { >>>> + /* XXX: This is wrong! We're ignoring the DF flag >>>> configuration >>>> + * requested by the user. However, TC for now has no way to >>>> pass >>>> + * that flag and it is set by default, meaning tunnel >>>> offloading >>>> + * will not work if 'options:df_default=false' is not set. >>>> + * Keeping incorrect behavior for now. */ >>>> + } >>>> + break; >>>> + case OVS_TUNNEL_KEY_ATTR_CSUM: { >>>> + attr_csum_present = true; >>>> + action->encap.no_csum = !nl_attr_get_u8(tun_attr); >>>> + } >>>> + break; >>>> case OVS_TUNNEL_KEY_ATTR_IPV6_SRC: { >>>> action->encap.ipv6.ipv6_src = >>>> nl_attr_get_in6_addr(tun_attr); >>>> @@ -1350,9 +1366,17 @@ parse_put_flow_set_action(struct tc_flower *flower, >>>> struct tc_action *action, >>>> action->encap.data.present.len = nl_attr_get_size(tun_attr); >>>> } >>>> break; >>>> + default: >>>> + VLOG_DBG_RL(&rl, "unsupported tunnel key attribute %d", >>>> + nl_attr_type(tun_attr)); >>>> + return EOPNOTSUPP; >>>> } >>>> } >>>> + if (!attr_csum_present) { >>>> + action->encap.no_csum = 1; >>>> + } >>>> + >>>> return 0; >>>> } >>>> @@ -1448,7 +1472,7 @@ test_key_and_mask(struct match *match) >>>> static void >>>> flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl >>>> *tnl, >>>> - const struct flow_tnl *tnl_mask) >>>> + struct flow_tnl *tnl_mask) >>>> { >>>> struct geneve_opt *opt, *opt_mask; >>>> int len, cnt = 0; >>>> @@ -1472,6 +1496,10 @@ flower_match_to_tun_opt(struct tc_flower *flower, >>>> const struct flow_tnl *tnl, >>>> } >>>> flower->mask.tunnel.metadata.present.len = tnl->metadata.present.len; >>>> + >>>> + memset(tnl_mask->metadata.opts.gnv, 0, tnl->metadata.present.len); >>>> + memset(&tnl_mask->metadata.present.len, 0, >>>> + sizeof tnl_mask->metadata.present.len); >>>> } >>>> static void >>>> @@ -1576,7 +1604,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct >>>> match *match, >>>> const struct flow *key = &match->flow; >>>> struct flow *mask = &match->wc.masks; >>>> const struct flow_tnl *tnl = &match->flow.tunnel; >>>> - const struct flow_tnl *tnl_mask = &mask->tunnel; >>>> + struct flow_tnl *tnl_mask = &mask->tunnel; >>>> struct tc_action *action; >>>> bool recirc_act = false; >>>> uint32_t block_id = 0; >>>> @@ -1624,10 +1652,25 @@ netdev_tc_flow_put(struct netdev *netdev, struct >>>> match *match, >>>> flower.mask.tunnel.tos = tnl_mask->ip_tos; >>>> flower.mask.tunnel.ttl = tnl_mask->ip_ttl; >>>> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? >>>> tnl_mask->tun_id : 0; >>>> + memset(&tnl_mask->tun_id, 0, sizeof tnl_mask->tun_id); >>>> + memset(&tnl_mask->ip_src, 0, sizeof tnl_mask->ip_src); >>>> + memset(&tnl_mask->ip_dst, 0, sizeof tnl_mask->ip_dst); >>>> + memset(&tnl_mask->ipv6_src, 0, sizeof tnl_mask->ipv6_src); >>>> + memset(&tnl_mask->ipv6_dst, 0, sizeof tnl_mask->ipv6_dst); >>>> + memset(&tnl_mask->ip_tos, 0, sizeof tnl_mask->ip_tos); >>>> + memset(&tnl_mask->ip_ttl, 0, sizeof tnl_mask->ip_ttl); >>>> + tnl_mask->flags &= ~FLOW_TNL_F_KEY; >>>> + >>>> + /* XXX: This is wrong! We're ignoring DF and CSUM flags >>>> configuration >>>> + * requested by the user. However, TC for now has no way to pass >>>> + * these flags in a flower key and their masks are set by default, >>>> + * meaning tunnel offloading will not work at all if not cleared. >>>> + * Keeping incorrect behavior for now. */ >>>> + tnl_mask->flags &= ~(FLOW_TNL_F_DONT_FRAGMENT | FLOW_TNL_F_CSUM); >>>> + >>>> flower_match_to_tun_opt(&flower, tnl, tnl_mask); >>>> flower.tunnel = true; >>>> } >>>> - memset(&mask->tunnel, 0, sizeof mask->tunnel); >>>> flower.key.eth_type = key->dl_type; >>>> flower.mask.eth_type = mask->dl_type; >>>> @@ -1899,10 +1942,6 @@ netdev_tc_flow_put(struct netdev *netdev, struct >>>> match *match, >>>> if (err) { >>>> return err; >>>> } >>>> - if (action->type == TC_ACT_ENCAP) { >>>> - action->encap.tp_dst = info->tp_dst_port; >>>> - action->encap.no_csum = !info->tunnel_csum_on; >>>> - } >>>> } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_SET_MASKED) { >>>> const struct nlattr *set = nl_attr_get(nla); >>>> const size_t set_len = nl_attr_get_size(nla); >>>> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h >>>> index 8237a85dd..93eb2df48 100644 >>>> --- a/lib/netdev-offload.h >>>> +++ b/lib/netdev-offload.h >>>> @@ -65,9 +65,6 @@ struct netdev_flow_dump { >>>> /* Flow offloading. */ >>>> struct offload_info { >>>> - ovs_be16 tp_dst_port; /* Destination port for tunnel in SET action */ >>>> - uint8_t tunnel_csum_on; /* Tunnel header with checksum */ >>>> - >>>> bool recirc_id_shared_with_tc; /* Indicates whever tc chains will >>>> be in >>>> * sync with datapath recirc ids. */ >>> >>> Acked-by: Roi Dayan <[email protected]> >> >> hi, >> >> I did ack too early maybe I did some more tests and found dpctl rule I >> could add before this commit now fail with this commit. >> >> I created an ovs bridge with a vxlan port and tried the following: >> >> ovs-appctl dpctl/add-flow >> 'ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5,recirc_id(0),in_port(2),eth_type(0x0800),eth(src=00:00:00:00:00:00/00:00:00:00:00:00,dst=fa:16:3e:2a:4e:23),tunnel(tun_id=0x65,src=10.10.11.3,dst=10.10.11.2,ttl=0/0,tp_dst=4789,flags(+key)),ipv4(src=0.0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=0/0,tos=0/0x3,ttl=0/0,frag=no)' >> >> 'set(tunnel(tun_id=0x66,src=10.10.12.2,dst=10.10.12.3,tp_dst=4789,flags(key))),2' >> >> ovs-vswitchd: updating flow table (Invalid argument) >> ovs-appctl: ovs-vswitchd: server returned an error >> >> >> Other rule like the following that was offloaded to TC before this >> commit now not added to TC. added ovs dbg and got the error in the log. >> >> ovs-appctl dpctl/add-flow >> 'ufid:c5f9a0b1-3399-4436-b742-30825c64a1e5,recirc_id(0),tunnel(tun_id=0x2a,src=2.2.2.3,dst=2.2.2.2,tp_dst=4789,ttl=64/0),in_port(3),eth(src=56:52:2d:21:4d:93,dst=92:c1:04:ce:fd:51),eth_type(0x0800),ipv4(src=1.1.1.1)' >> 2 >> >> 2022-07-13T08:59:36.100Z|00053|netdev_offload_tc|DBG|offloading isn't >> supported, unknown attribute >> >> I need to go over the commit again and debug for more detailed info. >> >> Thanks, >> Roi > > > > In netdev_tc_flow_put() since you removed the memset on the tunnel > and reset per field you need to reset also tp_src/dst to keep working > and not break at this commit. > > + memset(&tnl_mask->tp_src, 0, sizeof tnl_mask->tp_src); > + memset(&tnl_mask->tp_dst, 0, sizeof tnl_mask->tp_dst); > > > this solves the issues I encountered and my tests pass. >
Good catch. Yes, we need to move that part from the patch #2 to this one, even though we're not using these masks. We're still using keys themselves, but with exact match instead. Is it the only issue you found with this change? _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
