| Message ID | 20230515082356.3471136-8-roid@nvidia.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | Add vxlan gbp offload with TC | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Mon, May 15, 2023 at 11:23:56AM +0300, Roi Dayan via dev wrote: > From: Gavin Li <gavinl@nvidia.com> > > Add TC offload support for vxlan encap with gbp option > > Signed-off-by: Gavin Li <gavinl@nvidia.com> > Reviewed-by: Gavi Teitz <gavi@nvidia.com> > Reviewed-by: Roi Dayan <roid@nvidia.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 23/05/2023 19:06, Simon Horman wrote: > On Mon, May 15, 2023 at 11:23:56AM +0300, Roi Dayan via dev wrote: >> From: Gavin Li <gavinl@nvidia.com> >> >> Add TC offload support for vxlan encap with gbp option >> >> Signed-off-by: Gavin Li <gavinl@nvidia.com> >> Reviewed-by: Gavi Teitz <gavi@nvidia.com> >> Reviewed-by: Roi Dayan <roid@nvidia.com> > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > thanks for review Simon!
On 15 May 2023, at 10:23, Roi Dayan wrote: > From: Gavin Li <gavinl@nvidia.com> > > Add TC offload support for vxlan encap with gbp option > > Signed-off-by: Gavin Li <gavinl@nvidia.com> > Reviewed-by: Gavi Teitz <gavi@nvidia.com> > Reviewed-by: Roi Dayan <roid@nvidia.com> I have a couple of comments below, and some general ones. 1) Can we add some unit tests for this to check-offload? 2) Can you explain what happens if the kernel does not support the newly added option? I guess it will return -EINVAL when trying to install a flow, which will probably result in an error message. This is because EOPNOTSUPP is the only one ignored as not a problem. So maybe we need some probing? This does conclude my review of v3. //Eelco > --- > acinclude.m4 | 7 ++++ > include/linux/tc_act/tc_tunnel_key.h | 17 +++++++- > lib/netdev-offload-tc.c | 31 ++++++++++++++- > lib/odp-util.c | 41 ++++++++++++++------ > lib/odp-util.h | 4 +- > lib/tc.c | 58 ++++++++++++++++++++++++++++ > lib/tc.h | 1 + > 7 files changed, 143 insertions(+), 16 deletions(-) > > diff --git a/acinclude.m4 b/acinclude.m4 > index ac1eab790041..690a13c25967 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -191,6 +191,13 @@ AC_DEFUN([OVS_CHECK_LINUX_TC], [ > [AC_DEFINE([HAVE_TCA_TUNNEL_KEY_ENC_TTL], [1], > [Define to 1 if TCA_TUNNEL_KEY_ENC_TTL is available.])]) > > + AC_COMPILE_IFELSE([ > + AC_LANG_PROGRAM([#include <linux/tc_act/tc_tunnel_key.h>], [ > + int x = TCA_TUNNEL_KEY_ENC_OPTS_VXLAN; > + ])], > + [AC_DEFINE([HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN], [1], > + [Define to 1 if TCA_TUNNEL_KEY_ENC_OPTS_VXLAN is available.])]) > + > AC_COMPILE_IFELSE([ > AC_LANG_PROGRAM([#include <linux/tc_act/tc_pedit.h>], [ > int x = TCA_PEDIT_KEY_EX_HDR_TYPE_UDP; > diff --git a/include/linux/tc_act/tc_tunnel_key.h b/include/linux/tc_act/tc_tunnel_key.h > index f13acf17dd75..17291b90bf3c 100644 > --- a/include/linux/tc_act/tc_tunnel_key.h > +++ b/include/linux/tc_act/tc_tunnel_key.h > @@ -1,7 +1,7 @@ > #ifndef __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H > #define __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H 1 > > -#if defined(__KERNEL__) || defined(HAVE_TCA_TUNNEL_KEY_ENC_TTL) > +#if defined(__KERNEL__) || defined(HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN) > #include_next <linux/tc_act/tc_tunnel_key.h> > #else > > @@ -53,6 +53,10 @@ enum { > * TCA_TUNNEL_KEY_ENC_OPTS_GENEVE > * attributes > */ > + TCA_TUNNEL_KEY_ENC_OPTS_VXLAN, /* Nested > + * TCA_TUNNEL_KEY_ENC_OPTS_VXLAN > + * attributes > + */ > __TCA_TUNNEL_KEY_ENC_OPTS_MAX, > }; > > @@ -70,6 +74,15 @@ enum { > #define TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX \ > (__TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX - 1) > > -#endif /* __KERNEL__ || HAVE_TCA_TUNNEL_KEY_ENC_TTL */ > +enum { > + TCA_TUNNEL_KEY_ENC_OPT_VXLAN_UNSPEC, > + TCA_TUNNEL_KEY_ENC_OPT_VXLAN_GBP, /* u32 */ > + __TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX, > +}; > + > +#define TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX \ > + (__TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX - 1) > + > +#endif /* __KERNEL__ || HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN */ > > #endif /* __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H */ > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 1c97681bc92b..a32ed8021c4b 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -668,6 +668,24 @@ static void parse_tc_flower_geneve_opts(struct tc_action *action, > nl_msg_end_nested(buf, geneve_off); > } > > +static void > +parse_tc_flower_vxlan_tun_opts(struct tc_action *action, > + struct ofpbuf *buf) > +{ > + size_t gbp_off; > + uint32_t gbp_raw; > + > + if (!action->encap.gbp.id_present) { > + return; > + } > + > + gbp_off = nl_msg_start_nested(buf, OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS); > + gbp_raw = odp_encode_gbp_raw(action->encap.gbp.flags, > + action->encap.gbp.id); > + nl_msg_put_u32(buf, OVS_VXLAN_EXT_GBP, gbp_raw); > + nl_msg_end_nested(buf, gbp_off); > +} > + > static void > flower_tun_opt_to_match(struct match *match, struct tc_flower *flower) > { > @@ -863,7 +881,7 @@ parse_tc_flower_to_actions__(struct tc_flower *flower, struct ofpbuf *buf, > if (!action->encap.no_csum) { > nl_msg_put_flag(buf, OVS_TUNNEL_KEY_ATTR_CSUM); > } > - > + parse_tc_flower_vxlan_tun_opts(action, buf); > parse_tc_flower_geneve_opts(action, buf); > nl_msg_end_nested(buf, tunnel_offset); > nl_msg_end_nested(buf, set_offset); > @@ -1552,6 +1570,7 @@ parse_put_flow_set_action(struct tc_flower *flower, struct tc_action *action, > > action->type = TC_ACT_ENCAP; > action->encap.id_present = false; > + action->encap.gbp.id_present = false; > action->encap.no_csum = 1; > flower->action_count++; > NL_ATTR_FOR_EACH_UNSAFE(tun_attr, tun_left, tunnel, tunnel_len) { > @@ -1613,6 +1632,16 @@ 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; > + case OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS: { > + if (odp_vxlan_tun_opts_from_attr(tun_attr, > + &action->encap.gbp.id, > + &action->encap.gbp.flags, > + &action->encap.gbp.id_present)) { > + VLOG_ERR_RL(&rl, "error parsing VXLAN options"); > + return EINVAL; > + } > + } > + break; > default: > VLOG_DBG_RL(&rl, "unsupported tunnel key attribute %d", > nl_attr_type(tun_attr)); > diff --git a/lib/odp-util.c b/lib/odp-util.c > index bf34c61fec58..34c9001f85fc 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -3149,22 +3149,13 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, > tun->flags |= FLOW_TNL_F_OAM; > break; > case OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS: { > - static const struct nl_policy vxlan_opts_policy[] = { > - [OVS_VXLAN_EXT_GBP] = { .type = NL_A_U32 }, > - }; > - struct nlattr *ext[ARRAY_SIZE(vxlan_opts_policy)]; > - > - if (!nl_parse_nested(a, vxlan_opts_policy, ext, ARRAY_SIZE(ext))) { > + if (odp_vxlan_tun_opts_from_attr(a, &tun->gbp_id, > + &tun->gbp_flags, > + NULL)) { > odp_parse_error(&rl, errorp, "error parsing VXLAN options"); > return ODP_FIT_ERROR; > } > > - if (ext[OVS_VXLAN_EXT_GBP]) { > - uint32_t gbp = nl_attr_get_u32(ext[OVS_VXLAN_EXT_GBP]); > - > - odp_decode_gbp_raw(gbp, &tun->gbp_id, &tun->gbp_flags); > - } > - > break; > } > case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS: > @@ -8843,3 +8834,29 @@ commit_odp_actions(const struct flow *flow, struct flow *base, > > return slow1 ? slow1 : slow2; > } > + > +int > +odp_vxlan_tun_opts_from_attr(const struct nlattr *tun_attr, ovs_be16 *id, > + uint8_t *flags, bool *id_present) > +{ > + static const struct nl_policy vxlan_opts_policy[] = { > + [OVS_VXLAN_EXT_GBP] = { .type = NL_A_U32 }, > + }; > + struct nlattr *ext[ARRAY_SIZE(vxlan_opts_policy)]; > + > + if (!nl_parse_nested(tun_attr, vxlan_opts_policy, ext, ARRAY_SIZE(ext))) { > + return EINVAL; > + } > + > + if (ext[OVS_VXLAN_EXT_GBP]) { > + uint32_t gbp_raw = nl_attr_get_u32(ext[OVS_VXLAN_EXT_GBP]); > + > + odp_decode_gbp_raw(gbp_raw, id, flags); > + } > + > + if (id_present) { > + *id_present = !!ext[OVS_VXLAN_EXT_GBP]; > + } > + > + return 0; > +} > diff --git a/lib/odp-util.h b/lib/odp-util.h > index 163efe7a87b5..03f15ef5052b 100644 > --- a/lib/odp-util.h > +++ b/lib/odp-util.h > @@ -291,7 +291,9 @@ enum slow_path_reason commit_odp_actions(const struct flow *, > bool pending_encap, > bool pending_decap, > struct ofpbuf *encap_data); > - You removed ‘form feed’ here, some people want this to stay in, see code guidelines… > +int odp_vxlan_tun_opts_from_attr(const struct nlattr *tun_attr, ovs_be16 *id, > + uint8_t *flags, bool *id_present); > + > /* ofproto-dpif interface. > * > * The following types and functions are logically part of ofproto-dpif. > diff --git a/lib/tc.c b/lib/tc.c > index 94df4c507f14..7a13aaabbe59 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -1287,6 +1287,35 @@ nl_parse_act_geneve_opts(const struct nlattr *in_nlattr, > return 0; > } > > +static int > +nl_parse_act_vxlan_opts(struct nlattr *in_nlattr, struct tc_action *action) > +{ > + const struct ofpbuf *msg; > + struct nlattr *nla; > + struct ofpbuf buf; > + size_t left; > + > + nl_attr_get_nested(in_nlattr, &buf); > + msg = &buf; > + > + NL_ATTR_FOR_EACH (nla, left, ofpbuf_at(msg, 0, 0), msg->size) { > + uint16_t type = nl_attr_type(nla); > + int32_t gbp_raw; > + > + switch (type) { > + case TCA_FLOWER_KEY_ENC_OPT_VXLAN_GBP: > + gbp_raw = nl_attr_get_u32(nla); > + odp_decode_gbp_raw(gbp_raw, &action->encap.gbp.id, > + &action->encap.gbp.flags); > + action->encap.gbp.id_present = true; > + > + break; > + } > + } > + > + return 0; > +} > + > static int > nl_parse_act_tunnel_opts(struct nlattr *options, struct tc_action *action) > { > @@ -1312,6 +1341,13 @@ nl_parse_act_tunnel_opts(struct nlattr *options, struct tc_action *action) > return err; > } > > + break; > + case TCA_TUNNEL_KEY_ENC_OPTS_VXLAN: > + err = nl_parse_act_vxlan_opts(nla, action); > + if (err) { > + return err; > + } > + > break; > } > } > @@ -2627,6 +2663,27 @@ nl_msg_put_act_tunnel_geneve_option(struct ofpbuf *request, > nl_msg_end_nested(request, outer); > } > > +static void > +nl_msg_put_act_tunnel_vxlan_opts(struct ofpbuf *request, > + struct tc_action_encap *encap) > +{ > + size_t outer, inner; > + uint32_t gbp_raw; > + > + if (!encap->gbp.id_present) { > + return; > + } > + > + gbp_raw = odp_encode_gbp_raw(encap->gbp.flags, > + encap->gbp.id); > + outer = nl_msg_start_nested_with_flag(request, TCA_TUNNEL_KEY_ENC_OPTS); > + inner = nl_msg_start_nested_with_flag(request, > + TCA_TUNNEL_KEY_ENC_OPTS_VXLAN); > + nl_msg_put_u32(request, TCA_TUNNEL_KEY_ENC_OPT_VXLAN_GBP, gbp_raw); > + nl_msg_end_nested(request, inner); > + nl_msg_end_nested(request, outer); > +} > + > static void > nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, > struct tc_action_encap *encap, > @@ -2667,6 +2724,7 @@ nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, > nl_msg_put_be16(request, TCA_TUNNEL_KEY_ENC_DST_PORT, > encap->tp_dst); > } > + nl_msg_put_act_tunnel_vxlan_opts(request, encap); > nl_msg_put_act_tunnel_geneve_option(request, &encap->data); > nl_msg_put_u8(request, TCA_TUNNEL_KEY_NO_CSUM, encap->no_csum); > } > diff --git a/lib/tc.h b/lib/tc.h > index 1d648282a004..b493cb52c4ca 100644 > --- a/lib/tc.h > +++ b/lib/tc.h > @@ -218,6 +218,7 @@ struct tc_action_encap { > uint8_t tos; > uint8_t ttl; > uint8_t no_csum; > + struct tc_tunnel_gbp gbp; Any reason why you put it here? If not, I would suggest putting it at the bottom as it might be used less frequent. > struct { > ovs_be32 ipv4_src; > ovs_be32 ipv4_dst; > -- > 2.38.0
On 5/26/2023 8:18 PM, Eelco Chaudron wrote: > External email: Use caution opening links or attachments > > > On 15 May 2023, at 10:23, Roi Dayan wrote: > >> From: Gavin Li <gavinl@nvidia.com> >> >> Add TC offload support for vxlan encap with gbp option >> >> Signed-off-by: Gavin Li <gavinl@nvidia.com> >> Reviewed-by: Gavi Teitz <gavi@nvidia.com> >> Reviewed-by: Roi Dayan <roid@nvidia.com> > I have a couple of comments below, and some general ones. > > 1) Can we add some unit tests for this to check-offload? > > 2) Can you explain what happens if the kernel does not support the newly added option? I guess it will return -EINVAL when trying to install a flow, which will probably result in an error message. This is because EOPNOTSUPP is the only one ignored as not a problem. So maybe we need some probing? > > This does conclude my review of v3. > > //Eelco ACK Will add unit tests for this to check-offload and add probing. > >> --- >> acinclude.m4 | 7 ++++ >> include/linux/tc_act/tc_tunnel_key.h | 17 +++++++- >> lib/netdev-offload-tc.c | 31 ++++++++++++++- >> lib/odp-util.c | 41 ++++++++++++++------ >> lib/odp-util.h | 4 +- >> lib/tc.c | 58 ++++++++++++++++++++++++++++ >> lib/tc.h | 1 + >> 7 files changed, 143 insertions(+), 16 deletions(-) >> >> diff --git a/acinclude.m4 b/acinclude.m4 >> index ac1eab790041..690a13c25967 100644 >> --- a/acinclude.m4 >> +++ b/acinclude.m4 >> @@ -191,6 +191,13 @@ AC_DEFUN([OVS_CHECK_LINUX_TC], [ >> [AC_DEFINE([HAVE_TCA_TUNNEL_KEY_ENC_TTL], [1], >> [Define to 1 if TCA_TUNNEL_KEY_ENC_TTL is available.])]) >> >> + AC_COMPILE_IFELSE([ >> + AC_LANG_PROGRAM([#include <linux/tc_act/tc_tunnel_key.h>], [ >> + int x = TCA_TUNNEL_KEY_ENC_OPTS_VXLAN; >> + ])], >> + [AC_DEFINE([HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN], [1], >> + [Define to 1 if TCA_TUNNEL_KEY_ENC_OPTS_VXLAN is available.])]) >> + >> AC_COMPILE_IFELSE([ >> AC_LANG_PROGRAM([#include <linux/tc_act/tc_pedit.h>], [ >> int x = TCA_PEDIT_KEY_EX_HDR_TYPE_UDP; >> diff --git a/include/linux/tc_act/tc_tunnel_key.h b/include/linux/tc_act/tc_tunnel_key.h >> index f13acf17dd75..17291b90bf3c 100644 >> --- a/include/linux/tc_act/tc_tunnel_key.h >> +++ b/include/linux/tc_act/tc_tunnel_key.h >> @@ -1,7 +1,7 @@ >> #ifndef __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H >> #define __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H 1 >> >> -#if defined(__KERNEL__) || defined(HAVE_TCA_TUNNEL_KEY_ENC_TTL) >> +#if defined(__KERNEL__) || defined(HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN) >> #include_next <linux/tc_act/tc_tunnel_key.h> >> #else >> >> @@ -53,6 +53,10 @@ enum { >> * TCA_TUNNEL_KEY_ENC_OPTS_GENEVE >> * attributes >> */ >> + TCA_TUNNEL_KEY_ENC_OPTS_VXLAN, /* Nested >> + * TCA_TUNNEL_KEY_ENC_OPTS_VXLAN >> + * attributes >> + */ >> __TCA_TUNNEL_KEY_ENC_OPTS_MAX, >> }; >> >> @@ -70,6 +74,15 @@ enum { >> #define TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX \ >> (__TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX - 1) >> >> -#endif /* __KERNEL__ || HAVE_TCA_TUNNEL_KEY_ENC_TTL */ >> +enum { >> + TCA_TUNNEL_KEY_ENC_OPT_VXLAN_UNSPEC, >> + TCA_TUNNEL_KEY_ENC_OPT_VXLAN_GBP, /* u32 */ >> + __TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX, >> +}; >> + >> +#define TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX \ >> + (__TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX - 1) >> + >> +#endif /* __KERNEL__ || HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN */ >> >> #endif /* __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H */ >> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >> index 1c97681bc92b..a32ed8021c4b 100644 >> --- a/lib/netdev-offload-tc.c >> +++ b/lib/netdev-offload-tc.c >> @@ -668,6 +668,24 @@ static void parse_tc_flower_geneve_opts(struct tc_action *action, >> nl_msg_end_nested(buf, geneve_off); >> } >> >> +static void >> +parse_tc_flower_vxlan_tun_opts(struct tc_action *action, >> + struct ofpbuf *buf) >> +{ >> + size_t gbp_off; >> + uint32_t gbp_raw; >> + >> + if (!action->encap.gbp.id_present) { >> + return; >> + } >> + >> + gbp_off = nl_msg_start_nested(buf, OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS); >> + gbp_raw = odp_encode_gbp_raw(action->encap.gbp.flags, >> + action->encap.gbp.id); >> + nl_msg_put_u32(buf, OVS_VXLAN_EXT_GBP, gbp_raw); >> + nl_msg_end_nested(buf, gbp_off); >> +} >> + >> static void >> flower_tun_opt_to_match(struct match *match, struct tc_flower *flower) >> { >> @@ -863,7 +881,7 @@ parse_tc_flower_to_actions__(struct tc_flower *flower, struct ofpbuf *buf, >> if (!action->encap.no_csum) { >> nl_msg_put_flag(buf, OVS_TUNNEL_KEY_ATTR_CSUM); >> } >> - >> + parse_tc_flower_vxlan_tun_opts(action, buf); >> parse_tc_flower_geneve_opts(action, buf); >> nl_msg_end_nested(buf, tunnel_offset); >> nl_msg_end_nested(buf, set_offset); >> @@ -1552,6 +1570,7 @@ parse_put_flow_set_action(struct tc_flower *flower, struct tc_action *action, >> >> action->type = TC_ACT_ENCAP; >> action->encap.id_present = false; >> + action->encap.gbp.id_present = false; >> action->encap.no_csum = 1; >> flower->action_count++; >> NL_ATTR_FOR_EACH_UNSAFE(tun_attr, tun_left, tunnel, tunnel_len) { >> @@ -1613,6 +1632,16 @@ 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; >> + case OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS: { >> + if (odp_vxlan_tun_opts_from_attr(tun_attr, >> + &action->encap.gbp.id, >> + &action->encap.gbp.flags, >> + &action->encap.gbp.id_present)) { >> + VLOG_ERR_RL(&rl, "error parsing VXLAN options"); >> + return EINVAL; >> + } >> + } >> + break; >> default: >> VLOG_DBG_RL(&rl, "unsupported tunnel key attribute %d", >> nl_attr_type(tun_attr)); >> diff --git a/lib/odp-util.c b/lib/odp-util.c >> index bf34c61fec58..34c9001f85fc 100644 >> --- a/lib/odp-util.c >> +++ b/lib/odp-util.c >> @@ -3149,22 +3149,13 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, >> tun->flags |= FLOW_TNL_F_OAM; >> break; >> case OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS: { >> - static const struct nl_policy vxlan_opts_policy[] = { >> - [OVS_VXLAN_EXT_GBP] = { .type = NL_A_U32 }, >> - }; >> - struct nlattr *ext[ARRAY_SIZE(vxlan_opts_policy)]; >> - >> - if (!nl_parse_nested(a, vxlan_opts_policy, ext, ARRAY_SIZE(ext))) { >> + if (odp_vxlan_tun_opts_from_attr(a, &tun->gbp_id, >> + &tun->gbp_flags, >> + NULL)) { >> odp_parse_error(&rl, errorp, "error parsing VXLAN options"); >> return ODP_FIT_ERROR; >> } >> >> - if (ext[OVS_VXLAN_EXT_GBP]) { >> - uint32_t gbp = nl_attr_get_u32(ext[OVS_VXLAN_EXT_GBP]); >> - >> - odp_decode_gbp_raw(gbp, &tun->gbp_id, &tun->gbp_flags); >> - } >> - >> break; >> } >> case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS: >> @@ -8843,3 +8834,29 @@ commit_odp_actions(const struct flow *flow, struct flow *base, >> >> return slow1 ? slow1 : slow2; >> } >> + >> +int >> +odp_vxlan_tun_opts_from_attr(const struct nlattr *tun_attr, ovs_be16 *id, >> + uint8_t *flags, bool *id_present) >> +{ >> + static const struct nl_policy vxlan_opts_policy[] = { >> + [OVS_VXLAN_EXT_GBP] = { .type = NL_A_U32 }, >> + }; >> + struct nlattr *ext[ARRAY_SIZE(vxlan_opts_policy)]; >> + >> + if (!nl_parse_nested(tun_attr, vxlan_opts_policy, ext, ARRAY_SIZE(ext))) { >> + return EINVAL; >> + } >> + >> + if (ext[OVS_VXLAN_EXT_GBP]) { >> + uint32_t gbp_raw = nl_attr_get_u32(ext[OVS_VXLAN_EXT_GBP]); >> + >> + odp_decode_gbp_raw(gbp_raw, id, flags); >> + } >> + >> + if (id_present) { >> + *id_present = !!ext[OVS_VXLAN_EXT_GBP]; >> + } >> + >> + return 0; >> +} >> diff --git a/lib/odp-util.h b/lib/odp-util.h >> index 163efe7a87b5..03f15ef5052b 100644 >> --- a/lib/odp-util.h >> +++ b/lib/odp-util.h >> @@ -291,7 +291,9 @@ enum slow_path_reason commit_odp_actions(const struct flow *, >> bool pending_encap, >> bool pending_decap, >> struct ofpbuf *encap_data); >> - > You removed ‘form feed’ here, some people want this to stay in, see code guidelines… ACK > >> +int odp_vxlan_tun_opts_from_attr(const struct nlattr *tun_attr, ovs_be16 *id, >> + uint8_t *flags, bool *id_present); >> + >> /* ofproto-dpif interface. >> * >> * The following types and functions are logically part of ofproto-dpif. >> diff --git a/lib/tc.c b/lib/tc.c >> index 94df4c507f14..7a13aaabbe59 100644 >> --- a/lib/tc.c >> +++ b/lib/tc.c >> @@ -1287,6 +1287,35 @@ nl_parse_act_geneve_opts(const struct nlattr *in_nlattr, >> return 0; >> } >> >> +static int >> +nl_parse_act_vxlan_opts(struct nlattr *in_nlattr, struct tc_action *action) >> +{ >> + const struct ofpbuf *msg; >> + struct nlattr *nla; >> + struct ofpbuf buf; >> + size_t left; >> + >> + nl_attr_get_nested(in_nlattr, &buf); >> + msg = &buf; >> + >> + NL_ATTR_FOR_EACH (nla, left, ofpbuf_at(msg, 0, 0), msg->size) { >> + uint16_t type = nl_attr_type(nla); >> + int32_t gbp_raw; >> + >> + switch (type) { >> + case TCA_FLOWER_KEY_ENC_OPT_VXLAN_GBP: >> + gbp_raw = nl_attr_get_u32(nla); >> + odp_decode_gbp_raw(gbp_raw, &action->encap.gbp.id, >> + &action->encap.gbp.flags); >> + action->encap.gbp.id_present = true; >> + >> + break; >> + } >> + } >> + >> + return 0; >> +} >> + >> static int >> nl_parse_act_tunnel_opts(struct nlattr *options, struct tc_action *action) >> { >> @@ -1312,6 +1341,13 @@ nl_parse_act_tunnel_opts(struct nlattr *options, struct tc_action *action) >> return err; >> } >> >> + break; >> + case TCA_TUNNEL_KEY_ENC_OPTS_VXLAN: >> + err = nl_parse_act_vxlan_opts(nla, action); >> + if (err) { >> + return err; >> + } >> + >> break; >> } >> } >> @@ -2627,6 +2663,27 @@ nl_msg_put_act_tunnel_geneve_option(struct ofpbuf *request, >> nl_msg_end_nested(request, outer); >> } >> >> +static void >> +nl_msg_put_act_tunnel_vxlan_opts(struct ofpbuf *request, >> + struct tc_action_encap *encap) >> +{ >> + size_t outer, inner; >> + uint32_t gbp_raw; >> + >> + if (!encap->gbp.id_present) { >> + return; >> + } >> + >> + gbp_raw = odp_encode_gbp_raw(encap->gbp.flags, >> + encap->gbp.id); >> + outer = nl_msg_start_nested_with_flag(request, TCA_TUNNEL_KEY_ENC_OPTS); >> + inner = nl_msg_start_nested_with_flag(request, >> + TCA_TUNNEL_KEY_ENC_OPTS_VXLAN); >> + nl_msg_put_u32(request, TCA_TUNNEL_KEY_ENC_OPT_VXLAN_GBP, gbp_raw); >> + nl_msg_end_nested(request, inner); >> + nl_msg_end_nested(request, outer); >> +} >> + >> static void >> nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, >> struct tc_action_encap *encap, >> @@ -2667,6 +2724,7 @@ nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, >> nl_msg_put_be16(request, TCA_TUNNEL_KEY_ENC_DST_PORT, >> encap->tp_dst); >> } >> + nl_msg_put_act_tunnel_vxlan_opts(request, encap); >> nl_msg_put_act_tunnel_geneve_option(request, &encap->data); >> nl_msg_put_u8(request, TCA_TUNNEL_KEY_NO_CSUM, encap->no_csum); >> } >> diff --git a/lib/tc.h b/lib/tc.h >> index 1d648282a004..b493cb52c4ca 100644 >> --- a/lib/tc.h >> +++ b/lib/tc.h >> @@ -218,6 +218,7 @@ struct tc_action_encap { >> uint8_t tos; >> uint8_t ttl; >> uint8_t no_csum; >> + struct tc_tunnel_gbp gbp; > Any reason why you put it here? If not, I would suggest putting it at the bottom as it might be used less frequent. ACK > > >> struct { >> ovs_be32 ipv4_src; >> ovs_be32 ipv4_dst; >> -- >> 2.38.0
diff --git a/acinclude.m4 b/acinclude.m4 index ac1eab790041..690a13c25967 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -191,6 +191,13 @@ AC_DEFUN([OVS_CHECK_LINUX_TC], [ [AC_DEFINE([HAVE_TCA_TUNNEL_KEY_ENC_TTL], [1], [Define to 1 if TCA_TUNNEL_KEY_ENC_TTL is available.])]) + AC_COMPILE_IFELSE([ + AC_LANG_PROGRAM([#include <linux/tc_act/tc_tunnel_key.h>], [ + int x = TCA_TUNNEL_KEY_ENC_OPTS_VXLAN; + ])], + [AC_DEFINE([HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN], [1], + [Define to 1 if TCA_TUNNEL_KEY_ENC_OPTS_VXLAN is available.])]) + AC_COMPILE_IFELSE([ AC_LANG_PROGRAM([#include <linux/tc_act/tc_pedit.h>], [ int x = TCA_PEDIT_KEY_EX_HDR_TYPE_UDP; diff --git a/include/linux/tc_act/tc_tunnel_key.h b/include/linux/tc_act/tc_tunnel_key.h index f13acf17dd75..17291b90bf3c 100644 --- a/include/linux/tc_act/tc_tunnel_key.h +++ b/include/linux/tc_act/tc_tunnel_key.h @@ -1,7 +1,7 @@ #ifndef __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H #define __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H 1 -#if defined(__KERNEL__) || defined(HAVE_TCA_TUNNEL_KEY_ENC_TTL) +#if defined(__KERNEL__) || defined(HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN) #include_next <linux/tc_act/tc_tunnel_key.h> #else @@ -53,6 +53,10 @@ enum { * TCA_TUNNEL_KEY_ENC_OPTS_GENEVE * attributes */ + TCA_TUNNEL_KEY_ENC_OPTS_VXLAN, /* Nested + * TCA_TUNNEL_KEY_ENC_OPTS_VXLAN + * attributes + */ __TCA_TUNNEL_KEY_ENC_OPTS_MAX, }; @@ -70,6 +74,15 @@ enum { #define TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX \ (__TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX - 1) -#endif /* __KERNEL__ || HAVE_TCA_TUNNEL_KEY_ENC_TTL */ +enum { + TCA_TUNNEL_KEY_ENC_OPT_VXLAN_UNSPEC, + TCA_TUNNEL_KEY_ENC_OPT_VXLAN_GBP, /* u32 */ + __TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX, +}; + +#define TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX \ + (__TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX - 1) + +#endif /* __KERNEL__ || HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN */ #endif /* __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H */ diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index 1c97681bc92b..a32ed8021c4b 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -668,6 +668,24 @@ static void parse_tc_flower_geneve_opts(struct tc_action *action, nl_msg_end_nested(buf, geneve_off); } +static void +parse_tc_flower_vxlan_tun_opts(struct tc_action *action, + struct ofpbuf *buf) +{ + size_t gbp_off; + uint32_t gbp_raw; + + if (!action->encap.gbp.id_present) { + return; + } + + gbp_off = nl_msg_start_nested(buf, OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS); + gbp_raw = odp_encode_gbp_raw(action->encap.gbp.flags, + action->encap.gbp.id); + nl_msg_put_u32(buf, OVS_VXLAN_EXT_GBP, gbp_raw); + nl_msg_end_nested(buf, gbp_off); +} + static void flower_tun_opt_to_match(struct match *match, struct tc_flower *flower) { @@ -863,7 +881,7 @@ parse_tc_flower_to_actions__(struct tc_flower *flower, struct ofpbuf *buf, if (!action->encap.no_csum) { nl_msg_put_flag(buf, OVS_TUNNEL_KEY_ATTR_CSUM); } - + parse_tc_flower_vxlan_tun_opts(action, buf); parse_tc_flower_geneve_opts(action, buf); nl_msg_end_nested(buf, tunnel_offset); nl_msg_end_nested(buf, set_offset); @@ -1552,6 +1570,7 @@ parse_put_flow_set_action(struct tc_flower *flower, struct tc_action *action, action->type = TC_ACT_ENCAP; action->encap.id_present = false; + action->encap.gbp.id_present = false; action->encap.no_csum = 1; flower->action_count++; NL_ATTR_FOR_EACH_UNSAFE(tun_attr, tun_left, tunnel, tunnel_len) { @@ -1613,6 +1632,16 @@ 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; + case OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS: { + if (odp_vxlan_tun_opts_from_attr(tun_attr, + &action->encap.gbp.id, + &action->encap.gbp.flags, + &action->encap.gbp.id_present)) { + VLOG_ERR_RL(&rl, "error parsing VXLAN options"); + return EINVAL; + } + } + break; default: VLOG_DBG_RL(&rl, "unsupported tunnel key attribute %d", nl_attr_type(tun_attr)); diff --git a/lib/odp-util.c b/lib/odp-util.c index bf34c61fec58..34c9001f85fc 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -3149,22 +3149,13 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, tun->flags |= FLOW_TNL_F_OAM; break; case OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS: { - static const struct nl_policy vxlan_opts_policy[] = { - [OVS_VXLAN_EXT_GBP] = { .type = NL_A_U32 }, - }; - struct nlattr *ext[ARRAY_SIZE(vxlan_opts_policy)]; - - if (!nl_parse_nested(a, vxlan_opts_policy, ext, ARRAY_SIZE(ext))) { + if (odp_vxlan_tun_opts_from_attr(a, &tun->gbp_id, + &tun->gbp_flags, + NULL)) { odp_parse_error(&rl, errorp, "error parsing VXLAN options"); return ODP_FIT_ERROR; } - if (ext[OVS_VXLAN_EXT_GBP]) { - uint32_t gbp = nl_attr_get_u32(ext[OVS_VXLAN_EXT_GBP]); - - odp_decode_gbp_raw(gbp, &tun->gbp_id, &tun->gbp_flags); - } - break; } case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS: @@ -8843,3 +8834,29 @@ commit_odp_actions(const struct flow *flow, struct flow *base, return slow1 ? slow1 : slow2; } + +int +odp_vxlan_tun_opts_from_attr(const struct nlattr *tun_attr, ovs_be16 *id, + uint8_t *flags, bool *id_present) +{ + static const struct nl_policy vxlan_opts_policy[] = { + [OVS_VXLAN_EXT_GBP] = { .type = NL_A_U32 }, + }; + struct nlattr *ext[ARRAY_SIZE(vxlan_opts_policy)]; + + if (!nl_parse_nested(tun_attr, vxlan_opts_policy, ext, ARRAY_SIZE(ext))) { + return EINVAL; + } + + if (ext[OVS_VXLAN_EXT_GBP]) { + uint32_t gbp_raw = nl_attr_get_u32(ext[OVS_VXLAN_EXT_GBP]); + + odp_decode_gbp_raw(gbp_raw, id, flags); + } + + if (id_present) { + *id_present = !!ext[OVS_VXLAN_EXT_GBP]; + } + + return 0; +} diff --git a/lib/odp-util.h b/lib/odp-util.h index 163efe7a87b5..03f15ef5052b 100644 --- a/lib/odp-util.h +++ b/lib/odp-util.h @@ -291,7 +291,9 @@ enum slow_path_reason commit_odp_actions(const struct flow *, bool pending_encap, bool pending_decap, struct ofpbuf *encap_data); - +int odp_vxlan_tun_opts_from_attr(const struct nlattr *tun_attr, ovs_be16 *id, + uint8_t *flags, bool *id_present); + /* ofproto-dpif interface. * * The following types and functions are logically part of ofproto-dpif. diff --git a/lib/tc.c b/lib/tc.c index 94df4c507f14..7a13aaabbe59 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -1287,6 +1287,35 @@ nl_parse_act_geneve_opts(const struct nlattr *in_nlattr, return 0; } +static int +nl_parse_act_vxlan_opts(struct nlattr *in_nlattr, struct tc_action *action) +{ + const struct ofpbuf *msg; + struct nlattr *nla; + struct ofpbuf buf; + size_t left; + + nl_attr_get_nested(in_nlattr, &buf); + msg = &buf; + + NL_ATTR_FOR_EACH (nla, left, ofpbuf_at(msg, 0, 0), msg->size) { + uint16_t type = nl_attr_type(nla); + int32_t gbp_raw; + + switch (type) { + case TCA_FLOWER_KEY_ENC_OPT_VXLAN_GBP: + gbp_raw = nl_attr_get_u32(nla); + odp_decode_gbp_raw(gbp_raw, &action->encap.gbp.id, + &action->encap.gbp.flags); + action->encap.gbp.id_present = true; + + break; + } + } + + return 0; +} + static int nl_parse_act_tunnel_opts(struct nlattr *options, struct tc_action *action) { @@ -1312,6 +1341,13 @@ nl_parse_act_tunnel_opts(struct nlattr *options, struct tc_action *action) return err; } + break; + case TCA_TUNNEL_KEY_ENC_OPTS_VXLAN: + err = nl_parse_act_vxlan_opts(nla, action); + if (err) { + return err; + } + break; } } @@ -2627,6 +2663,27 @@ nl_msg_put_act_tunnel_geneve_option(struct ofpbuf *request, nl_msg_end_nested(request, outer); } +static void +nl_msg_put_act_tunnel_vxlan_opts(struct ofpbuf *request, + struct tc_action_encap *encap) +{ + size_t outer, inner; + uint32_t gbp_raw; + + if (!encap->gbp.id_present) { + return; + } + + gbp_raw = odp_encode_gbp_raw(encap->gbp.flags, + encap->gbp.id); + outer = nl_msg_start_nested_with_flag(request, TCA_TUNNEL_KEY_ENC_OPTS); + inner = nl_msg_start_nested_with_flag(request, + TCA_TUNNEL_KEY_ENC_OPTS_VXLAN); + nl_msg_put_u32(request, TCA_TUNNEL_KEY_ENC_OPT_VXLAN_GBP, gbp_raw); + nl_msg_end_nested(request, inner); + nl_msg_end_nested(request, outer); +} + static void nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, struct tc_action_encap *encap, @@ -2667,6 +2724,7 @@ nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, nl_msg_put_be16(request, TCA_TUNNEL_KEY_ENC_DST_PORT, encap->tp_dst); } + nl_msg_put_act_tunnel_vxlan_opts(request, encap); nl_msg_put_act_tunnel_geneve_option(request, &encap->data); nl_msg_put_u8(request, TCA_TUNNEL_KEY_NO_CSUM, encap->no_csum); } diff --git a/lib/tc.h b/lib/tc.h index 1d648282a004..b493cb52c4ca 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -218,6 +218,7 @@ struct tc_action_encap { uint8_t tos; uint8_t ttl; uint8_t no_csum; + struct tc_tunnel_gbp gbp; struct { ovs_be32 ipv4_src; ovs_be32 ipv4_dst;