| 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: Fri, 11 Sep 2020 21:49:10 +0000
From: Saeed Mahameed <saeedm@...dia.com>
To: "kuba@...nel.org" <kuba@...nel.org>,
"davem@...emloft.net" <davem@...emloft.net>
CC: "mkubecek@...e.cz" <mkubecek@...e.cz>,
Tariq Toukan <tariqt@...dia.com>,
"michael.chan@...adcom.com" <michael.chan@...adcom.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"alexander.duyck@...il.com" <alexander.duyck@...il.com>,
"andrew@...n.ch" <andrew@...n.ch>
Subject: Re: [PATCH net-next 6/8] mlx5: add pause frame stats
On Fri, 2020-09-11 at 12:52 -0700, Jakub Kicinski wrote:
> Plumb through all the indirection and copy some code from
> ethtool -S. The names of the group indicate that these are
> the stats we are after.
>
Yes they are.
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en.h | 2 ++
> .../ethernet/mellanox/mlx5/core/en_ethtool.c | 15 ++++++++++
> .../net/ethernet/mellanox/mlx5/core/en_rep.c | 9 ++++++
> .../ethernet/mellanox/mlx5/core/en_stats.c | 30
> +++++++++++++++++++
> .../ethernet/mellanox/mlx5/core/en_stats.h | 3 ++
> 5 files changed, 59 insertions(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> index 4f33658da25a..615ac0dc248e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
> @@ -1053,6 +1053,8 @@ int mlx5e_ethtool_get_ts_info(struct mlx5e_priv
> *priv,
> struct ethtool_ts_info *info);
> int mlx5e_ethtool_flash_device(struct mlx5e_priv *priv,
> struct ethtool_flash *flash);
> +void mlx5e_ethtool_get_pause_stats(struct mlx5e_priv *priv,
> + struct ethtool_pause_stats
> *pause_stats);
> void mlx5e_ethtool_get_pauseparam(struct mlx5e_priv *priv,
> struct ethtool_pauseparam
> *pauseparam);
> int mlx5e_ethtool_set_pauseparam(struct mlx5e_priv *priv,
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> index 5cb1e4839eb7..c9fb3c018b96 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
> @@ -1341,6 +1341,20 @@ static int mlx5e_set_tunable(struct net_device
> *dev,
> return err;
> }
>
> +void mlx5e_ethtool_get_pause_stats(struct mlx5e_priv *priv,
> + struct ethtool_pause_stats
> *pause_stats)
> +{
> + mlx5e_stats_pause_get(priv, pause_stats);
this function is only being called here, I would just unfold it here
and skip the redundant definition in en_stat.c and declaration in
en_stats.h.
> +}
> +
> +static void mlx5e_get_pause_stats(struct net_device *netdev,
> + struct ethtool_pause_stats
> *pause_stats)
> +{
> + struct mlx5e_priv *priv = netdev_priv(netdev);
> +
> + mlx5e_ethtool_get_pause_stats(priv, pause_stats);
> +}
> +
> void mlx5e_ethtool_get_pauseparam(struct mlx5e_priv *priv,
> struct ethtool_pauseparam
> *pauseparam)
> {
> @@ -2033,6 +2047,7 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
> .set_rxnfc = mlx5e_set_rxnfc,
> .get_tunable = mlx5e_get_tunable,
> .set_tunable = mlx5e_set_tunable,
> + .get_pause_stats = mlx5e_get_pause_stats,
> .get_pauseparam = mlx5e_get_pauseparam,
> .set_pauseparam = mlx5e_set_pauseparam,
> .get_ts_info = mlx5e_get_ts_info,
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> index 135ee26881c9..856ae4c8cb25 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rep.c
> @@ -288,6 +288,14 @@ static u32 mlx5e_rep_get_rxfh_indir_size(struct
> net_device *netdev)
> return mlx5e_ethtool_get_rxfh_indir_size(priv);
> }
>
> +static void mlx5e_uplink_rep_get_pause_stats(struct net_device
> *netdev,
> + struct ethtool_pause_stats
> *stats)
> +{
> + struct mlx5e_priv *priv = netdev_priv(netdev);
> +
> + mlx5e_ethtool_get_pause_stats(priv, stats);
> +}
> +
> static void mlx5e_uplink_rep_get_pauseparam(struct net_device
> *netdev,
> struct ethtool_pauseparam
> *pauseparam)
> {
> @@ -362,6 +370,7 @@ static const struct ethtool_ops
> mlx5e_uplink_rep_ethtool_ops = {
> .set_rxfh = mlx5e_set_rxfh,
> .get_rxnfc = mlx5e_get_rxnfc,
> .set_rxnfc = mlx5e_set_rxnfc,
> + .get_pause_stats = mlx5e_uplink_rep_get_pause_stats,
> .get_pauseparam = mlx5e_uplink_rep_get_pauseparam,
> .set_pauseparam = mlx5e_uplink_rep_set_pauseparam,
> };
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> index e3b2f59408e6..97b03aa7535f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> @@ -677,6 +677,36 @@ static
> MLX5E_DECLARE_STATS_GRP_OP_UPDATE_STATS(802_3)
> mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPCNT, 0,
> 0);
> }
>
> +#define MLX5E_READ_CTR64_BE_F(ptr, c) \
> + be64_to_cpu(*(__be64 *)((char *)ptr + \
> + MLX5_BYTE_OFF(ppcnt_reg, \
> + counter_set.eth_802_3_cntrs_grp_data_layout.c##
> _high)))
> +
> +void mlx5e_stats_pause_get(struct mlx5e_priv *priv,
> + struct ethtool_pause_stats *pause_stats)
> +{
> + struct mlx5e_pport_stats *pstats = &priv->stats.pport;
> + struct mlx5_core_dev *mdev = priv->mdev;
> + u32 in[MLX5_ST_SZ_DW(ppcnt_reg)] = {0};
no need for explicit array initializer ^^ just {} should be good
enough.
> + int sz = MLX5_ST_SZ_BYTES(ppcnt_reg);
> + void *out;
> +
> + if (!MLX5_BASIC_PPCNT_SUPPORTED(mdev))
> + return;
> +
> + MLX5_SET(ppcnt_reg, in, local_port, 1);
> + out = pstats->IEEE_802_3_counters;
you are reading the outbox buffer into a shared buffer, and this could
lead to weird race conditions with ethtool stats, maybe both ethtool
stats and pause_stats are protected with rtnl_lock, but let's not make
any assumption here and use a local buffer for this flow anyway.
just define "out" to be
u32 ppcnt_ieee_802_3[MLX5_ST_SZ_DW(ppcnt_reg)];
I hope this is not too big for the stack.
> + MLX5_SET(ppcnt_reg, in, grp, MLX5_IEEE_802_3_COUNTERS_GROUP);
> + mlx5_core_access_reg(mdev, in, sz, out, sz, MLX5_REG_PPCNT, 0,
replace "out" here ^^^ with ppcnt_ieee_802_3
> 0);
> +
> + pause_stats->tx_pause_frames =
> + MLX5E_READ_CTR64_BE_F(&priv-
> >stats.pport.IEEE_802_3_counters,
> + a_pause_mac_ctrl_frames_transmitt
> ed);
> + pause_stats->rx_pause_frames =
> + MLX5E_READ_CTR64_BE_F(&priv-
> >stats.pport.IEEE_802_3_counters,
> + a_pause_mac_ctrl_frames_received)
> ;
in the above 2 lines you could have just used "out" instead of
&priv->stats.pport.IEEE_802_3_counters, but with my suggestion above
just use the local buffer.
> +}
> +
> #define PPORT_2863_OFF(c) \
> MLX5_BYTE_OFF(ppcnt_reg, \
> counter_set.eth_2863_cntrs_grp_data_layout.c##_hi
> gh)
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> index 2e1cca1923b9..9d9ee269a041 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.h
> @@ -104,6 +104,9 @@ void mlx5e_stats_update(struct mlx5e_priv *priv);
> void mlx5e_stats_fill(struct mlx5e_priv *priv, u64 *data, int idx);
> void mlx5e_stats_fill_strings(struct mlx5e_priv *priv, u8 *data);
>
> +void mlx5e_stats_pause_get(struct mlx5e_priv *priv,
> + struct ethtool_pause_stats *pause_stats);
> +
> /* Concrete NIC Stats */
>
> struct mlx5e_sw_stats {
Powered by blists - more mailing lists