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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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