On 30/08/2018 14:33, Simon Horman wrote:
> Hi Pieter,
>
> On Tue, Aug 21, 2018 at 02:49:45PM +0100, Pieter Jansen van Vuuren wrote:
>> Add TC offload support for encapsulating geneve tunnels with options.
>>
>> Signed-off-by: Pieter Jansen van Vuuren
>> <pieter.jansenvanvuu...@netronome.com>
>> Reviewed-by: Simon Horman <simon.hor...@netronome.com>
>
> thanks for your patch. In general I am happy with this and would value any
> feedback from others.
>
> Travis noticed an alignment problem which I have annotated inline.
>
>> ---
>> include/linux/tc_act/tc_tunnel_key.h | 23 ++++++
>> lib/netdev-tc-offloads.c | 30 ++++++++
>> lib/tc.c | 106 ++++++++++++++++++++++++++-
>> lib/tc.h | 2 +
>> 4 files changed, 158 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/tc_act/tc_tunnel_key.h
>> b/include/linux/tc_act/tc_tunnel_key.h
>> index 26cbd2ff1..f13acf17d 100644
>> --- a/include/linux/tc_act/tc_tunnel_key.h
>> +++ b/include/linux/tc_act/tc_tunnel_key.h
>> @@ -47,6 +47,29 @@ enum {
>>
>> #define TCA_TUNNEL_KEY_MAX (__TCA_TUNNEL_KEY_MAX - 1)
>>
>> +enum {
>> + TCA_TUNNEL_KEY_ENC_OPTS_UNSPEC,
>> + TCA_TUNNEL_KEY_ENC_OPTS_GENEVE, /* Nested
>> + * TCA_TUNNEL_KEY_ENC_OPTS_GENEVE
>> + * attributes
>> + */
>> + __TCA_TUNNEL_KEY_ENC_OPTS_MAX,
>> +};
>> +
>> +#define TCA_TUNNEL_KEY_ENC_OPTS_MAX (__TCA_TUNNEL_KEY_ENC_OPTS_MAX - 1)
>> +
>> +enum {
>> + TCA_TUNNEL_KEY_ENC_OPT_GENEVE_UNSPEC,
>> + TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS, /* be16 */
>> + TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE, /* u8 */
>> + TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA, /* 4 to 128 bytes */
>> +
>> + __TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX,
>> +};
>> +
>> +#define TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX \
>> + (__TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX - 1)
>> +
>> #endif /* __KERNEL__ || HAVE_TCA_TUNNEL_KEY_ENC_TTL */
>>
>> #endif /* __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H */
>> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
>> index 7bc745e95..0612b1348 100644
>> --- a/lib/netdev-tc-offloads.c
>> +++ b/lib/netdev-tc-offloads.c
>> @@ -409,6 +409,29 @@ parse_flower_rewrite_to_netlink_action(struct ofpbuf
>> *buf,
>> }
>> }
>>
>> +static void parse_tc_flower_geneve_opts(struct tc_action *action,
>> + struct ofpbuf *buf)
>> +{
>> + int tun_opt_len = action->encap.data.present.len;
>> + size_t geneve_off;
>> + int idx = 0;
>> +
>> + if (!tun_opt_len) {
>> + return;
>> + }
>> +
>> + geneve_off = nl_msg_start_nested(buf, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS);
>> + while (tun_opt_len) {
>> + struct geneve_opt *opt;
>> +
>> + opt = &action->encap.data.opts.gnv[idx];
>> + nl_msg_put(buf, opt, sizeof(struct geneve_opt) + opt->length * 4);
>> + idx += sizeof(struct geneve_opt) + opt->length * 4;
>> + tun_opt_len -= sizeof(struct geneve_opt) + opt->length * 4;
>> + }
>> + nl_msg_end_nested(buf, geneve_off);
>> +}
>> +
>> static int
>> parse_tc_flower_to_match(struct tc_flower *flower,
>> struct match *match,
>> @@ -580,6 +603,7 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>> nl_msg_put_be16(buf, OVS_TUNNEL_KEY_ATTR_TP_DST,
>> action->encap.tp_dst);
>>
>> + parse_tc_flower_geneve_opts(action, buf);
>> nl_msg_end_nested(buf, tunnel_offset);
>> nl_msg_end_nested(buf, set_offset);
>> }
>> @@ -786,6 +810,12 @@ parse_put_flow_set_action(struct tc_flower *flower,
>> struct tc_action *action,
>> action->encap.tp_dst = nl_attr_get_be16(tun_attr);
>> }
>> break;
>> + case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS: {
>> + memcpy(action->encap.data.opts.gnv, nl_attr_get(tun_attr),
>> + nl_attr_get_size(tun_attr));
>> + action->encap.data.present.len = nl_attr_get_size(tun_attr);
>> + }
>> + break;
>> }
>> }
>>
>> diff --git a/lib/tc.c b/lib/tc.c
>> index bbc382326..fcaf7832e 100644
>> --- a/lib/tc.c
>> +++ b/lib/tc.c
>> @@ -659,8 +659,72 @@ static const struct nl_policy tunnel_key_policy[] = {
>> [TCA_TUNNEL_KEY_ENC_DST_PORT] = { .type = NL_A_U16, .optional = true, },
>> [TCA_TUNNEL_KEY_ENC_TOS] = { .type = NL_A_U8, .optional = true, },
>> [TCA_TUNNEL_KEY_ENC_TTL] = { .type = NL_A_U8, .optional = true, },
>> + [TCA_TUNNEL_KEY_ENC_OPTS] = { .type = NL_A_NESTED, .optional =
>> true,},
>> };
>>
>> +static void
>> +nl_parse_act_geneve_opts(const struct nlattr *in_nlattr,
>> + struct tc_action *action)
>> +{
>> + struct geneve_opt *opt = NULL;
>> + const struct ofpbuf *msg;
>> + struct nlattr *nla;
>> + struct ofpbuf buf;
>> + size_t left;
>> + int cnt;
>> +
>> + nl_attr_get_nested(in_nlattr, &buf);
>> + msg = &buf;
>> +
>> + cnt = 0;
>> + NL_ATTR_FOR_EACH (nla, left, ofpbuf_at(msg, 0, 0), msg->size) {
>> + uint16_t type = nl_attr_type(nla);
>> +
>> + switch (type) {
>> + case TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS:
>> + opt = (struct geneve_opt *) &action->encap.data.opts.gnv[cnt];
>
> Clang believe that casting from uint8_t * to unsigned char *
> changes the alignment requirements from 1 to 2.
>
> https://travis-ci.org/horms2/ovs/jobs/422509606
Thanks Simon, I'll fix this up and repost.
>
>> + opt->opt_class = nl_attr_get_be16(nla);
>> + action->encap.data.present.len += sizeof(struct geneve_opt);
>> + cnt += sizeof(struct geneve_opt);
>> + break;
>> + case TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE:
>> + opt->type = nl_attr_get_u8(nla);
>> + break;
>> + case TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA:
>> + opt->length = nl_attr_get_size(nla) / 4;
>> + memcpy(opt + 1, nl_attr_get_unspec(nla, 1), opt->length * 4);
>> + action->encap.data.present.len += opt->length * 4;
>> + cnt += opt->length * 4;
>> + break;
>> + }
>> + }
>> +}
>> +
>> +static void
>> +nl_parse_act_tunnel_opts(struct nlattr *options, struct tc_action *action)
>> +{
>> + const struct ofpbuf *msg;
>> + struct nlattr *nla;
>> + struct ofpbuf buf;
>> + size_t left;
>> +
>> + if (!options) {
>> + return;
>> + }
>> +
>> + nl_attr_get_nested(options, &buf);
>> + msg = &buf;
>> +
>> + NL_ATTR_FOR_EACH (nla, left, ofpbuf_at(msg, 0, 0), msg->size) {
>> + uint16_t type = nl_attr_type(nla);
>> + switch (type) {
>> + case TCA_TUNNEL_KEY_ENC_OPTS_GENEVE:
>> + nl_parse_act_geneve_opts(nla, action);
>> + break;
>> + }
>> + }
>> +}
>> +
>> static int
>> nl_parse_act_tunnel_key(struct nlattr *options, struct tc_flower *flower)
>> {
>> @@ -686,6 +750,7 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct
>> tc_flower *flower)
>> struct nlattr *ipv6_dst = tun_attrs[TCA_TUNNEL_KEY_ENC_IPV6_DST];
>> struct nlattr *tos = tun_attrs[TCA_TUNNEL_KEY_ENC_TOS];
>> struct nlattr *ttl = tun_attrs[TCA_TUNNEL_KEY_ENC_TTL];
>> + struct nlattr *tun_opt = tun_attrs[TCA_TUNNEL_KEY_ENC_OPTS];
>>
>> action = &flower->actions[flower->action_count++];
>> action->type = TC_ACT_ENCAP;
>> @@ -701,6 +766,7 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct
>> tc_flower *flower)
>> action->encap.tp_dst = dst_port ? nl_attr_get_be16(dst_port) : 0;
>> action->encap.tos = tos ? nl_attr_get_u8(tos) : 0;
>> action->encap.ttl = ttl ? nl_attr_get_u8(ttl) : 0;
>> + nl_parse_act_tunnel_opts(tun_opt, action);
>> } else if (tun->t_action == TCA_TUNNEL_KEY_ACT_RELEASE) {
>> flower->tunnel.tunnel = true;
>> } else {
>> @@ -1268,13 +1334,44 @@ nl_msg_put_act_tunnel_key_release(struct ofpbuf
>> *request)
>> nl_msg_end_nested(request, offset);
>> }
>>
>> +static void
>> +nl_msg_put_act_tunnel_geneve_option(struct ofpbuf *request,
>> + uint8_t *tun_options, int tun_opt_len)
>> +{
>> + const struct geneve_opt *opt;
>> + size_t outer, inner;
>> + int idx = 0;
>> +
>> + if (!tun_opt_len) {
>> + return;
>> + }
>> +
>> + outer = nl_msg_start_nested(request, TCA_TUNNEL_KEY_ENC_OPTS);
>> +
>> + while (tun_opt_len) {
>> + opt = (struct geneve_opt *) &tun_options[idx];
>> + inner = nl_msg_start_nested(request,
>> TCA_TUNNEL_KEY_ENC_OPTS_GENEVE);
>> + nl_msg_put_be16(request, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS,
>> + opt->opt_class);
>> + nl_msg_put_u8(request, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE,
>> opt->type);
>> + nl_msg_put_unspec(request, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA, opt
>> + 1,
>> + opt->length * 4);
>> + nl_msg_end_nested(request, inner);
>> +
>> + idx += sizeof(struct geneve_opt) + opt->length * 4;
>> + tun_opt_len -= sizeof(struct geneve_opt) + opt->length * 4;
>> + }
>> +
>> + nl_msg_end_nested(request, outer);
>> +}
>> +
>> static void
>> nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, ovs_be64 id,
>> ovs_be32 ipv4_src, ovs_be32 ipv4_dst,
>> struct in6_addr *ipv6_src,
>> struct in6_addr *ipv6_dst,
>> - ovs_be16 tp_dst,
>> - uint8_t tos, uint8_t ttl)
>> + ovs_be16 tp_dst, uint8_t tos, uint8_t ttl,
>> + void *tun_options, int tun_opt_len)
>> {
>> size_t offset;
>>
>> @@ -1304,6 +1401,7 @@ nl_msg_put_act_tunnel_key_set(struct ofpbuf *request,
>> ovs_be64 id,
>> nl_msg_put_u8(request, TCA_TUNNEL_KEY_ENC_TTL, ttl);
>> }
>> nl_msg_put_be16(request, TCA_TUNNEL_KEY_ENC_DST_PORT, tp_dst);
>> + nl_msg_put_act_tunnel_geneve_option(request, tun_options,
>> tun_opt_len);
>> }
>> nl_msg_end_nested(request, offset);
>> }
>> @@ -1546,7 +1644,9 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct
>> tc_flower *flower)
>> &action->encap.ipv6.ipv6_dst,
>> action->encap.tp_dst,
>> action->encap.tos,
>> - action->encap.ttl);
>> + action->encap.ttl,
>> + action->encap.data.opts.gnv,
>> +
>> action->encap.data.present.len);
>> nl_msg_end_nested(request, act_offset);
>> }
>> break;
>> diff --git a/lib/tc.h b/lib/tc.h
>> index aa8805df2..4ae06eb27 100644
>> --- a/lib/tc.h
>> +++ b/lib/tc.h
>> @@ -27,6 +27,7 @@
>> #include "odp-netlink.h"
>> #include "openvswitch/ofpbuf.h"
>> #include "openvswitch/flow.h"
>> +#include "openvswitch/tun-metadata.h"
>>
>> /* For backwards compatability with older kernels */
>> #ifndef TC_H_CLSACT
>> @@ -140,6 +141,7 @@ struct tc_action {
>> struct in6_addr ipv6_src;
>> struct in6_addr ipv6_dst;
>> } ipv6;
>> + struct tun_metadata data;
>> } encap;
>> };
>>
>> --
>> 2.17.0
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
