| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 2 Feb 2023 13:07:15 +0200
From: Leon Romanovsky <leon@...nel.org>
To: Paul Blakey <paulb@...dia.com>
Cc: netdev@...r.kernel.org, Saeed Mahameed <saeedm@...dia.com>,
Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Jamal Hadi Salim <jhs@...atatu.com>,
Cong Wang <xiyou.wangcong@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Oz Shlomo <ozsh@...dia.com>, Jiri Pirko <jiri@...dia.com>,
Roi Dayan <roid@...dia.com>, Vlad Buslov <vladbu@...dia.com>
Subject: Re: [PATCH net-next v7 4/6] net/mlx5: Refactor tc miss handling to a
single function
On Tue, Jan 31, 2023 at 11:10:25AM +0200, Paul Blakey wrote:
> Move tc miss handling code to en_tc.c, and remove
> duplicate code.
>
> Signed-off-by: Paul Blakey <paulb@...dia.com>
> Reviewed-by: Roi Dayan <roid@...dia.com>
> ---
> .../ethernet/mellanox/mlx5/core/en/rep/tc.c | 224 ++----------------
> .../net/ethernet/mellanox/mlx5/core/en_rx.c | 4 +-
> .../net/ethernet/mellanox/mlx5/core/en_tc.c | 221 +++++++++++++++--
> .../net/ethernet/mellanox/mlx5/core/en_tc.h | 11 +-
> 4 files changed, 231 insertions(+), 229 deletions(-)
<...>
> void mlx5e_rep_tc_receive(struct mlx5_cqe64 *cqe, struct mlx5e_rq *rq,
> struct sk_buff *skb)
> {
> - u32 reg_c1 = be32_to_cpu(cqe->ft_metadata);
> + u32 reg_c1 = be32_to_cpu(cqe->ft_metadata), reg_c0, zone_restore_id, tunnel_id;
I would recommend to put reg_c1 = be32_to_cpu(cqe->ft_metadata) above
reg_c0 = ... below and not as part of long list of variables.
> struct mlx5e_tc_update_priv tc_priv = {};
> - struct mlx5_mapped_obj mapped_obj;
> + struct mlx5_rep_uplink_priv *uplink_priv;
> + struct mlx5e_rep_priv *uplink_rpriv;
> + struct mlx5_tc_ct_priv *ct_priv;
> + struct mapping_ctx *mapping_ctx;
> struct mlx5_eswitch *esw;
> - bool forward_tx = false;
> struct mlx5e_priv *priv;
> - u32 reg_c0;
> - int err;
>
> reg_c0 = (be32_to_cpu(cqe->sop_drop_qpn) & MLX5E_TC_FLOW_ID_MASK);
> if (!reg_c0 || reg_c0 == MLX5_FS_DEFAULT_FLOW_TAG)
> goto forward;
<...>
> +static bool mlx5e_tc_restore_tunnel(struct mlx5e_priv *priv, struct sk_buff *skb,
> + struct mlx5e_tc_update_priv *tc_priv,
> + u32 tunnel_id)
> {
> -#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
> - u32 chain = 0, chain_tag, reg_b, zone_restore_id;
> - struct mlx5e_priv *priv = netdev_priv(skb->dev);
> - struct mlx5_mapped_obj mapped_obj;
> - struct tc_skb_ext *tc_skb_ext;
> - struct mlx5e_tc_table *tc;
> + struct mlx5_eswitch *esw = priv->mdev->priv.eswitch;
> + struct tunnel_match_enc_opts enc_opts = {};
> + struct mlx5_rep_uplink_priv *uplink_priv;
> + struct mlx5e_rep_priv *uplink_rpriv;
> + struct metadata_dst *tun_dst;
> + struct tunnel_match_key key;
> + u32 tun_id, enc_opts_id;
> + struct net_device *dev;
> int err;
>
> - reg_b = be32_to_cpu(cqe->ft_metadata);
> - tc = mlx5e_fs_get_tc(priv->fs);
> - chain_tag = reg_b & MLX5E_TC_TABLE_CHAIN_TAG_MASK;
> + enc_opts_id = tunnel_id & ENC_OPTS_BITS_MASK;
> + tun_id = tunnel_id >> ENC_OPTS_BITS;
>
> - err = mapping_find(tc->mapping, chain_tag, &mapped_obj);
> + if (!tun_id)
> + return true;
> +
> + uplink_rpriv = mlx5_eswitch_get_uplink_priv(esw, REP_ETH);
> + uplink_priv = &uplink_rpriv->uplink_priv;
> +
> + err = mapping_find(uplink_priv->tunnel_mapping, tun_id, &key);
> if (err) {
> netdev_dbg(priv->netdev,
> - "Couldn't find chain for chain tag: %d, err: %d\n",
> - chain_tag, err);
> + "Couldn't find tunnel for tun_id: %d, err: %d\n",
> + tun_id, err);
> + return false;
> + }
> +
> + if (enc_opts_id) {
> + err = mapping_find(uplink_priv->tunnel_enc_opts_mapping,
> + enc_opts_id, &enc_opts);
> + if (err) {
> + netdev_dbg(priv->netdev,
> + "Couldn't find tunnel (opts) for tun_id: %d, err: %d\n",
> + enc_opts_id, err);
> + return false;
> + }
> + }
> +
> + if (key.enc_control.addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
> + tun_dst = __ip_tun_set_dst(key.enc_ipv4.src, key.enc_ipv4.dst,
> + key.enc_ip.tos, key.enc_ip.ttl,
> + key.enc_tp.dst, TUNNEL_KEY,
> + key32_to_tunnel_id(key.enc_key_id.keyid),
> + enc_opts.key.len);
> + } else if (key.enc_control.addr_type == FLOW_DISSECTOR_KEY_IPV6_ADDRS) {
> + tun_dst = __ipv6_tun_set_dst(&key.enc_ipv6.src, &key.enc_ipv6.dst,
> + key.enc_ip.tos, key.enc_ip.ttl,
> + key.enc_tp.dst, 0, TUNNEL_KEY,
> + key32_to_tunnel_id(key.enc_key_id.keyid),
> + enc_opts.key.len);
> + } else {
> + netdev_dbg(priv->netdev,
> + "Couldn't restore tunnel, unsupported addr_type: %d\n",
> + key.enc_control.addr_type);
> return false;
> }
I know that you moved this code from some other place, but it is not
mlx5 style, we use switch<->case for code like this.
>
> - if (mapped_obj.type == MLX5_MAPPED_OBJ_CHAIN) {
> - chain = mapped_obj.chain;
> + if (!tun_dst) {
> + netdev_dbg(priv->netdev, "Couldn't restore tunnel, no tun_dst\n");
> + return false;
> + }
> +
<...>
> +static bool mlx5e_tc_restore_skb_chain(struct sk_buff *skb, struct mlx5_tc_ct_priv *ct_priv,
> + u32 chain, u32 zone_restore_id,
> + u32 tunnel_id, struct mlx5e_tc_update_priv *tc_priv)
> +{
> + struct mlx5e_priv *priv = netdev_priv(skb->dev);
> + struct tc_skb_ext *tc_skb_ext;
> +
> +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
Is it possible to rewrite all these "#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)"
to something more close to recommended kernel style?
if (IS_ENABLED(CONFIG_NET_TC_SKB_EXT) && chain) {
...
Spaghetti code like this is not very friendly to ctags.
> + if (chain) {
> + if (!mlx5e_tc_ct_restore_flow(ct_priv, skb, zone_restore_id))
> + return false;
> +
> tc_skb_ext = tc_skb_ext_alloc(skb);
> - if (WARN_ON(!tc_skb_ext))
> + if (!tc_skb_ext) {
> + WARN_ON(1);
> return false;
> + }
>
> tc_skb_ext->chain = chain;
> + }
> +#endif /* CONFIG_NET_TC_SKB_EXT */
<...>
> +bool mlx5e_tc_update_skb_nic(struct mlx5_cqe64 *cqe, struct sk_buff *skb)
This function is declared in .h file and has empty implementation for not
enabled CONFIG_NET_TC_SKB_EXT. Why do you need this IS_ENABLED?
> +{
> +#if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
> + struct mlx5e_priv *priv = netdev_priv(skb->dev);
> + u32 mapped_obj_id, reg_b, zone_restore_id;
> + struct mlx5_tc_ct_priv *ct_priv;
> + struct mapping_ctx *mapping_ctx;
> + struct mlx5e_tc_table *tc;
> +
> + reg_b = be32_to_cpu(cqe->ft_metadata);
> + tc = mlx5e_fs_get_tc(priv->fs);
> + mapped_obj_id = reg_b & MLX5E_TC_TABLE_CHAIN_TAG_MASK;
> + zone_restore_id = (reg_b >> MLX5_REG_MAPPING_MOFFSET(NIC_ZONE_RESTORE_TO_REG)) &
> + ESW_ZONE_ID_MASK;
> + ct_priv = tc->ct;
> + mapping_ctx = tc->mapping;
> +
> + return mlx5e_tc_update_skb(cqe, skb, mapping_ctx, mapped_obj_id, ct_priv, zone_restore_id,
> + 0, NULL);
> #endif /* CONFIG_NET_TC_SKB_EXT */
>
> return true;
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
> index ce516dc7f3fd..4fa5d4e024cd 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.h
> @@ -59,6 +59,8 @@ int mlx5e_tc_num_filters(struct mlx5e_priv *priv, unsigned long flags);
>
> struct mlx5e_tc_update_priv {
> struct net_device *fwd_dev;
> + bool skb_done;
> + bool forward_tx;
> };
>
> struct mlx5_nic_flow_attr {
> @@ -386,14 +388,19 @@ static inline bool mlx5e_cqe_regb_chain(struct mlx5_cqe64 *cqe)
> return false;
> }
>
> -bool mlx5e_tc_update_skb(struct mlx5_cqe64 *cqe, struct sk_buff *skb);
> +bool mlx5e_tc_update_skb_nic(struct mlx5_cqe64 *cqe, struct sk_buff *skb);
> +bool mlx5e_tc_update_skb(struct mlx5_cqe64 *cqe, struct sk_buff *skb,
> + struct mapping_ctx *mapping_ctx, u32 mapped_obj_id,
> + struct mlx5_tc_ct_priv *ct_priv,
> + u32 zone_restore_id, u32 tunnel_id,
> + struct mlx5e_tc_update_priv *tc_priv);
> #else /* CONFIG_MLX5_CLS_ACT */
> static inline struct mlx5e_tc_table *mlx5e_tc_table_alloc(void) { return NULL; }
> static inline void mlx5e_tc_table_free(struct mlx5e_tc_table *tc) {}
> static inline bool mlx5e_cqe_regb_chain(struct mlx5_cqe64 *cqe)
> { return false; }
> static inline bool
> -mlx5e_tc_update_skb(struct mlx5_cqe64 *cqe, struct sk_buff *skb)
> +mlx5e_tc_update_skb_nic(struct mlx5_cqe64 *cqe, struct sk_buff *skb)
> { return true; }
> #endif
>
> --
> 2.30.1
>
>
Powered by blists - more mailing lists