On 8/2/22 09:13, Roi Dayan wrote:
>
>
> On 2022-08-02 9:53 AM, Roi Dayan wrote:
>>
>>
>> On 2022-08-01 9:31 AM, Roi Dayan wrote:
>>>
>>>
>>> On 2022-07-29 5:53 PM, 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, destination port as well as CSUM configuration for unknown
>>>> reason was not taken from the actions list and were passed via HW
>>>> offload info instead of being consumed from the set() action.
>>>>
>>>> Reported-at:
>>>> https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395522.html
>>>> Reported-by: Eelco Chaudron <echau...@redhat.com>
>>>> Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using
>>>> tc interface")
>>>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
>>>> ---
>>>> lib/dpif-netlink.c | 14 +---------
>>>> lib/netdev-offload-tc.c | 61 ++++++++++++++++++++++++++++++++++++-----
>>>> lib/netdev-offload.h | 3 --
>>>> 3 files changed, 55 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>>>> index a498d5667..d9243a735 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 a3eee8df3..57385597a 100644
>>>> --- a/lib/netdev-offload-tc.c
>>>> +++ b/lib/netdev-offload-tc.c
>>>> @@ -1389,9 +1389,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);
>>>> @@ -1405,6 +1407,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++;
>>>> @@ -1431,6 +1434,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);
>>>> @@ -1455,9 +1471,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;
>>>> }
>>>> @@ -1553,7 +1577,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;
>>>> @@ -1579,6 +1603,10 @@ flower_match_to_tun_opt(struct tc_flower *flower,
>>>> const struct flow_tnl *tnl,
>>>> /* Copying from the key and not from the mask, since in the 'flower'
>>>> * the length for a mask is not a mask, but the actual length. */
>>>> 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
>>>> @@ -1867,10 +1895,6 @@ netdev_tc_parse_nl_actions(struct netdev *netdev,
>>>> struct tc_flower *flower,
>>>> 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);
>>>> @@ -1946,7 +1970,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;
>>>> bool recirc_act = false;
>>>> uint32_t block_id = 0;
>>>> struct tcf_id id;
>>>> @@ -1984,6 +2008,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct
>>>> match *match,
>>>> flower.key.tunnel.ttl = tnl->ip_ttl;
>>>> flower.key.tunnel.tp_src = tnl->tp_src;
>>>> flower.key.tunnel.tp_dst = tnl->tp_dst;
>>>> +
>>>> flower.mask.tunnel.ipv4.ipv4_src = tnl_mask->ip_src;
>>>> flower.mask.tunnel.ipv4.ipv4_dst = tnl_mask->ip_dst;
>>>> flower.mask.tunnel.ipv6.ipv6_src = tnl_mask->ipv6_src;
>>>> @@ -1996,10 +2021,32 @@ netdev_tc_flow_put(struct netdev *netdev, struct
>>>> match *match,
>>>> * Degrading the flow down to exact match for now as a
>>>> workaround. */
>>>> flower.mask.tunnel.tp_dst = OVS_BE16_MAX;
>>>> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ?
>>>> tnl_mask->tun_id : 0;
>>>> +
>>>> + 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);
>>>> + memset(&tnl_mask->tp_dst, 0, sizeof tnl_mask->tp_dst);
>>>> +
>>>> + memset(&tnl_mask->tun_id, 0, sizeof tnl_mask->tun_id);
>>>> + 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;
>>>> + } else {
>>>> + /* There is no tunnel metadata to match on, but there could be
>>>> some
>>>> + * mask bits set due to flow translation artifacts. Clear them.
>>>> */
>>>> + memset(&mask->tunnel, 0, sizeof mask->tunnel);
>>>> }
>>>> - memset(&mask->tunnel, 0, sizeof mask->tunnel);
>>>> flower.key.eth_type = key->dl_type;
>>>> flower.mask.eth_type = mask->dl_type;
>>>> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
>>>> index 249a3102a..180d3f95f 100644
>>>> --- a/lib/netdev-offload.h
>>>> +++ b/lib/netdev-offload.h
>>>> @@ -66,9 +66,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. */
>>>
>>> Reviewed-by: Roi Dayan <r...@nvidia.com>
>>
>> Hi,
>>
>> Maybe replied too soon. I tested manually vxlan but also added
>> the patches for our verification team to run their regression
>> at night and they failed using geneve tunnel for mask not being
>> zero.
>>
>> Checking it now.
>>
>> Thanks,
>> Roi
>
> ok found the issue.
> the patch is missing removing the FLOW_TNL_F_UDPIF flag when parsing
> tunnel options.
> this fixes the issue.
> our verification team didnt encounter any other issue.
>
>
> @@ -1621,6 +1633,7 @@ flower_match_to_tun_opt(struct tc_flower *flower, const
> struct flow_tnl *tnl,
> memset(tnl_mask->metadata.opts.gnv, 0, tnl->metadata.present.len);
> memset(&tnl_mask->metadata.present.len, 0,
> sizeof tnl_mask->metadata.present.len);
> + tnl_mask->flags &= ~FLOW_TNL_F_UDPIF;
> }
>
>
Good point. It looks like we also need to reject offload if
that flag is not set, right? This way we will actually consume
it instead of clearing blindly.
Something like this:
static void
flower_match_to_tun_opt(struct tc_flower *flower, const struct flow_tnl *tnl,
struct flow_tnl *tnl_mask)
{
struct geneve_opt *opt, *opt_mask;
int len, cnt = 0;
if (tnl->flags & (tnl_mask->flags & FLOW_TNL_F_UDPIF)) {
tnl_mask->flags &= ~FLOW_TNL_F_UDPIF;
} else {
return;
}
...
}
What do you think?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
