Add HW offloading support for TC flows with VxLAN GBP encap/decap.
Example of encap rule:
tc filter add dev eth0 protocol ip ingress flower \
action tunnel_key set id 42 vxlan_opts 512 \
action mirred egress redirect dev vxlan1
Example of decap rule:
tc filter add dev vxlan1 protocol ip ingress flower \
enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
action tunnel_key unset action mirred egress redirect dev eth0
Change-Id: I48f61d02201bf3f79dcbe5d0f022f7bb27ed630f
Signed-off-by: Gavin Li <[email protected]>
Reviewed-by: Roi Dayan <[email protected]>
Reviewed-by: Maor Dickman <[email protected]>
Acked-by: Saeed Mahameed <[email protected]>
---
.../mellanox/mlx5/core/en/tc_tun_vxlan.c | 85 ++++++++++++++++++-
include/linux/mlx5/device.h | 6 ++
include/linux/mlx5/mlx5_ifc.h | 13 ++-
3 files changed, 100 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
index 1f62c702b625..444512ca9e0d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
/* Copyright (c) 2018 Mellanox Technologies. */
+#include <net/ip_tunnels.h>
#include <net/vxlan.h>
#include "lib/vxlan.h"
#include "en/tc_tun.h"
@@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
const struct ip_tunnel_key *tun_key = &e->tun_info->key;
__be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
struct udphdr *udp = (struct udphdr *)(buf);
+ const struct vxlan_metadata *md;
struct vxlanhdr *vxh;
- if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
+ if (tun_key->tun_flags & TUNNEL_VXLAN_OPT &&
+ e->tun_info->options_len != sizeof(*md))
return -EOPNOTSUPP;
vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
*ip_proto = IPPROTO_UDP;
@@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
udp->dest = tun_key->tp_dst;
vxh->vx_flags = VXLAN_HF_VNI;
vxh->vx_vni = vxlan_vni_field(tun_id);
+ if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
+ md = ip_tunnel_info_opts((struct ip_tunnel_info *)e->tun_info);
+ vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
+ (struct vxlan_metadata *)md);
+ }
+
+ return 0;
+}
+
+static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv *priv,
+ struct mlx5_flow_spec *spec,
+ struct flow_cls_offload *f)
+{
+ struct flow_rule *rule = flow_cls_offload_flow_rule(f);
+ struct netlink_ext_ack *extack = f->common.extack;
+ struct flow_match_enc_opts enc_opts;
+ void *misc5_c, *misc5_v;
+ u32 *gbp, *gbp_mask;
+
+ flow_rule_match_enc_opts(rule, &enc_opts);
+
+ if (memchr_inv(&enc_opts.mask->data, 0, sizeof(enc_opts.mask->data)) &&
+ !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev, tunnel_header_0_1)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Matching on VxLAN GBP is not supported");
+ netdev_warn(priv->netdev,
+ "Matching on VxLAN GBP is not supported\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Wrong VxLAN option type: not GBP");
+ netdev_warn(priv->netdev,
+ "Wrong VxLAN option type: not GBP\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (enc_opts.key->len != sizeof(*gbp) ||
+ enc_opts.mask->len != sizeof(*gbp_mask)) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "VxLAN GBP option/mask len is not 32 bits");
+ netdev_warn(priv->netdev,
+ "VxLAN GBP option/mask len is not 32 bits\n");
+ return -EINVAL;
+ }
+
+ gbp = (u32 *)&enc_opts.key->data[0];
+ gbp_mask = (u32 *)&enc_opts.mask->data[0];
+
+ if (*gbp_mask & ~VXLAN_GBP_MASK) {
+ NL_SET_ERR_MSG_MOD(extack,
+ "Wrong VxLAN GBP mask");
+ netdev_warn(priv->netdev,
+ "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
+ return -EINVAL;
+ }
+
+ misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_5);
+ misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_5);
+ MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0, *gbp_mask);
+ MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
+
+ spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
return 0;
}
@@ -122,6 +189,14 @@ static int mlx5e_tc_tun_parse_vxlan(struct mlx5e_priv *priv,
if (!enc_keyid.mask->keyid)
return 0;
+ if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ENC_OPTS)) {
+ int err;
+
+ err = mlx5e_tc_tun_parse_vxlan_gbp_option(priv, spec, f);
+ if (err)
+ return err;
+ }
+
/* match on VNI is required */
if (!MLX5_CAP_ESW_FLOWTABLE_FDB(priv->mdev,
@@ -143,6 +218,12 @@ static int mlx5e_tc_tun_parse_vxlan(struct mlx5e_priv *priv,
return 0;
}
+static bool mlx5e_tc_tun_encap_info_equal_vxlan(struct mlx5e_encap_key *a,
+ struct mlx5e_encap_key *b)
+{
+ return mlx5e_tc_tun_encap_info_equal_options(a, b, TUNNEL_VXLAN_OPT);
+}
+
static int mlx5e_tc_tun_get_remote_ifindex(struct net_device *mirred_dev)
{
const struct vxlan_dev *vxlan = netdev_priv(mirred_dev);
@@ -160,6 +241,6 @@ struct mlx5e_tc_tunnel vxlan_tunnel = {
.generate_ip_tun_hdr = mlx5e_gen_ip_tunnel_header_vxlan,
.parse_udp_ports = mlx5e_tc_tun_parse_udp_ports_vxlan,
.parse_tunnel = mlx5e_tc_tun_parse_vxlan,
- .encap_info_equal = mlx5e_tc_tun_encap_info_equal_generic,
+ .encap_info_equal = mlx5e_tc_tun_encap_info_equal_vxlan,
.get_remote_ifindex = mlx5e_tc_tun_get_remote_ifindex,
};
diff --git a/include/linux/mlx5/device.h b/include/linux/mlx5/device.h
index 71b06ebad402..af4dd536a52c 100644
--- a/include/linux/mlx5/device.h
+++ b/include/linux/mlx5/device.h
@@ -1357,6 +1357,12 @@ enum mlx5_qcam_feature_groups {
#define MLX5_CAP_ESW_INGRESS_ACL_MAX(mdev, cap) \
MLX5_CAP_ESW_FLOWTABLE_MAX(mdev, flow_table_properties_esw_acl_ingress.cap)
+#define MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(mdev, cap) \
+ MLX5_CAP_ESW_FLOWTABLE(mdev, ft_field_support_2_esw_fdb.cap)
+
+#define MLX5_CAP_ESW_FT_FIELD_SUPPORT_2_MAX(mdev, cap) \
+ MLX5_CAP_ESW_FLOWTABLE_MAX(mdev, ft_field_support_2_esw_fdb.cap)
+
#define MLX5_CAP_ESW(mdev, cap) \
MLX5_GET(e_switch_cap, \
mdev->caps.hca[MLX5_CAP_ESWITCH]->cur, cap)
diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 1e530a8a2cf5..caef6aa20454 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -399,10 +399,13 @@ struct mlx5_ifc_flow_table_fields_supported_bits {
u8 metadata_reg_c_0[0x1];
};
+/* Table 2170 - Flow Table Fields Supported 2 Format */
struct mlx5_ifc_flow_table_fields_supported_2_bits {
u8 reserved_at_0[0xe];
u8 bth_opcode[0x1];
- u8 reserved_at_f[0x11];
+ u8 reserved_at_f[0x1];
+ u8 tunnel_header_0_1[0x1];
+ u8 reserved_at_11[0xf];
u8 reserved_at_20[0x60];
};
@@ -890,7 +893,13 @@ struct mlx5_ifc_flow_table_eswitch_cap_bits {
struct mlx5_ifc_flow_table_prop_layout_bits flow_table_properties_esw_acl_egress;
- u8 reserved_at_800[0x1000];
+ u8 reserved_at_800[0xC00];
+
+ struct mlx5_ifc_flow_table_fields_supported_2_bits ft_field_support_2_esw_fdb;
+
+ struct mlx5_ifc_flow_table_fields_supported_2_bits ft_field_bitmask_support_2_esw_fdb;
+
+ u8 reserved_at_1500[0x300];
u8 sw_steering_fdb_action_drop_icm_address_rx[0x40];
--
2.31.1
From: Gavin Li <[email protected]>
Date: Tue, 14 Feb 2023 15:41:37 +0200
> Add HW offloading support for TC flows with VxLAN GBP encap/decap.
>
> Example of encap rule:
> tc filter add dev eth0 protocol ip ingress flower \
> action tunnel_key set id 42 vxlan_opts 512 \
> action mirred egress redirect dev vxlan1
>
> Example of decap rule:
> tc filter add dev vxlan1 protocol ip ingress flower \
> enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
> action tunnel_key unset action mirred egress redirect dev eth0
>
> Change-Id: I48f61d02201bf3f79dcbe5d0f022f7bb27ed630f
> Signed-off-by: Gavin Li <[email protected]>
> Reviewed-by: Roi Dayan <[email protected]>
> Reviewed-by: Maor Dickman <[email protected]>
> Acked-by: Saeed Mahameed <[email protected]>
> ---
> .../mellanox/mlx5/core/en/tc_tun_vxlan.c | 85 ++++++++++++++++++-
> include/linux/mlx5/device.h | 6 ++
> include/linux/mlx5/mlx5_ifc.h | 13 ++-
> 3 files changed, 100 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
> index 1f62c702b625..444512ca9e0d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> /* Copyright (c) 2018 Mellanox Technologies. */
>
> +#include <net/ip_tunnels.h>
> #include <net/vxlan.h>
> #include "lib/vxlan.h"
> #include "en/tc_tun.h"
> @@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
> const struct ip_tunnel_key *tun_key = &e->tun_info->key;
> __be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
> struct udphdr *udp = (struct udphdr *)(buf);
> + const struct vxlan_metadata *md;
> struct vxlanhdr *vxh;
>
> - if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT &&
A separate pair of braces is preferred around bitops.
> + e->tun_info->options_len != sizeof(*md))
> return -EOPNOTSUPP;
> vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
> *ip_proto = IPPROTO_UDP;
> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
> udp->dest = tun_key->tp_dst;
> vxh->vx_flags = VXLAN_HF_VNI;
> vxh->vx_vni = vxlan_vni_field(tun_id);
> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
> + md = ip_tunnel_info_opts((struct ip_tunnel_info *)e->tun_info);
> + vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
> + (struct vxlan_metadata *)md);
Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
arguments instead of working around by casting away?
> + }
> +
> + return 0;
> +}
> +
> +static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv *priv,
> + struct mlx5_flow_spec *spec,
> + struct flow_cls_offload *f)
> +{
> + struct flow_rule *rule = flow_cls_offload_flow_rule(f);
> + struct netlink_ext_ack *extack = f->common.extack;
> + struct flow_match_enc_opts enc_opts;
> + void *misc5_c, *misc5_v;
> + u32 *gbp, *gbp_mask;
> +
> + flow_rule_match_enc_opts(rule, &enc_opts);
> +
> + if (memchr_inv(&enc_opts.mask->data, 0, sizeof(enc_opts.mask->data)) &&
> + !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev, tunnel_header_0_1)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Matching on VxLAN GBP is not supported");
> + netdev_warn(priv->netdev,
> + "Matching on VxLAN GBP is not supported\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Wrong VxLAN option type: not GBP");
Fits into one line I believe.
> + netdev_warn(priv->netdev,
> + "Wrong VxLAN option type: not GBP\n");
> + return -EOPNOTSUPP;
> + }
> +
> + if (enc_opts.key->len != sizeof(*gbp) ||
> + enc_opts.mask->len != sizeof(*gbp_mask)) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "VxLAN GBP option/mask len is not 32 bits");
> + netdev_warn(priv->netdev,
> + "VxLAN GBP option/mask len is not 32 bits\n");
> + return -EINVAL;
> + }
> +
> + gbp = (u32 *)&enc_opts.key->data[0];
> + gbp_mask = (u32 *)&enc_opts.mask->data[0];
> +
> + if (*gbp_mask & ~VXLAN_GBP_MASK) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "Wrong VxLAN GBP mask");
You can use new NL_SET_ERR_MSG_FMT_MOD() here to print @gbp_mask to the
user, as you do it next line.
> + netdev_warn(priv->netdev,
> + "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
> + return -EINVAL;
> + }
> +
> + misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_5);
> + misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_5);
> + MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0, *gbp_mask);
> + MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
> +
> + spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
>
> return 0;
> }
Thanks,
Olek
On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <[email protected]>
> Date: Tue, 14 Feb 2023 15:41:37 +0200
>
>> Add HW offloading support for TC flows with VxLAN GBP encap/decap.
>>
>> Example of encap rule:
>> tc filter add dev eth0 protocol ip ingress flower \
>> action tunnel_key set id 42 vxlan_opts 512 \
>> action mirred egress redirect dev vxlan1
>>
>> Example of decap rule:
>> tc filter add dev vxlan1 protocol ip ingress flower \
>> enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
>> action tunnel_key unset action mirred egress redirect dev eth0
>>
>> Change-Id: I48f61d02201bf3f79dcbe5d0f022f7bb27ed630f
>> Signed-off-by: Gavin Li <[email protected]>
>> Reviewed-by: Roi Dayan <[email protected]>
>> Reviewed-by: Maor Dickman <[email protected]>
>> Acked-by: Saeed Mahameed <[email protected]>
>> ---
>> .../mellanox/mlx5/core/en/tc_tun_vxlan.c | 85 ++++++++++++++++++-
>> include/linux/mlx5/device.h | 6 ++
>> include/linux/mlx5/mlx5_ifc.h | 13 ++-
>> 3 files changed, 100 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> index 1f62c702b625..444512ca9e0d 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> @@ -1,6 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>> /* Copyright (c) 2018 Mellanox Technologies. */
>>
>> +#include <net/ip_tunnels.h>
>> #include <net/vxlan.h>
>> #include "lib/vxlan.h"
>> #include "en/tc_tun.h"
>> @@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
>> const struct ip_tunnel_key *tun_key = &e->tun_info->key;
>> __be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
>> struct udphdr *udp = (struct udphdr *)(buf);
>> + const struct vxlan_metadata *md;
>> struct vxlanhdr *vxh;
>>
>> - if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT &&
> A separate pair of braces is preferred around bitops.
ACK
>
>> + e->tun_info->options_len != sizeof(*md))
>> return -EOPNOTSUPP;
>> vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
>> *ip_proto = IPPROTO_UDP;
>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
>> udp->dest = tun_key->tp_dst;
>> vxh->vx_flags = VXLAN_HF_VNI;
>> vxh->vx_vni = vxlan_vni_field(tun_id);
>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>> + md = ip_tunnel_info_opts((struct ip_tunnel_info *)e->tun_info);
>> + vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>> + (struct vxlan_metadata *)md);
> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
> arguments instead of working around by casting away?
The reason to cast it is a WA to use ip_tunnel_info_opts which takes
non-const arg while
e->tun_info here is a const one.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv *priv,
>> + struct mlx5_flow_spec *spec,
>> + struct flow_cls_offload *f)
>> +{
>> + struct flow_rule *rule = flow_cls_offload_flow_rule(f);
>> + struct netlink_ext_ack *extack = f->common.extack;
>> + struct flow_match_enc_opts enc_opts;
>> + void *misc5_c, *misc5_v;
>> + u32 *gbp, *gbp_mask;
>> +
>> + flow_rule_match_enc_opts(rule, &enc_opts);
>> +
>> + if (memchr_inv(&enc_opts.mask->data, 0, sizeof(enc_opts.mask->data)) &&
>> + !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev, tunnel_header_0_1)) {
>> + NL_SET_ERR_MSG_MOD(extack,
>> + "Matching on VxLAN GBP is not supported");
>> + netdev_warn(priv->netdev,
>> + "Matching on VxLAN GBP is not supported\n");
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
>> + NL_SET_ERR_MSG_MOD(extack,
>> + "Wrong VxLAN option type: not GBP");
> Fits into one line I believe.
ACK
>
>> + netdev_warn(priv->netdev,
>> + "Wrong VxLAN option type: not GBP\n");
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + if (enc_opts.key->len != sizeof(*gbp) ||
>> + enc_opts.mask->len != sizeof(*gbp_mask)) {
>> + NL_SET_ERR_MSG_MOD(extack,
>> + "VxLAN GBP option/mask len is not 32 bits");
>> + netdev_warn(priv->netdev,
>> + "VxLAN GBP option/mask len is not 32 bits\n");
>> + return -EINVAL;
>> + }
>> +
>> + gbp = (u32 *)&enc_opts.key->data[0];
>> + gbp_mask = (u32 *)&enc_opts.mask->data[0];
>> +
>> + if (*gbp_mask & ~VXLAN_GBP_MASK) {
>> + NL_SET_ERR_MSG_MOD(extack,
>> + "Wrong VxLAN GBP mask");
> You can use new NL_SET_ERR_MSG_FMT_MOD() here to print @gbp_mask to the
> user, as you do it next line.
ACK
>
>> + netdev_warn(priv->netdev,
>> + "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
>> + return -EINVAL;
>> + }
>> +
>> + misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_5);
>> + misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_5);
>> + MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0, *gbp_mask);
>> + MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
>> +
>> + spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
>>
>> return 0;
>> }
> Thanks,
> Olek
On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <[email protected]>
> Date: Tue, 14 Feb 2023 15:41:37 +0200
>
>> Add HW offloading support for TC flows with VxLAN GBP encap/decap.
>>
>> Example of encap rule:
>> tc filter add dev eth0 protocol ip ingress flower \
>> action tunnel_key set id 42 vxlan_opts 512 \
>> action mirred egress redirect dev vxlan1
>>
>> Example of decap rule:
>> tc filter add dev vxlan1 protocol ip ingress flower \
>> enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
>> action tunnel_key unset action mirred egress redirect dev eth0
>>
>> Change-Id: I48f61d02201bf3f79dcbe5d0f022f7bb27ed630f
>> Signed-off-by: Gavin Li <[email protected]>
>> Reviewed-by: Roi Dayan <[email protected]>
>> Reviewed-by: Maor Dickman <[email protected]>
>> Acked-by: Saeed Mahameed <[email protected]>
>> ---
>> .../mellanox/mlx5/core/en/tc_tun_vxlan.c | 85 ++++++++++++++++++-
>> include/linux/mlx5/device.h | 6 ++
>> include/linux/mlx5/mlx5_ifc.h | 13 ++-
>> 3 files changed, 100 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> index 1f62c702b625..444512ca9e0d 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>> @@ -1,6 +1,7 @@
>> // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>> /* Copyright (c) 2018 Mellanox Technologies. */
>>
>> +#include <net/ip_tunnels.h>
>> #include <net/vxlan.h>
>> #include "lib/vxlan.h"
>> #include "en/tc_tun.h"
>> @@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
>> const struct ip_tunnel_key *tun_key = &e->tun_info->key;
>> __be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
>> struct udphdr *udp = (struct udphdr *)(buf);
>> + const struct vxlan_metadata *md;
>> struct vxlanhdr *vxh;
>>
>> - if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT &&
> A separate pair of braces is preferred around bitops.
>
>> + e->tun_info->options_len != sizeof(*md))
>> return -EOPNOTSUPP;
>> vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
>> *ip_proto = IPPROTO_UDP;
>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char buf[],
>> udp->dest = tun_key->tp_dst;
>> vxh->vx_flags = VXLAN_HF_VNI;
>> vxh->vx_vni = vxlan_vni_field(tun_id);
>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>> + md = ip_tunnel_info_opts((struct ip_tunnel_info *)e->tun_info);
>> + vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>> + (struct vxlan_metadata *)md);
> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
> arguments instead of working around by casting away?
ACK. Sorry for the confusion---I misunderstood the comment.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv *priv,
>> + struct mlx5_flow_spec *spec,
>> + struct flow_cls_offload *f)
>> +{
>> + struct flow_rule *rule = flow_cls_offload_flow_rule(f);
>> + struct netlink_ext_ack *extack = f->common.extack;
>> + struct flow_match_enc_opts enc_opts;
>> + void *misc5_c, *misc5_v;
>> + u32 *gbp, *gbp_mask;
>> +
>> + flow_rule_match_enc_opts(rule, &enc_opts);
>> +
>> + if (memchr_inv(&enc_opts.mask->data, 0, sizeof(enc_opts.mask->data)) &&
>> + !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev, tunnel_header_0_1)) {
>> + NL_SET_ERR_MSG_MOD(extack,
>> + "Matching on VxLAN GBP is not supported");
>> + netdev_warn(priv->netdev,
>> + "Matching on VxLAN GBP is not supported\n");
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
>> + NL_SET_ERR_MSG_MOD(extack,
>> + "Wrong VxLAN option type: not GBP");
> Fits into one line I believe.
>
>> + netdev_warn(priv->netdev,
>> + "Wrong VxLAN option type: not GBP\n");
>> + return -EOPNOTSUPP;
>> + }
>> +
>> + if (enc_opts.key->len != sizeof(*gbp) ||
>> + enc_opts.mask->len != sizeof(*gbp_mask)) {
>> + NL_SET_ERR_MSG_MOD(extack,
>> + "VxLAN GBP option/mask len is not 32 bits");
>> + netdev_warn(priv->netdev,
>> + "VxLAN GBP option/mask len is not 32 bits\n");
>> + return -EINVAL;
>> + }
>> +
>> + gbp = (u32 *)&enc_opts.key->data[0];
>> + gbp_mask = (u32 *)&enc_opts.mask->data[0];
>> +
>> + if (*gbp_mask & ~VXLAN_GBP_MASK) {
>> + NL_SET_ERR_MSG_MOD(extack,
>> + "Wrong VxLAN GBP mask");
> You can use new NL_SET_ERR_MSG_FMT_MOD() here to print @gbp_mask to the
> user, as you do it next line.
>
>> + netdev_warn(priv->netdev,
>> + "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
>> + return -EINVAL;
>> + }
>> +
>> + misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, misc_parameters_5);
>> + misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, misc_parameters_5);
>> + MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0, *gbp_mask);
>> + MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
>> +
>> + spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
>>
>> return 0;
>> }
> Thanks,
> Olek
On 2/15/2023 11:36 AM, Gavin Li wrote:
> External email: Use caution opening links or attachments
>
>
> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <[email protected]>
>> Date: Tue, 14 Feb 2023 15:41:37 +0200
>>
>>> Add HW offloading support for TC flows with VxLAN GBP encap/decap.
>>>
>>> Example of encap rule:
>>> tc filter add dev eth0 protocol ip ingress flower \
>>> action tunnel_key set id 42 vxlan_opts 512 \
>>> action mirred egress redirect dev vxlan1
>>>
>>> Example of decap rule:
>>> tc filter add dev vxlan1 protocol ip ingress flower \
>>> enc_key_id 42 enc_dst_port 4789 vxlan_opts 1024 \
>>> action tunnel_key unset action mirred egress redirect dev eth0
>>>
>>> Change-Id: I48f61d02201bf3f79dcbe5d0f022f7bb27ed630f
>>> Signed-off-by: Gavin Li <[email protected]>
>>> Reviewed-by: Roi Dayan <[email protected]>
>>> Reviewed-by: Maor Dickman <[email protected]>
>>> Acked-by: Saeed Mahameed <[email protected]>
>>> ---
>>> .../mellanox/mlx5/core/en/tc_tun_vxlan.c | 85
>>> ++++++++++++++++++-
>>> include/linux/mlx5/device.h | 6 ++
>>> include/linux/mlx5/mlx5_ifc.h | 13 ++-
>>> 3 files changed, 100 insertions(+), 4 deletions(-)
>>>
>>> diff --git
>>> a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>>> b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>>> index 1f62c702b625..444512ca9e0d 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_vxlan.c
>>> @@ -1,6 +1,7 @@
>>> // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
>>> /* Copyright (c) 2018 Mellanox Technologies. */
>>>
>>> +#include <net/ip_tunnels.h>
>>> #include <net/vxlan.h>
>>> #include "lib/vxlan.h"
>>> #include "en/tc_tun.h"
>>> @@ -86,9 +87,11 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>> buf[],
>>> const struct ip_tunnel_key *tun_key = &e->tun_info->key;
>>> __be32 tun_id = tunnel_id_to_key32(tun_key->tun_id);
>>> struct udphdr *udp = (struct udphdr *)(buf);
>>> + const struct vxlan_metadata *md;
>>> struct vxlanhdr *vxh;
>>>
>>> - if (tun_key->tun_flags & TUNNEL_VXLAN_OPT)
>>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT &&
>> A separate pair of braces is preferred around bitops.
ACK
>>
>>> + e->tun_info->options_len != sizeof(*md))
>>> return -EOPNOTSUPP;
>>> vxh = (struct vxlanhdr *)((char *)udp + sizeof(struct udphdr));
>>> *ip_proto = IPPROTO_UDP;
>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>> buf[],
>>> udp->dest = tun_key->tp_dst;
>>> vxh->vx_flags = VXLAN_HF_VNI;
>>> vxh->vx_vni = vxlan_vni_field(tun_id);
>>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>> + md = ip_tunnel_info_opts((struct ip_tunnel_info
>>> *)e->tun_info);
>>> + vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>> + (struct vxlan_metadata *)md);
>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>> arguments instead of working around by casting away?
> ACK. Sorry for the confusion---I misunderstood the comment.
This ip_tunnel_info_opts is tricky to use const to annotate the arg
because it will have to cast from const to non-const again upon returning.
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int mlx5e_tc_tun_parse_vxlan_gbp_option(struct mlx5e_priv
>>> *priv,
>>> + struct mlx5_flow_spec
>>> *spec,
>>> + struct flow_cls_offload
>>> *f)
>>> +{
>>> + struct flow_rule *rule = flow_cls_offload_flow_rule(f);
>>> + struct netlink_ext_ack *extack = f->common.extack;
>>> + struct flow_match_enc_opts enc_opts;
>>> + void *misc5_c, *misc5_v;
>>> + u32 *gbp, *gbp_mask;
>>> +
>>> + flow_rule_match_enc_opts(rule, &enc_opts);
>>> +
>>> + if (memchr_inv(&enc_opts.mask->data, 0,
>>> sizeof(enc_opts.mask->data)) &&
>>> + !MLX5_CAP_ESW_FT_FIELD_SUPPORT_2(priv->mdev,
>>> tunnel_header_0_1)) {
>>> + NL_SET_ERR_MSG_MOD(extack,
>>> + "Matching on VxLAN GBP is not
>>> supported");
>>> + netdev_warn(priv->netdev,
>>> + "Matching on VxLAN GBP is not supported\n");
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + if (enc_opts.key->dst_opt_type != TUNNEL_VXLAN_OPT) {
>>> + NL_SET_ERR_MSG_MOD(extack,
>>> + "Wrong VxLAN option type: not GBP");
>> Fits into one line I believe.
ACK
>>
>>> + netdev_warn(priv->netdev,
>>> + "Wrong VxLAN option type: not GBP\n");
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + if (enc_opts.key->len != sizeof(*gbp) ||
>>> + enc_opts.mask->len != sizeof(*gbp_mask)) {
>>> + NL_SET_ERR_MSG_MOD(extack,
>>> + "VxLAN GBP option/mask len is not
>>> 32 bits");
>>> + netdev_warn(priv->netdev,
>>> + "VxLAN GBP option/mask len is not 32
>>> bits\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> + gbp = (u32 *)&enc_opts.key->data[0];
>>> + gbp_mask = (u32 *)&enc_opts.mask->data[0];
>>> +
>>> + if (*gbp_mask & ~VXLAN_GBP_MASK) {
>>> + NL_SET_ERR_MSG_MOD(extack,
>>> + "Wrong VxLAN GBP mask");
>> You can use new NL_SET_ERR_MSG_FMT_MOD() here to print @gbp_mask to the
>> user, as you do it next line.
ACK
>>
>>> + netdev_warn(priv->netdev,
>>> + "Wrong VxLAN GBP mask(0x%08X)\n", *gbp_mask);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + misc5_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria,
>>> misc_parameters_5);
>>> + misc5_v = MLX5_ADDR_OF(fte_match_param, spec->match_value,
>>> misc_parameters_5);
>>> + MLX5_SET(fte_match_set_misc5, misc5_c, tunnel_header_0,
>>> *gbp_mask);
>>> + MLX5_SET(fte_match_set_misc5, misc5_v, tunnel_header_0, *gbp);
>>> +
>>> + spec->match_criteria_enable |= MLX5_MATCH_MISC_PARAMETERS_5;
>>>
>>> return 0;
>>> }
>> Thanks,
>> Olek
From: Gavin Li <[email protected]>
Date: Wed, 15 Feb 2023 10:50:04 +0800
>
> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <[email protected]>
>> Date: Tue, 14 Feb 2023 15:41:37 +0200
>>
>>> Add HW offloading support for TC flows with VxLAN GBP encap/decap.
[...]
>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>> buf[],
>>> udp->dest = tun_key->tp_dst;
>>> vxh->vx_flags = VXLAN_HF_VNI;
>>> vxh->vx_vni = vxlan_vni_field(tun_id);
>>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>> + md = ip_tunnel_info_opts((struct ip_tunnel_info
>>> *)e->tun_info);
>>> + vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>> + (struct vxlan_metadata *)md);
>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>> arguments instead of working around by casting away?
> The reason to cast it is a WA to use ip_tunnel_info_opts which takes
> non-const arg while>
> e->tun_info here is a const one.
That's what I'm asking - why not make ip_tunnel_info_opts() and
vxlan_build_gbp_hdr() take const tun_info?
>
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
[...]
Thanks,
Olek
From: Gavin Li <[email protected]>
Date: Wed, 15 Feb 2023 11:36:24 +0800
>
> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <[email protected]>
>> Date: Tue, 14 Feb 2023 15:41:37 +0200
[...]
>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>> buf[],
>>> udp->dest = tun_key->tp_dst;
>>> vxh->vx_flags = VXLAN_HF_VNI;
>>> vxh->vx_vni = vxlan_vni_field(tun_id);
>>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>> + md = ip_tunnel_info_opts((struct ip_tunnel_info
>>> *)e->tun_info);
>>> + vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>> + (struct vxlan_metadata *)md);
>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>> arguments instead of working around by casting away?
> ACK. Sorry for the confusion---I misunderstood the comment.
Ah, no worries :D Sorry that I sent the prev mail and only then opened
this one.
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
Thanks,
Olek
From: Gavin Li <[email protected]>
Date: Wed, 15 Feb 2023 16:30:04 +0800
>
> On 2/15/2023 11:36 AM, Gavin Li wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> From: Gavin Li <[email protected]>
>>> Date: Tue, 14 Feb 2023 15:41:37 +0200
[...]
>>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>>> buf[],
>>>> udp->dest = tun_key->tp_dst;
>>>> vxh->vx_flags = VXLAN_HF_VNI;
>>>> vxh->vx_vni = vxlan_vni_field(tun_id);
>>>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>>> + md = ip_tunnel_info_opts((struct ip_tunnel_info
>>>> *)e->tun_info);
>>>> + vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>>> + (struct vxlan_metadata *)md);
>>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>>> arguments instead of working around by casting away?
>> ACK. Sorry for the confusion---I misunderstood the comment.
> This ip_tunnel_info_opts is tricky to use const to annotate the arg
> because it will have to cast from const to non-const again upon returning.
It's okay to cast away for the `void *` returned.
Alternatively, use can convert it to a macro and use
__builtin_choose_expr() or _Generic to return const or non-const
depending on whether the argument is constant. That's what was recently
done for container_of() IIRC.
>>>
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
[...]
Thanks,
Olek
On 2/16/2023 1:01 AM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <[email protected]>
> Date: Wed, 15 Feb 2023 16:30:04 +0800
>
>> On 2/15/2023 11:36 AM, Gavin Li wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> From: Gavin Li <[email protected]>
>>>> Date: Tue, 14 Feb 2023 15:41:37 +0200
> [...]
>
>>>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>>>> buf[],
>>>>> udp->dest = tun_key->tp_dst;
>>>>> vxh->vx_flags = VXLAN_HF_VNI;
>>>>> vxh->vx_vni = vxlan_vni_field(tun_id);
>>>>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>>>> + md = ip_tunnel_info_opts((struct ip_tunnel_info
>>>>> *)e->tun_info);
>>>>> + vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>>>> + (struct vxlan_metadata *)md);
>>>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>>>> arguments instead of working around by casting away?
>>> ACK. Sorry for the confusion---I misunderstood the comment.
>> This ip_tunnel_info_opts is tricky to use const to annotate the arg
>> because it will have to cast from const to non-const again upon returning.
> It's okay to cast away for the `void *` returned.
> Alternatively, use can convert it to a macro and use
> __builtin_choose_expr() or _Generic to return const or non-const
> depending on whether the argument is constant. That's what was recently
> done for container_of() IIRC.
I've fixed vxlan_build_gbp_hdr in V2. For ip_tunnel_info_opts, it's
confusing to me.
It would be as below after constifying the parameter.
static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
{
return (void *)(info + 1);
}
Is there any value gained by this change?
>
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
> [...]
>
> Thanks,
> Olek
From: Gavin Li <[email protected]>
Date: Thu, 16 Feb 2023 16:40:33 +0800
>
> On 2/16/2023 1:01 AM, Alexander Lobakin wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> From: Gavin Li <[email protected]>
>> Date: Wed, 15 Feb 2023 16:30:04 +0800
>>
>>> On 2/15/2023 11:36 AM, Gavin Li wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> From: Gavin Li <[email protected]>
>>>>> Date: Tue, 14 Feb 2023 15:41:37 +0200
>> [...]
>>
>>>>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>>>>> buf[],
>>>>>> udp->dest = tun_key->tp_dst;
>>>>>> vxh->vx_flags = VXLAN_HF_VNI;
>>>>>> vxh->vx_vni = vxlan_vni_field(tun_id);
>>>>>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>>>>> + md = ip_tunnel_info_opts((struct ip_tunnel_info
>>>>>> *)e->tun_info);
>>>>>> + vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>>>>> + (struct vxlan_metadata *)md);
>>>>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>>>>> arguments instead of working around by casting away?
>>>> ACK. Sorry for the confusion---I misunderstood the comment.
>>> This ip_tunnel_info_opts is tricky to use const to annotate the arg
>>> because it will have to cast from const to non-const again upon
>>> returning.
>> It's okay to cast away for the `void *` returned.
>> Alternatively, use can convert it to a macro and use
>> __builtin_choose_expr() or _Generic to return const or non-const
>> depending on whether the argument is constant. That's what was recently
>> done for container_of() IIRC.
>
> I've fixed vxlan_build_gbp_hdr in V2. For ip_tunnel_info_opts, it's
> confusing to me.
>
> It would be as below after constifying the parameter.
>
> static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
> {
> return (void *)(info + 1);
> }
> Is there any value gained by this change?
You wouldn't need to W/A it each time in each driver, just do it once in
the inline itself.
I did it once in __skb_header_pointer()[0] to be able to pass data
pointer as const to optimize code a bit and point out explicitly that
the function doesn't modify the packet anyhow, don't see any reason to
not do the same here.
Or, as I said, you can use macros + __builtin_choose_expr() or _Generic.
container_of_const() uses the latter[1]. A __builtin_choose_expr()
variant could rely on the __same_type() macro to check whether the
pointer passed from the driver const or not.
>
>>
>>>>>> + }
>>>>>> +
>>>>>> + return 0;
>>>>>> +}
>> [...]
>>
>> Thanks,
>> Olek
[0]
https://elixir.bootlin.com/linux/v6.2-rc8/source/include/linux/skbuff.h#L3992
[1]
https://elixir.bootlin.com/linux/v6.2-rc8/source/include/linux/container_of.h#L33
Thanks,
Olek
On 2/16/2023 11:19 PM, Alexander Lobakin wrote:
> External email: Use caution opening links or attachments
>
>
> From: Gavin Li <[email protected]>
> Date: Thu, 16 Feb 2023 16:40:33 +0800
>
>> On 2/16/2023 1:01 AM, Alexander Lobakin wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> From: Gavin Li <[email protected]>
>>> Date: Wed, 15 Feb 2023 16:30:04 +0800
>>>
>>>> On 2/15/2023 11:36 AM, Gavin Li wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On 2/14/2023 11:26 PM, Alexander Lobakin wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> From: Gavin Li <[email protected]>
>>>>>> Date: Tue, 14 Feb 2023 15:41:37 +0200
>>> [...]
>>>
>>>>>>> @@ -96,6 +99,70 @@ static int mlx5e_gen_ip_tunnel_header_vxlan(char
>>>>>>> buf[],
>>>>>>> udp->dest = tun_key->tp_dst;
>>>>>>> vxh->vx_flags = VXLAN_HF_VNI;
>>>>>>> vxh->vx_vni = vxlan_vni_field(tun_id);
>>>>>>> + if (tun_key->tun_flags & TUNNEL_VXLAN_OPT) {
>>>>>>> + md = ip_tunnel_info_opts((struct ip_tunnel_info
>>>>>>> *)e->tun_info);
>>>>>>> + vxlan_build_gbp_hdr(vxh, tun_key->tun_flags,
>>>>>>> + (struct vxlan_metadata *)md);
>>>>>> Maybe constify both ip_tunnel_info_opts() and vxlan_build_gbp_hdr()
>>>>>> arguments instead of working around by casting away?
>>>>> ACK. Sorry for the confusion---I misunderstood the comment.
>>>> This ip_tunnel_info_opts is tricky to use const to annotate the arg
>>>> because it will have to cast from const to non-const again upon
>>>> returning.
>>> It's okay to cast away for the `void *` returned.
>>> Alternatively, use can convert it to a macro and use
>>> __builtin_choose_expr() or _Generic to return const or non-const
>>> depending on whether the argument is constant. That's what was recently
>>> done for container_of() IIRC.
>> I've fixed vxlan_build_gbp_hdr in V2. For ip_tunnel_info_opts, it's
>> confusing to me.
>>
>> It would be as below after constifying the parameter.
>>
>> static inline void *ip_tunnel_info_opts(const struct ip_tunnel_info *info)
>> {
>> return (void *)(info + 1);
>> }
>> Is there any value gained by this change?
> You wouldn't need to W/A it each time in each driver, just do it once in
> the inline itself.
> I did it once in __skb_header_pointer()[0] to be able to pass data
> pointer as const to optimize code a bit and point out explicitly that
> the function doesn't modify the packet anyhow, don't see any reason to
> not do the same here.
> Or, as I said, you can use macros + __builtin_choose_expr() or _Generic.
> container_of_const() uses the latter[1]. A __builtin_choose_expr()
> variant could rely on the __same_type() macro to check whether the
> pointer passed from the driver const or not.
ACK
>
>>>>>>> + }
>>>>>>> +
>>>>>>> + return 0;
>>>>>>> +}
>>> [...]
>>>
>>> Thanks,
>>> Olek
> [0]
> https://elixir.bootlin.com/linux/v6.2-rc8/source/include/linux/skbuff.h#L3992
> [1]
> https://elixir.bootlin.com/linux/v6.2-rc8/source/include/linux/container_of.h#L33
>
> Thanks,
> Olek