netdev - Re: [net-next 03/15] net/mlx5e: PFC stall prevention support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <220ed51e-5aeb-1a82-6c43-749e9cf8c4c1@mellanox.com>
Date:   Sun, 25 Mar 2018 13:28:30 +0300
From:   Gal Pressman <galp@...lanox.com>
To:     Andrew Lunn <andrew@...n.ch>, Saeed Mahameed <saeedm@...lanox.com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Inbar Karmy <inbark@...lanox.com>
Subject: Re: [net-next 03/15] net/mlx5e: PFC stall prevention support

On 24-Mar-18 18:07, Andrew Lunn wrote:
> On Fri, Mar 23, 2018 at 03:39:13PM -0700, Saeed Mahameed wrote:
>> From: Inbar Karmy <inbark@...lanox.com>
>>
>> Implement set/get functions to configure PFC stall prevention
>> timeout by tunables api through ethtool.
>> By default the stall prevention timeout is configured to 8 sec.
>> Timeout range is: 80-8000 msec.
>> Enabling stall prevention without a specific timeout will set
>> the timeout to 100 msec.
> 
> Hi Inbar
> 
> I think this last sentence should be talking about auto, not without a
> specific timeout.

Hi Andrew,
Good catch, we will fix that.

> 
>>
>> Signed-off-by: Inbar Karmy <inbark@...lanox.com>
>> Signed-off-by: Saeed Mahameed <saeedm@...lanox.com>
>> ---
>>  .../net/ethernet/mellanox/mlx5/core/en_ethtool.c   | 57 +++++++++++++++++++
>>  drivers/net/ethernet/mellanox/mlx5/core/port.c     | 64 +++++++++++++++++++---
>>  include/linux/mlx5/mlx5_ifc.h                      | 17 ++++--
>>  include/linux/mlx5/port.h                          |  6 ++
>>  4 files changed, 132 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> index cc8048f68f11..62061fd23143 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
>> @@ -1066,6 +1066,57 @@ static int mlx5e_get_rxnfc(struct net_device *netdev,
>>  	return err;
>>  }
>>  
>> +#define MLX5E_PFC_PREVEN_AUTO_TOUT_MSEC		100
>> +#define MLX5E_PFC_PREVEN_TOUT_MAX_MSEC		8000
>> +#define MLX5E_PFC_PREVEN_MINOR_PRECENT		85
>> +#define MLX5E_PFC_PREVEN_TOUT_MIN_MSEC		80
>> +#define MLX5E_DEVICE_STALL_MINOR_WATERMARK(critical_tout) \
>> +	max_t(u16, MLX5E_PFC_PREVEN_TOUT_MIN_MSEC, \
>> +	      (critical_tout * MLX5E_PFC_PREVEN_MINOR_PRECENT) / 100)
>> +
>> +static int mlx5e_get_pfc_prevention_tout(struct net_device *netdev,
>> +					 u16 *pfc_prevention_tout)
>> +{
>> +	struct mlx5e_priv *priv    = netdev_priv(netdev);
>> +	struct mlx5_core_dev *mdev = priv->mdev;
>> +
>> +	if (!MLX5_CAP_PCAM_FEATURE((priv)->mdev, pfcc_mask) ||
>> +	    !MLX5_CAP_DEBUG((priv)->mdev, stall_detect))
>> +		return -EOPNOTSUPP;
>> +
>> +	return mlx5_query_port_stall_watermark(mdev, pfc_prevention_tout, NULL);
> 
> Shouldn't you map a value of MLX5E_PFC_PREVEN_AUTO_TOUT_MSEC back to 
> PFC_STORM_PREVENTION_AUTO?

We discussed this point internally, mapping MLX5E_PFC_PREVEN_AUTO_TOUT_MSEC (100) to
PFC_STORM_PREVENTION_AUTO might cause confusion when the user explicitly asks for 100msec timeout
and gets auto in his following query.
Also, this way the "auto" timeout is visible to the user, which might help him get an initial
clue of which values are recommended.

> 
> 	Andrew
>