Hi Dave,
This series provides devlink parameters updates to both devlink API and
mlx5 driver, it is a 2nd iteration of the dropped patches sent in a previous
mlx5 submission "net/mlx5: Support PCIe buffer congestion handling via
Devlink" to address review comments [1].
Changes from the original series:
- According to the discussion outcome, we are keeping the congestion control
setting as mlx5 device specific for the current HW generation.
- Changed the congestion_mode and congestion action param type to string
- Added patches to fix devlink handling of param type string
- Added a patch which adds extack messages support for param set.
- At the end of this series, I've added yet another mlx5 devlink related
feature, firmware snapshot support.
For more information please see tag log below.
Please pull and let me know if there's any problem.
[1] https://patchwork.ozlabs.org/patch/945996/
Thanks,
Saeed.
---
The following changes since commit e6476c21447c4b17c47e476aade6facf050f31e8:
net: remove bogus RCU annotations on socket.wq (2018-07-31 12:40:22 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git tags/mlx5-updates-2018-08-01
for you to fetch changes up to 2ac6108c65ffcb1e5eab1fba1fd59272604d1c32:
net/mlx5: Use devlink region_snapshot parameter (2018-08-01 14:49:09 -0700)
----------------------------------------------------------------
mlx5-updates-2018-08-01
This series provides devlink parameters updates to both devlink API and
mlx5 driver,
1) Devlink changes: (Moshe Shemesh)
The first two patches fix devlink param infrastructure for string type
params.
The third patch adds a devlink helper function to safely copy string from
driver to devlink.
The forth patch adds extack support for param set.
2) mlx5 specific congestion parameters: (Eran Ben Elisha)
Next three patches add new devlink driver specific params for controlling
congestion action and mode, using string type params and extack messages support.
This congestion mode enables hw workaround in specific devices which is
controlled by devlink driver-specific params. The workaround is device
specific for this NIC generation, the next NIC will not need it.
Congestion parameters:
- Congestion action
HW W/A mechanism in the PCIe buffer which monitors the amount of
consumed PCIe buffer per host. This mechanism supports the
following actions in case of threshold overflow:
- Disabled - NOP (Default)
- Drop
- Mark - Mark CE bit in the CQE of received packet
- Congestion mode
- Aggressive - Aggressive static trigger threshold (Default)
- Dynamic - Dynamically change the trigger threshold
3) mlx5 firmware snapshot support via devlink: (Alex Vesker)
Last three patches, add the support for capturing region snapshot of the
firmware crspace during critical errors, using devlink region_snapshot
parameter.
-Saeed.
----------------------------------------------------------------
Alex Vesker (3):
net/mlx5: Add Vendor Specific Capability access gateway
net/mlx5: Add Crdump FW snapshot support
net/mlx5: Use devlink region_snapshot parameter
Eran Ben Elisha (3):
net/mlx5: Move all devlink related functions calls to devlink.c
net/mlx5: Add MPEGC register configuration functionality
net/mlx5: Enable PCIe buffer congestion handling workaround via devlink
Moshe Shemesh (4):
devlink: Fix param set handling for string type
devlink: Fix param cmode driverinit for string type
devlink: Add helper function for safely copy string param
devlink: Add extack messages support to param set
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 3 +-
drivers/net/ethernet/mellanox/mlx4/main.c | 6 +-
drivers/net/ethernet/mellanox/mlx5/core/Makefile | 3 +-
drivers/net/ethernet/mellanox/mlx5/core/devlink.c | 388 +++++++++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/devlink.h | 13 +
.../net/ethernet/mellanox/mlx5/core/diag/crdump.c | 223 ++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/health.c | 3 +
drivers/net/ethernet/mellanox/mlx5/core/lib/mlx5.h | 4 +
.../net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c | 320 +++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h | 56 +++
drivers/net/ethernet/mellanox/mlx5/core/main.c | 10 +-
include/linux/mlx5/driver.h | 5 +
include/net/devlink.h | 15 +-
net/core/devlink.c | 44 ++-
14 files changed, 1076 insertions(+), 17 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.h
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/diag/crdump.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h
From: Moshe Shemesh <redacted>
In case devlink param type is string, it needs to copy the string value
it got from the input to devlink_param_value.
Fixes: e3b7ca18ad7b ("devlink: Add param set command")
Signed-off-by: Moshe Shemesh <redacted>
Acked-by: Jiri Pirko <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
include/net/devlink.h | 2 +-
net/core/devlink.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
From: Moshe Shemesh <redacted>
Driverinit configuration mode value is held by devlink to enable the
driver fetch the value after reload command. In case the param type is
string devlink should copy the value from driver string buffer to
devlink string buffer on devlink_param_driverinit_value_set() and
vice-versa on devlink_param_driverinit_value_get().
Fixes: ec01aeb1803e ("devlink: Add support for get/set driverinit value")
Signed-off-by: Moshe Shemesh <redacted>
Acked-by: Jiri Pirko <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
net/core/devlink.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
From: Moshe Shemesh <redacted>
Devlink string param buffer is allocated at the size of
DEVLINK_PARAM_MAX_STRING_VALUE. Add helper function which makes sure
this size is not exceeded.
Renamed DEVLINK_PARAM_MAX_STRING_VALUE to
__DEVLINK_PARAM_MAX_STRING_VALUE to emphasize that it should be used by
devlink only. The driver should use the helper function instead to
verify it doesn't exceed the allowed length.
Signed-off-by: Moshe Shemesh <redacted>
Acked-by: Jiri Pirko <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
include/net/devlink.h | 12 ++++++++++--
net/core/devlink.c | 19 ++++++++++++++++++-
2 files changed, 28 insertions(+), 3 deletions(-)
From: Moshe Shemesh <redacted>
Add param extack messages support to param->set() function.
This will enable providing clear reasoning to user on setting new value
failure. Note that param->validate() already supports extack messages,
but it is not enough as input validation can pass or even not needed and
still setting new value may fail for some reason, such as device state,
FW support, etc.
Signed-off-by: Moshe Shemesh <redacted>
Signed-off-by: Jiri Pirko <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 3 ++-
drivers/net/ethernet/mellanox/mlx4/main.c | 6 ++++--
include/net/devlink.h | 3 ++-
net/core/devlink.c | 7 ++++---
4 files changed, 12 insertions(+), 7 deletions(-)
From: Eran Ben Elisha <redacted>
Centralize all devlink related callbacks in one file.
In the downstream patch, some more functionality will be added, this
patch is preparing the driver infrastructure for it.
Currently, move devlink un/register functions calls into this file.
Signed-off-by: Eran Ben Elisha <redacted>
Reviewed-by: Moshe Shemesh <redacted>
Reviewed-by: Jiri Pirko <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/Makefile | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/devlink.c | 14 ++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/devlink.h | 12 ++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/main.c | 5 +++--
4 files changed, 30 insertions(+), 3 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.h
@@ -0,0 +1,14 @@+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB+/* Copyright (c) 2018, Mellanox Technologies inc. All rights reserved. */++#include<devlink.h>++intmlx5_devlink_register(structdevlink*devlink,structdevice*dev)+{+returndevlink_register(devlink,dev);+}++voidmlx5_devlink_unregister(structdevlink*devlink)+{+devlink_unregister(devlink);+}
From: Eran Ben Elisha <redacted>
MPEGC register is used to configure and access the PCIe general
configuration.
Expose set/get for TX lossy overflow and TX overflow sense which use the
MPEGC register. These will be used in a downstream patch via devlink
params.
Signed-off-by: Eran Ben Elisha <redacted>
Reviewed-by: Moshe Shemesh <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
.../net/ethernet/mellanox/mlx5/core/devlink.c | 123 ++++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/devlink.h | 1 +
2 files changed, 124 insertions(+)
From: Alex Vesker <redacted>
The Vendor Specific Capability (VSC) is used to activate a gateway
interfacing with the device. The gateway is used to read or write
device configurations, which are organized in different domains (spaces).
A configuration access may result in multiple actions, reads, writes.
Example usages are accessing the Crspace domain to read the crspace or
locking a device semaphore using the Semaphore domain.
Signed-off-by: Alex Vesker <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
.../net/ethernet/mellanox/mlx5/core/Makefile | 2 +-
.../net/ethernet/mellanox/mlx5/core/health.c | 2 +
.../ethernet/mellanox/mlx5/core/lib/pci_vsc.c | 320 ++++++++++++++++++
.../ethernet/mellanox/mlx5/core/lib/pci_vsc.h | 56 +++
include/linux/mlx5/driver.h | 1 +
5 files changed, 380 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h
@@ -0,0 +1,320 @@+/*+*Copyright(c)2018,MellanoxTechnologies.Allrightsreserved.+*+*Thissoftwareisavailabletoyouunderachoiceofoneoftwo+*licenses.YoumaychoosetobelicensedunderthetermsoftheGNU+*GeneralPublicLicense(GPL)Version2,availablefromthefile+*COPYINGinthemaindirectoryofthissourcetree,orthe+*OpenIB.orgBSDlicensebelow:+*+*Redistributionanduseinsourceandbinaryforms,withor+*withoutmodification,arepermittedprovidedthatthefollowing+*conditionsaremet:+*+*-Redistributionsofsourcecodemustretaintheabove+*copyrightnotice,thislistofconditionsandthefollowing+*disclaimer.+*+*-Redistributionsinbinaryformmustreproducetheabove+*copyrightnotice,thislistofconditionsandthefollowing+*disclaimerinthedocumentationand/orothermaterials+*providedwiththedistribution.+*+*THESOFTWAREISPROVIDED"AS IS",WITHOUTWARRANTYOFANYKIND,+*EXPRESSORIMPLIED,INCLUDINGBUTNOTLIMITEDTOTHEWARRANTIESOF+*MERCHANTABILITY,FITNESSFORAPARTICULARPURPOSEAND+*NONINFRINGEMENT.INNOEVENTSHALLTHEAUTHORSORCOPYRIGHTHOLDERS+*BELIABLEFORANYCLAIM,DAMAGESOROTHERLIABILITY,WHETHERINAN+*ACTIONOFCONTRACT,TORTOROTHERWISE,ARISINGFROM,OUTOFORIN+*CONNECTIONWITHTHESOFTWAREORTHEUSEOROTHERDEALINGSINTHE+*SOFTWARE.+*/++#include<linux/pci.h>+#include"mlx5_core.h"+#include"pci_vsc.h"++#define MLX5_EXTRACT_C(source, offset, size) \+((((unsigned)(source))>>(offset))&MLX5_ONES32(size))+#define MLX5_EXTRACT(src, start, len) \+(((len)==32)?(src):MLX5_EXTRACT_C(src,start,len))+#define MLX5_ONES32(size) \+((size)?(0xffffffff>>(32-(size))):0)+#define MLX5_MASK32(offset, size) \+(MLX5_ONES32(size)<<(offset))+#define MLX5_MERGE_C(rsrc1, rsrc2, start, len) \+((((rsrc2)<<(start))&(MLX5_MASK32((start),(len))))|\+((rsrc1)&(~MLX5_MASK32((start),(len)))))+#define MLX5_MERGE(rsrc1, rsrc2, start, len) \+(((len)==32)?(rsrc2):MLX5_MERGE_C(rsrc1,rsrc2,start,len))++#define VSC_MAX_RETRIES 2048++enum{+MLX5_VSC_UNLOCK,+MLX5_VSC_LOCK,+};++enum{+VSC_CTRL_OFFSET=0x4,+VSC_COUNTER_OFFSET=0x8,+VSC_SEMAPHORE_OFFSET=0xc,+VSC_ADDR_OFFSET=0x10,+VSC_DATA_OFFSET=0x14,++VSC_FLAG_BIT_OFFS=31,+VSC_FLAG_BIT_LEN=1,++VSC_SYND_BIT_OFFS=30,+VSC_SYND_BIT_LEN=1,++VSC_ADDR_BIT_OFFS=0,+VSC_ADDR_BIT_LEN=30,++VSC_SPACE_BIT_OFFS=0,+VSC_SPACE_BIT_LEN=16,++VSC_SIZE_VLD_BIT_OFFS=28,+VSC_SIZE_VLD_BIT_LEN=1,++VSC_STATUS_BIT_OFFS=29,+VSC_STATUS_BIT_LEN=3,+};++intmlx5_vsc_init(structmlx5_core_dev*dev)+{+structmlx5_priv*priv=&dev->priv;++priv->health.vsc_addr=pci_find_capability(dev->pdev,+PCI_CAP_ID_VNDR);+if(!priv->health.vsc_addr)+mlx5_core_warn(dev,"Failed to get valid vendor specific ID\n");++return0;+}++intmlx5_vsc_gw_lock(structmlx5_core_dev*dev)+{+structmlx5_priv*priv=&dev->priv;+u32counter=0;+intretries=0;+u32lock_val;+intret;++do{+if(retries>VSC_MAX_RETRIES)+return-EBUSY;++/* Check if semaphore is already locked */+ret=pci_read_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_SEMAPHORE_OFFSET,+&lock_val);+if(ret)+returnret;++if(lock_val){+retries++;+usleep_range(1000,2000);+continue;+}++/*+*Readandwritecountervalue,ifwrittenvalueis+*thesame,semaphorewasacquiredsuccessfully.+*/+ret=pci_read_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_COUNTER_OFFSET,+&counter);+if(ret)+returnret;++ret=pci_write_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_SEMAPHORE_OFFSET,+counter);+if(ret)+returnret;++ret=pci_read_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_SEMAPHORE_OFFSET,+&lock_val);+if(ret)+returnret;++retries++;+}while(counter!=lock_val);++return0;+}++intmlx5_vsc_gw_unlock(structmlx5_core_dev*dev)+{+structmlx5_priv*priv=&dev->priv;+intret;++ret=pci_write_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_SEMAPHORE_OFFSET,+MLX5_VSC_UNLOCK);+returnret;+}++intmlx5_vsc_gw_set_space(structmlx5_core_dev*dev,u16space,+u32*ret_space_size)+{+structmlx5_priv*priv=&dev->priv;+intret;+u32val;++if(!mlx5_vsc_accessible(dev))+return-EINVAL;++if(ret_space_size)+*ret_space_size=0;++ret=pci_read_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_CTRL_OFFSET,+&val);+if(ret)+gotoout;++val=MLX5_MERGE(val,space,VSC_SPACE_BIT_OFFS,VSC_SPACE_BIT_LEN);+ret=pci_write_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_CTRL_OFFSET,+val);+if(ret)+gotoout;++ret=pci_read_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_CTRL_OFFSET,+&val);+if(ret)+gotoout;++if(MLX5_EXTRACT(val,VSC_STATUS_BIT_OFFS,VSC_STATUS_BIT_LEN)==0)+return-EINVAL;++/* Get space max address if indicated by size valid bit */+if(ret_space_size&&+MLX5_EXTRACT(val,VSC_SIZE_VLD_BIT_OFFS,VSC_SIZE_VLD_BIT_LEN)){+ret=pci_read_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_ADDR_OFFSET,+&val);+if(ret){+mlx5_core_warn(dev,"Failed to get max space size\n");+gotoout;+}+*ret_space_size=MLX5_EXTRACT(val,VSC_ADDR_BIT_OFFS,+VSC_ADDR_BIT_LEN);+}+return0;++out:+returnret;+}++staticintmlx5_vsc_wait_on_flag(structmlx5_core_dev*dev,u8expected_val)+{+structmlx5_priv*priv=&dev->priv;+intretries=0;+u32flag;+intret;++do{+if(retries>VSC_MAX_RETRIES)+return-EBUSY;++ret=pci_read_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_ADDR_OFFSET,+&flag);+flag=MLX5_EXTRACT(flag,VSC_FLAG_BIT_OFFS,VSC_FLAG_BIT_LEN);+retries++;++if((retries&0xf)==0)+usleep_range(1000,2000);++}while(flag!=expected_val);++return0;+}++staticintmlx5_vsc_gw_read(structmlx5_core_dev*dev,unsignedintaddress,+u32*data)+{+structmlx5_priv*priv=&dev->priv;+intret;++if(MLX5_EXTRACT(address,VSC_SYND_BIT_OFFS,+VSC_FLAG_BIT_LEN+VSC_SYND_BIT_LEN))+return-EINVAL;++ret=pci_write_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_ADDR_OFFSET,+address);+if(ret)+gotoout;++ret=mlx5_vsc_wait_on_flag(dev,1);+if(ret)+gotoout;++ret=pci_read_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_DATA_OFFSET,+data);+out:+returnret;+}++staticintmlx5_vsc_gw_read_fast(structmlx5_core_dev*dev,+unsignedintread_addr,+unsignedint*next_read_addr,+u32*data)+{+structmlx5_priv*priv=&dev->priv;+intret;++ret=mlx5_vsc_gw_read(dev,read_addr,data);+if(ret)+gotoout;++ret=pci_read_config_dword(dev->pdev,+priv->health.vsc_addr++VSC_ADDR_OFFSET,+next_read_addr);+if(ret)+gotoout;++*next_read_addr=MLX5_EXTRACT(*next_read_addr,VSC_ADDR_BIT_OFFS,+VSC_ADDR_BIT_LEN);++if(*next_read_addr<=read_addr)+ret=EINVAL;+out:+returnret;+}++intmlx5_vsc_gw_read_block_fast(structmlx5_core_dev*dev,u32*data,+intlength)+{+unsignedintnext_read_addr=0;+unsignedintread_addr=0;++while(read_addr<length){+if(mlx5_vsc_gw_read_fast(dev,read_addr,&next_read_addr,+&data[(read_addr>>2)]))+returnread_addr;++read_addr=next_read_addr;+}+returnlength;+}
From: Eran Ben Elisha <redacted>
Add support for two driver parameters via devlink params interface:
- Congestion action
HW mechanism in the PCIe buffer which monitors the amount of
consumed PCIe buffer per host. This mechanism supports the
following actions in case of threshold overflow:
- disabled - NOP (Default)
- drop
- mark - Mark CE bit in the CQE of received packet
- Congestion mode
- aggressive - Aggressive static trigger threshold (Default)
- dynamic - Dynamically change the trigger threshold
These driver-specific params enable the NIC HW workaround to handle
buffer congestion on the current NIC generation.
Signed-off-by: Eran Ben Elisha <redacted>
Reviewed-by: Moshe Shemesh <redacted>
Reviewed-by: Jiri Pirko <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
.../net/ethernet/mellanox/mlx5/core/devlink.c | 204 +++++++++++++++++-
1 file changed, 203 insertions(+), 1 deletion(-)
@@ -126,12 +126,214 @@ static int mlx5_devlink_query_tx_overflow_sense(struct mlx5_core_dev *mdev,return0;}+staticconstchar*constaction_to_str[]={+[MLX5_DEVLINK_CONGESTION_ACTION_DISABLED]="disabled",+[MLX5_DEVLINK_CONGESTION_ACTION_DROP]="drop",+[MLX5_DEVLINK_CONGESTION_ACTION_MARK]="mark"+};++staticconstchar*mlx5_devlink_congestion_action_to_str(intaction)+{+if(action>MLX5_DEVLINK_CONGESTION_ACTION_MAX){+WARN_ON(1);+returnERR_PTR(-EINVAL);+}++returnaction_to_str[action];+}++staticintmlx5_devlink_str_to_congestion_action(constchar*str,u8*action)+{+inti;++for(i=0;i<=MLX5_DEVLINK_CONGESTION_ACTION_MAX;i++){+if(!strcmp(str,action_to_str[i])){+*action=i;+return0;+}+}++return-EINVAL;+}++staticintmlx5_devlink_set_congestion_action(structdevlink*devlink,u32id,+structdevlink_param_gset_ctx*ctx,+structnetlink_ext_ack*extack)+{+structmlx5_core_dev*dev=devlink_priv(devlink);+u8max=MLX5_DEVLINK_CONGESTION_ACTION_MAX;+u8congestion_action;+u8sense;+interr;++if(!MLX5_CAP_MCAM_FEATURE(dev,mark_tx_action_cqe)&&+!MLX5_CAP_MCAM_FEATURE(dev,mark_tx_action_cnp))+max=MLX5_DEVLINK_CONGESTION_ACTION_MARK-1;++err=mlx5_devlink_str_to_congestion_action(ctx->val.vstr,+&congestion_action);+if(err)+returnerr;++if(congestion_action>max){+NL_SET_ERR_MSG(extack,"Requested congestion action is not supported on current device/FW");+return-EINVAL;+}++err=mlx5_devlink_query_tx_overflow_sense(dev,&sense);+if(err)+returnerr;++if(congestion_action==MLX5_DEVLINK_CONGESTION_ACTION_DISABLED&&+sense!=MLX5_DEVLINK_CONGESTION_MODE_AGGRESSIVE){+NL_SET_ERR_MSG(extack,"Congestion action \"disabled\" is allowed only while mode is configured to aggressive");+return-EINVAL;+}++returnmlx5_devlink_set_tx_lossy_overflow(dev,congestion_action);+}++staticintmlx5_devlink_get_congestion_action(structdevlink*devlink,u32id,+structdevlink_param_gset_ctx*ctx)+{+structmlx5_core_dev*dev=devlink_priv(devlink);+u8congestion_action;+constchar*val;+interr;++err=mlx5_devlink_query_tx_lossy_overflow(dev,&congestion_action);+if(err)+returnerr;++val=mlx5_devlink_congestion_action_to_str(congestion_action);+if(IS_ERR(val))+returnPTR_ERR(val);++devlink_param_value_str_fill(&ctx->val,val);+return0;+}++staticconstchar*constmode_to_str[]={+[MLX5_DEVLINK_CONGESTION_MODE_AGGRESSIVE]="aggressive",+[MLX5_DEVLINK_CONGESTION_MODE_DYNAMIC_ADJUSTMENT]="dynamic"+};++staticconstchar*mlx5_devlink_congestion_mode_to_str(intmode)+{+if(mode>MLX5_DEVLINK_CONGESTION_MODE_MAX){+WARN_ON(1);+returnERR_PTR(-EINVAL);+}++returnmode_to_str[mode];+}++staticintmlx5_devlink_str_to_congestion_mode(constchar*str,u8*mode)+{+inti;++for(i=0;i<=MLX5_DEVLINK_CONGESTION_MODE_MAX;i++){+if(!strcmp(str,mode_to_str[i])){+*mode=i;+return0;+}+}++return-EINVAL;+}++staticintmlx5_devlink_set_congestion_mode(structdevlink*devlink,u32id,+structdevlink_param_gset_ctx*ctx,+structnetlink_ext_ack*extack)+{+structmlx5_core_dev*dev=devlink_priv(devlink);+u8tx_lossy_overflow,congestion_mode;+interr;++err=mlx5_devlink_str_to_congestion_mode(ctx->val.vstr,+&congestion_mode);+if(err)+returnerr;++err=mlx5_devlink_query_tx_lossy_overflow(dev,&tx_lossy_overflow);+if(err)+returnerr;++if(congestion_mode!=MLX5_DEVLINK_CONGESTION_MODE_AGGRESSIVE&&+tx_lossy_overflow==MLX5_DEVLINK_CONGESTION_ACTION_DISABLED){+NL_SET_ERR_MSG(extack,"Congestion mode must be aggressive while congestion action is configured to \"disabled\"");+return-EINVAL;+}++returnmlx5_devlink_set_tx_overflow_sense(dev,congestion_mode);+}++staticintmlx5_devlink_get_congestion_mode(structdevlink*devlink,u32id,+structdevlink_param_gset_ctx*ctx)+{+structmlx5_core_dev*dev=devlink_priv(devlink);+u8congestion_mode;+constchar*val;+interr;++err=mlx5_devlink_query_tx_overflow_sense(dev,&congestion_mode);+if(err)+returnerr;++val=mlx5_devlink_congestion_mode_to_str(congestion_mode);+if(IS_ERR(val))+returnPTR_ERR(val);++devlink_param_value_str_fill(&ctx->val,val);+return0;+}++enummlx5_devlink_param_id{+MLX5_DEVLINK_PARAM_ID_BASE=DEVLINK_PARAM_GENERIC_ID_MAX,+MLX5_DEVLINK_PARAM_ID_CONGESTION_ACTION,+MLX5_DEVLINK_PARAM_ID_CONGESTION_MODE,+};++staticconststructdevlink_parammlx5_devlink_params[]={+DEVLINK_PARAM_DRIVER(MLX5_DEVLINK_PARAM_ID_CONGESTION_ACTION,+"congestion_action",+DEVLINK_PARAM_TYPE_STRING,+BIT(DEVLINK_PARAM_CMODE_RUNTIME),+mlx5_devlink_get_congestion_action,+mlx5_devlink_set_congestion_action,NULL),+DEVLINK_PARAM_DRIVER(MLX5_DEVLINK_PARAM_ID_CONGESTION_MODE,+"congestion_mode",+DEVLINK_PARAM_TYPE_STRING,+BIT(DEVLINK_PARAM_CMODE_RUNTIME),+mlx5_devlink_get_congestion_mode,+mlx5_devlink_set_congestion_mode,NULL),+};+intmlx5_devlink_register(structdevlink*devlink,structdevice*dev){-returndevlink_register(devlink,dev);+interr;++err=devlink_register(devlink,dev);+if(err)+returnerr;++err=devlink_params_register(devlink,mlx5_devlink_params,+ARRAY_SIZE(mlx5_devlink_params));+if(err){+dev_err(dev,"devlink_params_register failed\n");+gotounregister;+}++return0;++unregister:+devlink_unregister(devlink);+returnerr;}voidmlx5_devlink_unregister(structdevlink*devlink){+devlink_params_unregister(devlink,mlx5_devlink_params,+ARRAY_SIZE(mlx5_devlink_params));devlink_unregister(devlink);}
From: Alex Vesker <redacted>
Crdump allows the driver to create a snapshot of the FW PCI
crspace. This is useful in case of catastrophic issues which
require FW reset. The snapshot can be used for later debug.
The snapshot is exposed using devlink, cr-space
address regions are registered on init and snapshots are attached
once a new snapshot is collected by the driver.
Signed-off-by: Alex Vesker <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
.../net/ethernet/mellanox/mlx5/core/Makefile | 3 +-
.../ethernet/mellanox/mlx5/core/diag/crdump.c | 201 ++++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/health.c | 1 +
.../ethernet/mellanox/mlx5/core/lib/mlx5.h | 2 +
.../net/ethernet/mellanox/mlx5/core/main.c | 5 +
include/linux/mlx5/driver.h | 4 +
6 files changed, 215 insertions(+), 1 deletion(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/diag/crdump.c
@@ -0,0 +1,201 @@+/*+*Copyright(c)2018,MellanoxTechnologies.Allrightsreserved.+*+*Thissoftwareisavailabletoyouunderachoiceofoneoftwo+*licenses.YoumaychoosetobelicensedunderthetermsoftheGNU+*GeneralPublicLicense(GPL)Version2,availablefromthefile+*COPYINGinthemaindirectoryofthissourcetree,orthe+*OpenIB.orgBSDlicensebelow:+*+*Redistributionanduseinsourceandbinaryforms,withor+*withoutmodification,arepermittedprovidedthatthefollowing+*conditionsaremet:+*+*-Redistributionsofsourcecodemustretaintheabove+*copyrightnotice,thislistofconditionsandthefollowing+*disclaimer.+*+*-Redistributionsinbinaryformmustreproducetheabove+*copyrightnotice,thislistofconditionsandthefollowing+*disclaimerinthedocumentationand/orothermaterials+*providedwiththedistribution.+*+*THESOFTWAREISPROVIDED"AS IS",WITHOUTWARRANTYOFANYKIND,+*EXPRESSORIMPLIED,INCLUDINGBUTNOTLIMITEDTOTHEWARRANTIESOF+*MERCHANTABILITY,FITNESSFORAPARTICULARPURPOSEAND+*NONINFRINGEMENT.INNOEVENTSHALLTHEAUTHORSORCOPYRIGHTHOLDERS+*BELIABLEFORANYCLAIM,DAMAGESOROTHERLIABILITY,WHETHERINAN+*ACTIONOFCONTRACT,TORTOROTHERWISE,ARISINGFROM,OUTOFORIN+*CONNECTIONWITHTHESOFTWAREORTHEUSEOROTHERDEALINGSINTHE+*SOFTWARE.+*/++#include<linux/proc_fs.h>+#include<linux/mlx5/driver.h>+#include<net/devlink.h>+#include"mlx5_core.h"+#include"lib/pci_vsc.h"++#define BAD_ACCESS 0xBADACCE5+#define MLX5_PROTECTED_CR_SCAN_CRSPACE 0x7+#define MAX_NUM_OF_DUMPS_TO_STORE (8)++staticconstchar*region_cr_space_str="cr-space";++structmlx5_fw_crdump{+u32size;+structdevlink_region*region_crspace;+};++boolmlx5_crdump_enbaled(structmlx5_core_dev*dev)+{+structmlx5_priv*priv=&dev->priv;++return(!!priv->health.crdump);+}++staticintmlx5_crdump_fill(structmlx5_core_dev*dev)+{+structdevlink*devlink=priv_to_devlink(dev);+structmlx5_priv*priv=&dev->priv;+structmlx5_fw_crdump*crdump=priv->health.crdump;+inti,ret=0;+u32*cr_data;+u32id;++cr_data=kvmalloc(crdump->size,GFP_KERNEL);+if(!cr_data)+return-ENOMEM;++for(i=0;i<(crdump->size/4);i++)+cr_data[i]=BAD_ACCESS;++ret=mlx5_vsc_gw_read_block_fast(dev,cr_data,crdump->size);+if(ret<=0)+gotofree_data;++if(crdump->size!=ret){+mlx5_core_warn(dev,"failed to read full dump, read %d out of %u\n",+ret,crdump->size);+ret=-EINVAL;+gotofree_data;+}++/* Get the available snapshot ID for the dumps */+id=devlink_region_shapshot_id_get(devlink);+ret=devlink_region_snapshot_create(crdump->region_crspace,+crdump->size,(u8*)cr_data,+id,&kvfree);+if(ret){+mlx5_core_warn(dev,"crdump: devlink create %s snapshot id %d err %d\n",+region_cr_space_str,id,ret);+gotofree_data;+}else{+mlx5_core_info(dev,"crdump: added snapshot %d to devlink region %s\n",+id,region_cr_space_str);+}+return0;++free_data:+kvfree(cr_data);+returnret;+}++intmlx5_crdump_collect(structmlx5_core_dev*dev)+{+intret=0;++if(!mlx5_crdump_enbaled(dev))+return-ENODEV;++ret=mlx5_vsc_gw_lock(dev);+if(ret)+returnret;++ret=mlx5_vsc_gw_set_space(dev,MLX5_VSC_SPACE_SCAN_CRSPACE,NULL);+if(ret)+gotounlock;++ret=mlx5_crdump_fill(dev);+if(ret)+gotounlock;++unlock:+mlx5_vsc_gw_unlock(dev);+returnret;+}++intmlx5_crdump_init(structmlx5_core_dev*dev)+{+structdevlink*devlink=priv_to_devlink(dev);+structmlx5_priv*priv=&dev->priv;+structmlx5_fw_crdump*crdump;+u32space_size;+intret;++if(!mlx5_core_is_pf(dev)||!mlx5_vsc_accessible(dev)||+mlx5_crdump_enbaled(dev))+return0;++ret=mlx5_vsc_gw_lock(dev);+if(ret)+returnret;++/* Check if space is supported and get space size */+ret=mlx5_vsc_gw_set_space(dev,MLX5_VSC_SPACE_SCAN_CRSPACE,+&space_size);+if(ret){+/* Unlock and mask error since space is not supported */+mlx5_vsc_gw_unlock(dev);+return0;+}++if(space_size==0){+mlx5_core_warn(dev,"Invalid Crspace size, zero\n");+mlx5_vsc_gw_unlock(dev);+return-EINVAL;+}++ret=mlx5_vsc_gw_unlock(dev);+if(ret)+returnret;++crdump=kzalloc(sizeof(*crdump),GFP_KERNEL);+if(!crdump)+return-ENOMEM;++/* Create cr-space region */+crdump->size=space_size;+crdump->region_crspace=+devlink_region_create(devlink,+region_cr_space_str,+MAX_NUM_OF_DUMPS_TO_STORE,+space_size);+if(IS_ERR(crdump->region_crspace)){+mlx5_core_warn(dev,+"crdump: create devlink region %s err %ld\n",+region_cr_space_str,+PTR_ERR(crdump->region_crspace));+ret=PTR_ERR(crdump->region_crspace);+gotofree_crdump;+}+priv->health.crdump=crdump;+return0;++free_crdump:+kfree(crdump);+returnret;+}++voidmlx5_crdump_cleanup(structmlx5_core_dev*dev)+{+structmlx5_priv*priv=&dev->priv;+structmlx5_fw_crdump*crdump=priv->health.crdump;++if(!crdump)+return;++devlink_region_destroy(crdump->region_crspace);+kfree(crdump);+priv->health.crdump=NULL;+}
From: Alex Vesker <redacted>
This parameter enables capturing region snapshot of the crspace
during critical errors. The default value of this parameter is
disabled, it can be enabled using devlink param commands.
It is possible to configure during runtime and also driver init.
Signed-off-by: Alex Vesker <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
.../net/ethernet/mellanox/mlx5/core/devlink.c | 49 +++++++++++++++++++
.../ethernet/mellanox/mlx5/core/diag/crdump.c | 22 +++++++++
.../ethernet/mellanox/mlx5/core/lib/mlx5.h | 2 +
3 files changed, 73 insertions(+)
@@ -2,6 +2,7 @@/* Copyright (c) 2018, Mellanox Technologies inc. All rights reserved. */#include<devlink.h>+#include"lib/mlx5.h"enum{MLX5_DEVLINK_MPEGC_FIELD_SELECT_TX_OVERFLOW_DROP_EN=BIT(0),
@@ -41,5 +41,7 @@ int mlx5_core_reserved_gid_alloc(struct mlx5_core_dev *dev, int *gid_index);voidmlx5_core_reserved_gid_free(structmlx5_core_dev*dev,intgid_index);intmlx5_crdump_init(structmlx5_core_dev*dev);voidmlx5_crdump_cleanup(structmlx5_core_dev*dev);+boolmlx5_crdump_is_snapshot_enabled(structmlx5_core_dev*dev);+intmlx5_crdump_set_snapshot_enabled(structmlx5_core_dev*dev,boolvalue);#endif
From: Alexander Duyck <hidden> Date: 2018-08-02 00:06:19
On Wed, Aug 1, 2018 at 2:52 PM, Saeed Mahameed [off-list ref] wrote:
From: Eran Ben Elisha <redacted>
Add support for two driver parameters via devlink params interface:
- Congestion action
HW mechanism in the PCIe buffer which monitors the amount of
consumed PCIe buffer per host. This mechanism supports the
following actions in case of threshold overflow:
- disabled - NOP (Default)
- drop
- mark - Mark CE bit in the CQE of received packet
Any chance you could clarify the differences between "disabled" and
"drop"? I am assuming the "drop" is a head-of-line drop versus the
"disabled" being a incoming packet drop?
Also I still don't see this as necessarily being all that unique of a
feature/issue. Basically being PCIe bus limited is not all that
uncommon of a thing and has existed since the early days of PCI. In
the case of the Intel NICs we just throw a warning and end up dropping
the incoming packets instead of providing the two other options you
have listed.
- Congestion mode
- aggressive - Aggressive static trigger threshold (Default)
- dynamic - Dynamically change the trigger threshold
These driver-specific params enable the NIC HW workaround to handle
buffer congestion on the current NIC generation.
Is there any documentation anywhere for any of these features? In the
patch set I see you adding interfaces, but I don't see them documented
anywhere.
Ideally there should be some documentation going into the kernel when
you extend the devlink interface at least so that I know how to use
your new interfaces when you define them. Just updating devlink.c
seems like a messy way to do things.
- Alex
From: Jakub Kicinski <hidden> Date: 2018-08-02 00:21:42
On Wed, 1 Aug 2018 14:52:46 -0700, Saeed Mahameed wrote:
quoted hunk
From: Moshe Shemesh <redacted>
In case devlink param type is string, it needs to copy the string value
it got from the input to devlink_param_value.
Fixes: e3b7ca18ad7b ("devlink: Add param set command")
Signed-off-by: Moshe Shemesh <redacted>
Acked-by: Jiri Pirko <redacted>
Signed-off-by: Saeed Mahameed <redacted>
---
include/net/devlink.h | 2 +-
net/core/devlink.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
From: Alexander Duyck <hidden> Date: 2018-08-02 00:22:11
On Wed, Aug 1, 2018 at 2:52 PM, Saeed Mahameed [off-list ref] wrote:
Hi Dave,
This series provides devlink parameters updates to both devlink API and
mlx5 driver, it is a 2nd iteration of the dropped patches sent in a previous
mlx5 submission "net/mlx5: Support PCIe buffer congestion handling via
Devlink" to address review comments [1].
Changes from the original series:
- According to the discussion outcome, we are keeping the congestion control
setting as mlx5 device specific for the current HW generation.
- Changed the congestion_mode and congestion action param type to string
- Added patches to fix devlink handling of param type string
- Added a patch which adds extack messages support for param set.
- At the end of this series, I've added yet another mlx5 devlink related
feature, firmware snapshot support.
For more information please see tag log below.
Please pull and let me know if there's any problem.
[1] https://patchwork.ozlabs.org/patch/945996/
Thanks,
Saeed.
---
The following changes since commit e6476c21447c4b17c47e476aade6facf050f31e8:
net: remove bogus RCU annotations on socket.wq (2018-07-31 12:40:22 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git tags/mlx5-updates-2018-08-01
for you to fetch changes up to 2ac6108c65ffcb1e5eab1fba1fd59272604d1c32:
net/mlx5: Use devlink region_snapshot parameter (2018-08-01 14:49:09 -0700)
----------------------------------------------------------------
mlx5-updates-2018-08-01
This series provides devlink parameters updates to both devlink API and
mlx5 driver,
1) Devlink changes: (Moshe Shemesh)
The first two patches fix devlink param infrastructure for string type
params.
The third patch adds a devlink helper function to safely copy string from
driver to devlink.
The forth patch adds extack support for param set.
2) mlx5 specific congestion parameters: (Eran Ben Elisha)
Next three patches add new devlink driver specific params for controlling
congestion action and mode, using string type params and extack messages support.
This congestion mode enables hw workaround in specific devices which is
controlled by devlink driver-specific params. The workaround is device
specific for this NIC generation, the next NIC will not need it.
Congestion parameters:
- Congestion action
HW W/A mechanism in the PCIe buffer which monitors the amount of
consumed PCIe buffer per host. This mechanism supports the
following actions in case of threshold overflow:
- Disabled - NOP (Default)
- Drop
- Mark - Mark CE bit in the CQE of received packet
- Congestion mode
- Aggressive - Aggressive static trigger threshold (Default)
- Dynamic - Dynamically change the trigger threshold
3) mlx5 firmware snapshot support via devlink: (Alex Vesker)
Last three patches, add the support for capturing region snapshot of the
firmware crspace during critical errors, using devlink region_snapshot
parameter.
-Saeed.
----------------------------------------------------------------
Alex Vesker (3):
net/mlx5: Add Vendor Specific Capability access gateway
net/mlx5: Add Crdump FW snapshot support
net/mlx5: Use devlink region_snapshot parameter
Eran Ben Elisha (3):
net/mlx5: Move all devlink related functions calls to devlink.c
net/mlx5: Add MPEGC register configuration functionality
net/mlx5: Enable PCIe buffer congestion handling workaround via devlink
Moshe Shemesh (4):
devlink: Fix param set handling for string type
devlink: Fix param cmode driverinit for string type
devlink: Add helper function for safely copy string param
devlink: Add extack messages support to param set
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 3 +-
drivers/net/ethernet/mellanox/mlx4/main.c | 6 +-
drivers/net/ethernet/mellanox/mlx5/core/Makefile | 3 +-
drivers/net/ethernet/mellanox/mlx5/core/devlink.c | 388 +++++++++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/devlink.h | 13 +
.../net/ethernet/mellanox/mlx5/core/diag/crdump.c | 223 ++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/health.c | 3 +
drivers/net/ethernet/mellanox/mlx5/core/lib/mlx5.h | 4 +
.../net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c | 320 +++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h | 56 +++
drivers/net/ethernet/mellanox/mlx5/core/main.c | 10 +-
include/linux/mlx5/driver.h | 5 +
include/net/devlink.h | 15 +-
net/core/devlink.c | 44 ++-
14 files changed, 1076 insertions(+), 17 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.h
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/diag/crdump.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h
So after looking over the patch set the one thing I would ask for in
this is some sort of documentation at a minimum. As a user I don't see
how you can expect someone to be able to use this when the naming of
things are pretty cryptic and there is no real explanation anywhere if
you don't go through and read the patch description itself. When you
start adding driver specific interfaces, you should at least start
adding vendor specific documentation.
Also I don't see how using a vendor specific configuration space
section can be done without adding some tie-ins to the PCI core files
because it should be possible to race with someone poking at the
register space via something like setpci/lspci. Also one of the things
that came up was that drivers are not supposed to be banging on the
PCI configuration space at will, and it seems like this patch set is
doing exactly that through the VSC block.
- Alex
On Wed, Aug 1, 2018 at 3:34 PM, Alexander Duyck
[off-list ref] wrote:
On Wed, Aug 1, 2018 at 2:52 PM, Saeed Mahameed [off-list ref] wrote:
quoted
Hi Dave,
This series provides devlink parameters updates to both devlink API and
mlx5 driver, it is a 2nd iteration of the dropped patches sent in a previous
mlx5 submission "net/mlx5: Support PCIe buffer congestion handling via
Devlink" to address review comments [1].
Changes from the original series:
- According to the discussion outcome, we are keeping the congestion control
setting as mlx5 device specific for the current HW generation.
- Changed the congestion_mode and congestion action param type to string
- Added patches to fix devlink handling of param type string
- Added a patch which adds extack messages support for param set.
- At the end of this series, I've added yet another mlx5 devlink related
feature, firmware snapshot support.
For more information please see tag log below.
Please pull and let me know if there's any problem.
[1] https://patchwork.ozlabs.org/patch/945996/
Thanks,
Saeed.
---
The following changes since commit e6476c21447c4b17c47e476aade6facf050f31e8:
net: remove bogus RCU annotations on socket.wq (2018-07-31 12:40:22 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git tags/mlx5-updates-2018-08-01
for you to fetch changes up to 2ac6108c65ffcb1e5eab1fba1fd59272604d1c32:
net/mlx5: Use devlink region_snapshot parameter (2018-08-01 14:49:09 -0700)
----------------------------------------------------------------
mlx5-updates-2018-08-01
This series provides devlink parameters updates to both devlink API and
mlx5 driver,
1) Devlink changes: (Moshe Shemesh)
The first two patches fix devlink param infrastructure for string type
params.
The third patch adds a devlink helper function to safely copy string from
driver to devlink.
The forth patch adds extack support for param set.
2) mlx5 specific congestion parameters: (Eran Ben Elisha)
Next three patches add new devlink driver specific params for controlling
congestion action and mode, using string type params and extack messages support.
This congestion mode enables hw workaround in specific devices which is
controlled by devlink driver-specific params. The workaround is device
specific for this NIC generation, the next NIC will not need it.
Congestion parameters:
- Congestion action
HW W/A mechanism in the PCIe buffer which monitors the amount of
consumed PCIe buffer per host. This mechanism supports the
following actions in case of threshold overflow:
- Disabled - NOP (Default)
- Drop
- Mark - Mark CE bit in the CQE of received packet
- Congestion mode
- Aggressive - Aggressive static trigger threshold (Default)
- Dynamic - Dynamically change the trigger threshold
3) mlx5 firmware snapshot support via devlink: (Alex Vesker)
Last three patches, add the support for capturing region snapshot of the
firmware crspace during critical errors, using devlink region_snapshot
parameter.
-Saeed.
----------------------------------------------------------------
Alex Vesker (3):
net/mlx5: Add Vendor Specific Capability access gateway
net/mlx5: Add Crdump FW snapshot support
net/mlx5: Use devlink region_snapshot parameter
Eran Ben Elisha (3):
net/mlx5: Move all devlink related functions calls to devlink.c
net/mlx5: Add MPEGC register configuration functionality
net/mlx5: Enable PCIe buffer congestion handling workaround via devlink
Moshe Shemesh (4):
devlink: Fix param set handling for string type
devlink: Fix param cmode driverinit for string type
devlink: Add helper function for safely copy string param
devlink: Add extack messages support to param set
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 3 +-
drivers/net/ethernet/mellanox/mlx4/main.c | 6 +-
drivers/net/ethernet/mellanox/mlx5/core/Makefile | 3 +-
drivers/net/ethernet/mellanox/mlx5/core/devlink.c | 388 +++++++++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/devlink.h | 13 +
.../net/ethernet/mellanox/mlx5/core/diag/crdump.c | 223 ++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/health.c | 3 +
drivers/net/ethernet/mellanox/mlx5/core/lib/mlx5.h | 4 +
.../net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c | 320 +++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h | 56 +++
drivers/net/ethernet/mellanox/mlx5/core/main.c | 10 +-
include/linux/mlx5/driver.h | 5 +
include/net/devlink.h | 15 +-
net/core/devlink.c | 44 ++-
14 files changed, 1076 insertions(+), 17 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.h
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/diag/crdump.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h
So after looking over the patch set the one thing I would ask for in
this is some sort of documentation at a minimum. As a user I don't see
how you can expect someone to be able to use this when the naming of
things are pretty cryptic and there is no real explanation anywhere if
you don't go through and read the patch description itself. When you
start adding driver specific interfaces, you should at least start
adding vendor specific documentation.
Sure, sounds like a great idea, something like:
Documentation/networking/mlx5.txt and have a devlink section ?
or have a generic devlink doc and a mlx5 section in it ?
Also I don't see how using a vendor specific configuration space
section can be done without adding some tie-ins to the PCI core files
because it should be possible to race with someone poking at the
register space via something like setpci/lspci. Also one of the things
that came up was that drivers are not supposed to be banging on the
PCI configuration space at will, and it seems like this patch set is
doing exactly that through the VSC block.
this is a whole different feature than the device specific parameters.
The whole vendor specific configuration space access is needed only
for diagnostic/dump
purposes when something really bad happens and the command interface
with FW is down,
and when the FW is un-responsive, we want to dump the crspace into the
already existing devlink
crdump buffer, how do you expect us to read it if we are not allowed
to access it ?
What do you mean by tie-ins to the PCI core files ? can you please elaborate ?
From: Jakub Kicinski <hidden> Date: 2018-08-02 01:49:11
On Wed, 1 Aug 2018 14:52:45 -0700, Saeed Mahameed wrote:
- According to the discussion outcome, we are keeping the congestion control
setting as mlx5 device specific for the current HW generation.
I still see queuing and marking based on queue level. You want to add
a Qdisc that will mirror your HW's behaviour to offload, if you really
believe this is not a subset of RED, why not... But devlink params?
From: Alexander Duyck <hidden> Date: 2018-08-02 02:25:09
On Wed, Aug 1, 2018 at 4:13 PM, Saeed Mahameed
[off-list ref] wrote:
On Wed, Aug 1, 2018 at 3:34 PM, Alexander Duyck
[off-list ref] wrote:
quoted
On Wed, Aug 1, 2018 at 2:52 PM, Saeed Mahameed [off-list ref] wrote:
quoted
Hi Dave,
This series provides devlink parameters updates to both devlink API and
mlx5 driver, it is a 2nd iteration of the dropped patches sent in a previous
mlx5 submission "net/mlx5: Support PCIe buffer congestion handling via
Devlink" to address review comments [1].
Changes from the original series:
- According to the discussion outcome, we are keeping the congestion control
setting as mlx5 device specific for the current HW generation.
- Changed the congestion_mode and congestion action param type to string
- Added patches to fix devlink handling of param type string
- Added a patch which adds extack messages support for param set.
- At the end of this series, I've added yet another mlx5 devlink related
feature, firmware snapshot support.
For more information please see tag log below.
Please pull and let me know if there's any problem.
[1] https://patchwork.ozlabs.org/patch/945996/
Thanks,
Saeed.
---
The following changes since commit e6476c21447c4b17c47e476aade6facf050f31e8:
net: remove bogus RCU annotations on socket.wq (2018-07-31 12:40:22 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/saeed/linux.git tags/mlx5-updates-2018-08-01
for you to fetch changes up to 2ac6108c65ffcb1e5eab1fba1fd59272604d1c32:
net/mlx5: Use devlink region_snapshot parameter (2018-08-01 14:49:09 -0700)
----------------------------------------------------------------
mlx5-updates-2018-08-01
This series provides devlink parameters updates to both devlink API and
mlx5 driver,
1) Devlink changes: (Moshe Shemesh)
The first two patches fix devlink param infrastructure for string type
params.
The third patch adds a devlink helper function to safely copy string from
driver to devlink.
The forth patch adds extack support for param set.
2) mlx5 specific congestion parameters: (Eran Ben Elisha)
Next three patches add new devlink driver specific params for controlling
congestion action and mode, using string type params and extack messages support.
This congestion mode enables hw workaround in specific devices which is
controlled by devlink driver-specific params. The workaround is device
specific for this NIC generation, the next NIC will not need it.
Congestion parameters:
- Congestion action
HW W/A mechanism in the PCIe buffer which monitors the amount of
consumed PCIe buffer per host. This mechanism supports the
following actions in case of threshold overflow:
- Disabled - NOP (Default)
- Drop
- Mark - Mark CE bit in the CQE of received packet
- Congestion mode
- Aggressive - Aggressive static trigger threshold (Default)
- Dynamic - Dynamically change the trigger threshold
3) mlx5 firmware snapshot support via devlink: (Alex Vesker)
Last three patches, add the support for capturing region snapshot of the
firmware crspace during critical errors, using devlink region_snapshot
parameter.
-Saeed.
----------------------------------------------------------------
Alex Vesker (3):
net/mlx5: Add Vendor Specific Capability access gateway
net/mlx5: Add Crdump FW snapshot support
net/mlx5: Use devlink region_snapshot parameter
Eran Ben Elisha (3):
net/mlx5: Move all devlink related functions calls to devlink.c
net/mlx5: Add MPEGC register configuration functionality
net/mlx5: Enable PCIe buffer congestion handling workaround via devlink
Moshe Shemesh (4):
devlink: Fix param set handling for string type
devlink: Fix param cmode driverinit for string type
devlink: Add helper function for safely copy string param
devlink: Add extack messages support to param set
drivers/net/ethernet/broadcom/bnxt/bnxt_devlink.c | 3 +-
drivers/net/ethernet/mellanox/mlx4/main.c | 6 +-
drivers/net/ethernet/mellanox/mlx5/core/Makefile | 3 +-
drivers/net/ethernet/mellanox/mlx5/core/devlink.c | 388 +++++++++++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/devlink.h | 13 +
.../net/ethernet/mellanox/mlx5/core/diag/crdump.c | 223 ++++++++++++
drivers/net/ethernet/mellanox/mlx5/core/health.c | 3 +
drivers/net/ethernet/mellanox/mlx5/core/lib/mlx5.h | 4 +
.../net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c | 320 +++++++++++++++++
.../net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h | 56 +++
drivers/net/ethernet/mellanox/mlx5/core/main.c | 10 +-
include/linux/mlx5/driver.h | 5 +
include/net/devlink.h | 15 +-
net/core/devlink.c | 44 ++-
14 files changed, 1076 insertions(+), 17 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/devlink.h
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/diag/crdump.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/pci_vsc.h
So after looking over the patch set the one thing I would ask for in
this is some sort of documentation at a minimum. As a user I don't see
how you can expect someone to be able to use this when the naming of
things are pretty cryptic and there is no real explanation anywhere if
you don't go through and read the patch description itself. When you
start adding driver specific interfaces, you should at least start
adding vendor specific documentation.
Sure, sounds like a great idea, something like:
Documentation/networking/mlx5.txt and have a devlink section ?
or have a generic devlink doc and a mlx5 section in it ?
Either would work for me.
quoted
Also I don't see how using a vendor specific configuration space
section can be done without adding some tie-ins to the PCI core files
because it should be possible to race with someone poking at the
register space via something like setpci/lspci. Also one of the things
that came up was that drivers are not supposed to be banging on the
PCI configuration space at will, and it seems like this patch set is
doing exactly that through the VSC block.
this is a whole different feature than the device specific parameters.
The whole vendor specific configuration space access is needed only
for diagnostic/dump
purposes when something really bad happens and the command interface
with FW is down,
and when the FW is un-responsive, we want to dump the crspace into the
already existing devlink
crdump buffer, how do you expect us to read it if we are not allowed
to access it ?
What do you mean by tie-ins to the PCI core files ? can you please elaborate ?
You have added a vendor specific config section and you are using it
to access several of the pieces of metadata. The setup isn't too
different than the VPD setup and approach. However I don't see many of
the protections that exist for VPD in place for this vendor specific
configuration. As such I have concerns. For example what is to keep
requests to the various devlink interfaces from racing with each other
when they both end up operating through the VCS?
- Alex
From: David Miller <davem@davemloft.net> Date: 2018-08-02 03:29:18
From: Jakub Kicinski <redacted>
Date: Wed, 1 Aug 2018 17:00:47 -0700
On Wed, 1 Aug 2018 14:52:45 -0700, Saeed Mahameed wrote:
quoted
- According to the discussion outcome, we are keeping the congestion control
setting as mlx5 device specific for the current HW generation.
I still see queuing and marking based on queue level. You want to add
a Qdisc that will mirror your HW's behaviour to offload, if you really
believe this is not a subset of RED, why not... But devlink params?
I totally agree, devlink seems like absolutely to wrong level and set
of interfaces to be doing this stuff.
I will not pull these changes in and I probably should have not
accepted the DCB changes from the other day and they were sneakily
leading up to this crap.
Sorry, please follow Jakub's lead as I think his approach makes much
more technical sense than using devlink for this.
Thanks.
Thu, Aug 02, 2018 at 01:13:20AM CEST, saeedm@dev.mellanox.co.il wrote:
[...]
quoted
So after looking over the patch set the one thing I would ask for in
this is some sort of documentation at a minimum. As a user I don't see
how you can expect someone to be able to use this when the naming of
things are pretty cryptic and there is no real explanation anywhere if
you don't go through and read the patch description itself. When you
start adding driver specific interfaces, you should at least start
adding vendor specific documentation.
Sure, sounds like a great idea, something like:
Documentation/networking/mlx5.txt and have a devlink section ?
or have a generic devlink doc and a mlx5 section in it ?
I think that Documentation/networking/devlink/mlx5.txt would be good.
We can have generic param description in:
Documentation/networking/devlink/generic.txt
From: Petr Machata <hidden> Date: 2018-08-02 10:19:37
David Miller [off-list ref] writes:
From: Jakub Kicinski <redacted>
Date: Wed, 1 Aug 2018 17:00:47 -0700
quoted
On Wed, 1 Aug 2018 14:52:45 -0700, Saeed Mahameed wrote:
quoted
- According to the discussion outcome, we are keeping the congestion control
setting as mlx5 device specific for the current HW generation.
I still see queuing and marking based on queue level. You want to add
a Qdisc that will mirror your HW's behaviour to offload, if you really
believe this is not a subset of RED, why not... But devlink params?
I totally agree, devlink seems like absolutely to wrong level and set
of interfaces to be doing this stuff.
I will not pull these changes in and I probably should have not
accepted the DCB changes from the other day and they were sneakily
leading up to this crap.
Are you talking about the recent additions of DCB helpers
dcb_ieee_getapp_prio_dscp_mask_map() etc.?
If yes, I can assure there were no sneaky intentions at all. I'm at a
loss to understand the relation to mlx5 team's decision to use devlink
for congestion control configuration.
Could you please clarify your remark?
Thanks,
Petr
From: Eran Ben Elisha <hidden> Date: 2018-08-02 16:59:04
On 8/2/2018 4:40 AM, David Miller wrote:
From: Jakub Kicinski <redacted>
Date: Wed, 1 Aug 2018 17:00:47 -0700
quoted
On Wed, 1 Aug 2018 14:52:45 -0700, Saeed Mahameed wrote:
quoted
- According to the discussion outcome, we are keeping the congestion control
setting as mlx5 device specific for the current HW generation.
I still see queuing and marking based on queue level. You want to add
a Qdisc that will mirror your HW's behaviour to offload, if you really
believe this is not a subset of RED, why not... But devlink params?
I totally agree, devlink seems like absolutely to wrong level and set
of interfaces to be doing this stuff.
I will not pull these changes in and I probably should have not
accepted the DCB changes from the other day and they were sneakily
leading up to this crap.
Sorry, please follow Jakub's lead as I think his approach makes much
more technical sense than using devlink for this.
Thanks.
Hi Dave,
I would like to re-state that this feature was not meant to be a generic
one. This feature was added in order to resolve a HW bug which exist in
a small portion of our devices. Those params will be used only on those
current HWs and won't be in use for our future devices.
During the discussions, several alternatives where offered to be used by
various members of the community. These alternatives includes TC and
enhancements to PCI configuration tools.
Regarding the TC, from my perspective, this is not an option as:
1) The HW mechanism handles multiple functions and therefore cannot be
configured on as a regular TC
2) No PF + representors modeling can be applied here, this is a
MultiHost environment where one host is not aware to the other hosts,
and each is running on its own pci/driver. It is a device working mode
configuration.
3) The current HW W/A is very limited, maybe it has a similar algorithm
as WRED, but is being used for much simpler different use case (pci bus
congestion). It cannot be compared to a standard TC capability
(RED/WRED), and defining it as a offload fully controlled by the user
will be a big misuse. (for example, drop rate cannot be configured)
regarding the PCI config tools, there was a consensus that such tool is
not acceptable as it is not a part of the PCI spec.
Since module param/sysfs/debugfs/etc are no longer acceptable, and
current drivers still desired with a way to do some configurations to
the device/driver which cannot used standard Linux tool or by other
vendors, devlink params was developed (under the assumption that this
tool will be helpful for those needs, and those only).
From my perspective, Devlink is the tool to configure the device for
handling such unexpected bugs, i.e "PCIe buffer congestion handling
workaround".
Thanks,
Eran
From: Jakub Kicinski <hidden> Date: 2018-08-02 19:03:17
On Thu, 02 Aug 2018 11:29:12 +0300, Petr Machata wrote:
David Miller [off-list ref] writes:
quoted
From: Jakub Kicinski <redacted>
Date: Wed, 1 Aug 2018 17:00:47 -0700
quoted
On Wed, 1 Aug 2018 14:52:45 -0700, Saeed Mahameed wrote:
quoted
- According to the discussion outcome, we are keeping the congestion control
setting as mlx5 device specific for the current HW generation.
I still see queuing and marking based on queue level. You want to add
a Qdisc that will mirror your HW's behaviour to offload, if you really
believe this is not a subset of RED, why not... But devlink params?
I totally agree, devlink seems like absolutely to wrong level and set
of interfaces to be doing this stuff.
I will not pull these changes in and I probably should have not
accepted the DCB changes from the other day and they were sneakily
leading up to this crap.
Are you talking about the recent additions of DCB helpers
dcb_ieee_getapp_prio_dscp_mask_map() etc.?
If yes, I can assure there were no sneaky intentions at all. I'm at a
loss to understand the relation to mlx5 team's decision to use devlink
for congestion control configuration.
Could you please clarify your remark?
Oh, I think David meant the patches I was objecting to a while ago,
which were doing buffer configuration via the DCB API.
From: Jakub Kicinski <hidden> Date: 2018-08-03 00:46:38
On Thu, 2 Aug 2018 18:07:18 +0300, Eran Ben Elisha wrote:
On 8/2/2018 4:40 AM, David Miller wrote:
quoted
From: Jakub Kicinski <redacted>
Date: Wed, 1 Aug 2018 17:00:47 -0700
quoted
On Wed, 1 Aug 2018 14:52:45 -0700, Saeed Mahameed wrote:
quoted
- According to the discussion outcome, we are keeping the congestion control
setting as mlx5 device specific for the current HW generation.
I still see queuing and marking based on queue level. You want to add
a Qdisc that will mirror your HW's behaviour to offload, if you really
believe this is not a subset of RED, why not... But devlink params?
I totally agree, devlink seems like absolutely to wrong level and set
of interfaces to be doing this stuff.
I will not pull these changes in and I probably should have not
accepted the DCB changes from the other day and they were sneakily
leading up to this crap.
Sorry, please follow Jakub's lead as I think his approach makes much
more technical sense than using devlink for this.
Thanks.
Hi Dave,
I would like to re-state that this feature was not meant to be a generic
one. This feature was added in order to resolve a HW bug which exist in
a small portion of our devices.
Would you mind describing the HW bug in more detail? To a outside
reviewer it really looks like you're adding a feature. What are you
working around? Is the lack of full AQM on the PCIe side of the chip
considered a bug?
Those params will be used only on those current HWs and won't be in
use for our future devices.
I'm glad that is your plan today, however, customers may get used to
the simple interface you're adding now. This means the API you are
adding is effectively becoming an API other drivers may need to
implement to keep compatibility with someone's proprietary
orchestration.
During the discussions, several alternatives where offered to be used by
various members of the community. These alternatives includes TC and
enhancements to PCI configuration tools.
Regarding the TC, from my perspective, this is not an option as:
1) The HW mechanism handles multiple functions and therefore cannot be
configured on as a regular TC
Could you elaborate? What are the multiple functions? You seem to be
adding a knob to enable ECN marking and a knob for choosing between
some predefined slopes.
In what way would your solution not behave like a RED offload?
With TC offload you'd also get a well-defined set of statistics, I
presume right now you're planning on adding a set of ethtool -S
counters?
2) No PF + representors modeling can be applied here, this is a
MultiHost environment where one host is not aware to the other hosts,
and each is running on its own pci/driver. It is a device working mode
configuration.
Yes, the multihost part makes it less pleasant. But this is a problem
we have to tackle separately, at some point. It's not a center of
attention here.
3) The current HW W/A is very limited, maybe it has a similar algorithm
as WRED, but is being used for much simpler different use case (pci bus
congestion).
No one is requesting full RED offload here.. if someone sets the
parameters you can't support you simply won't offload them. And ignore
the parameters which only make sense in software terms. Look at the
docs for mlxsw:
https://github.com/Mellanox/mlxsw/wiki/Queues-Management#offloading-red
It says "not offloaded" in a number of places.
It cannot be compared to a standard TC capability (RED/WRED), and
defining it as a offload fully controlled by the user will be a big
misuse.
It's generally preferable to implement a subset of exiting well defined
API than create vendor knobs, hence hardly a misuse.
(for example, drop rate cannot be configured)
I don't know what "configuring drop rate" means in case of RED..
regarding the PCI config tools, there was a consensus that such tool is
not acceptable as it is not a part of the PCI spec.
As I said, this has nothing to do with PCI being the transport. The
port you're running over could be serial, SPI or anything else. You
have congestion on a port of a device, that's a networking problem.
Since module param/sysfs/debugfs/etc are no longer acceptable, and
current drivers still desired with a way to do some configurations to
the device/driver which cannot used standard Linux tool or by other
vendors, devlink params was developed (under the assumption that this
tool will be helpful for those needs, and those only).
From my perspective, Devlink is the tool to configure the device for
handling such unexpected bugs, i.e "PCIe buffer congestion handling
workaround".
Hm. Are you calling it a bug because you had to work around silicon
limitation in firmware? Hm. I'm very intrigued by the framing :)
On Thu, Aug 02, 2018 at 03:53:15PM -0700, Jakub Kicinski wrote:
No one is requesting full RED offload here.. if someone sets the
parameters you can't support you simply won't offload them. And ignore
the parameters which only make sense in software terms. Look at the
docs for mlxsw:
https://github.com/Mellanox/mlxsw/wiki/Queues-Management#offloading-red
It says "not offloaded" in a number of places.
...
It's generally preferable to implement a subset of exiting well defined
API than create vendor knobs, hence hardly a misuse.
Sorry for derailing the discussion, but you mentioned some points that
have been bothering me for a while.
I think we didn't do a very good job with buffer management and this is
exactly why you see some parameters marked as "not offloaded". Take the
"limit" (queue size) for example. It's configured via devlink-sb, by
setting a quota on the number of bytes that can be queued for the port
and TC (queue) that RED manages. See:
https://github.com/Mellanox/mlxsw/wiki/Quality-of-Service#pool-binding
It would have been much better and user friendly to not ignore this
parameter and have users configure the limit using existing interfaces
(tc), instead of creating a discrepancy between the software and
hardware data paths by configuring the hardware directly via devlink-sb.
I believe devlink-sb is mainly the result of Linux's short comings in
this area and our lack of perspective back then. While the qdisc layer
(Linux's shared buffers) works for end hosts, it requires enhancements
(mainly on ingress) for switches (physical/virtual) that forward
packets.
For example, switches (I'm familiar with Mellanox ASICs, but I assume
the concept is similar in other ASICs) have ingress buffers where
packets are stored while going through the pipeline. Once out of the
pipeline you know from which port and queue the packet should egress. In
case you have both lossless and lossy traffic in your network you
probably want to classify it into different ingress buffers and mark the
buffers where the lossless traffic is stored as such, so that PFC frames
would be emitted above a certain threshold.
This is currently configured using dcbnl, but it lacks a software model
which means that packets that are forwarded by the kernel don't get the
same treatment (e.g., skb priority isn't set). It also means that when
you want to limit the number of packets that are queued *from* a certain
port and ingress buffer you resort to tools such as devlink-sb that end
up colliding with existing tools (tc).
I was thinking (not too much...) about modelling the above using ingress
qdiscs. They don't do any queueing, but more of accounting. Once the
egress qdisc dequeues the packet, you give credit back to the ingress
qdisc from which the packet came from. I believe that modelling these
buffers using the qdisc layer is the right abstraction.
Would appreciate hearing your thoughts on the above.
From: Jakub Kicinski <hidden> Date: 2018-08-04 06:58:32
On Fri, 3 Aug 2018 19:41:50 +0300, Ido Schimmel wrote:
On Thu, Aug 02, 2018 at 03:53:15PM -0700, Jakub Kicinski wrote:
quoted
No one is requesting full RED offload here.. if someone sets the
parameters you can't support you simply won't offload them. And ignore
the parameters which only make sense in software terms. Look at the
docs for mlxsw:
https://github.com/Mellanox/mlxsw/wiki/Queues-Management#offloading-red
It says "not offloaded" in a number of places.
...
quoted
It's generally preferable to implement a subset of exiting well defined
API than create vendor knobs, hence hardly a misuse.
Sorry for derailing the discussion, but you mentioned some points that
have been bothering me for a while.
I think we didn't do a very good job with buffer management and this is
exactly why you see some parameters marked as "not offloaded". Take the
"limit" (queue size) for example. It's configured via devlink-sb, by
setting a quota on the number of bytes that can be queued for the port
and TC (queue) that RED manages. See:
https://github.com/Mellanox/mlxsw/wiki/Quality-of-Service#pool-binding
FWIW I was implementing a very similar thing for the NFP a while back.
devlink-sb to configure per-port limits and RED offload. I believe we
have some more qdisc offloads but out-of-tree/for appliances.
"Switchdev mode" + qdisc offloads work quite well. For RED I think
we also don't offload the limit.
It would have been much better and user friendly to not ignore this
parameter and have users configure the limit using existing interfaces
(tc), instead of creating a discrepancy between the software and
hardware data paths by configuring the hardware directly via devlink-sb.
I believe devlink-sb is mainly the result of Linux's short comings in
this area and our lack of perspective back then. While the qdisc layer
(Linux's shared buffers) works for end hosts, it requires enhancements
(mainly on ingress) for switches (physical/virtual) that forward
packets.
I could definitely agree with you. But there is another way to look at
this. Memory in ASICs is fundamentally more precious. If the problem
was never solved for Linux (placing constraints on the number of
packets in the system by ingress port) maybe it's just not important
for software stacks? Qdiscs are focused on egress. Perhaps a better
software equivalent to Shared Buffers would be Jesper's Buffer Pools?
With Buffer Pools the concern that a pre-configured and pinned pool of
DMA-mapped pages will start growing and eat all host's memory is more
real. That to me that's closer. If we develop XDP-based fastpaths
with DMA pools shared between devices - that's much more like an ASIC's
SB.
In my view we don't offload the limit not because we configure it via
an different API, but because the limit assumes there is abundance of
memory and queue has to be capped. Limit expresses how much queue
build up is okay, while SB config is strictly a resource quota. In
practice the quota is always a lot lower than user's desired limit so
we don't even bother with the limit.
For example, switches (I'm familiar with Mellanox ASICs, but I assume
the concept is similar in other ASICs) have ingress buffers where
packets are stored while going through the pipeline. Once out of the
pipeline you know from which port and queue the packet should egress. In
case you have both lossless and lossy traffic in your network you
probably want to classify it into different ingress buffers and mark the
buffers where the lossless traffic is stored as such, so that PFC frames
would be emitted above a certain threshold.
This is currently configured using dcbnl, but it lacks a software model
which means that packets that are forwarded by the kernel don't get the
same treatment (e.g., skb priority isn't set). It also means that when
you want to limit the number of packets that are queued *from* a certain
port and ingress buffer you resort to tools such as devlink-sb that end
up colliding with existing tools (tc).
Extending DCB further into the kernel on ingress does not seem
impossible. Maybe the AVB/industrial folks will tackle that at some
point?
I was thinking (not too much...) about modelling the above using ingress
qdiscs. They don't do any queueing, but more of accounting. Once the
egress qdisc dequeues the packet, you give credit back to the ingress
qdisc from which the packet came from. I believe that modelling these
buffers using the qdisc layer is the right abstraction.
Interesting. My concern would be mapping the packet back to ingress
port to free the right qdisc credit. MM direction, like Buffer Pools,
seem more viable to a layman like me. But ingress qdiscs sound worth
exploring.
Would appreciate hearing your thoughts on the above.
Thanks a lot for your response, you've certainly given me things to
think about over the weekend :) A lot of cool things we can build if
we keep moving forward :)
From: Eran Ben Elisha <hidden> Date: 2018-08-06 15:10:40
quoted
Hi Dave,
I would like to re-state that this feature was not meant to be a generic
one. This feature was added in order to resolve a HW bug which exist in
a small portion of our devices.
Would you mind describing the HW bug in more detail? To a outside
reviewer it really looks like you're adding a feature. What are you
working around? Is the lack of full AQM on the PCIe side of the chip
considered a bug?
In multiple function environment, there is an issue with buffer
allocation per function which may lead to starvation. There is an HW WA
for mitigate this starvation by identifying this state and apply early
drop/mark.
quoted
Those params will be used only on those current HWs and won't be in
use for our future devices.
I'm glad that is your plan today, however, customers may get used to
the simple interface you're adding now. This means the API you are
adding is effectively becoming an API other drivers may need to
implement to keep compatibility with someone's proprietary
orchestration.
This issue was refactored, thus no need to have this WA at all in future
NICs. So I don't believe we will end up in the situation you are describing.
It is less likely that other vendors will be facing the same issue and
will have to support such param. it was burn out of a bug and not as a
feature which other may follow.
quoted
During the discussions, several alternatives where offered to be used by
various members of the community. These alternatives includes TC and
enhancements to PCI configuration tools.
Regarding the TC, from my perspective, this is not an option as:
1) The HW mechanism handles multiple functions and therefore cannot be
configured on as a regular TC
Could you elaborate? What are the multiple functions? You seem to be
adding a knob to enable ECN marking and a knob for choosing between
some predefined slopes.
PSB, The sloped are dynamic and enabled in a dynamic way.
Indeed, we are adding a very specific knob for very non standard
specific issue which can be used in addition to standard ECN marking.
In what way would your solution not behave like a RED offload?
Existing Algo (RED, PIE, etc) are static, configurable. Our HW WA is
dynamic (dynamic slope), adjusted and auto enabled.
With TC offload you'd also get a well-defined set of statistics, I
presume right now you're planning on adding a set of ethtool -S
counters?
quoted
2) No PF + representors modeling can be applied here, this is a
MultiHost environment where one host is not aware to the other hosts,
and each is running on its own pci/driver. It is a device working mode
configuration.
Yes, the multihost part makes it less pleasant. But this is a problem
we have to tackle separately, at some point. It's not a center of
attention here.
Agree, however the multihost part makes it non-transparent if we chose a
solution which is not based on direct vendor configuration. This will
lead to a bad user experience.
quoted
3) The current HW W/A is very limited, maybe it has a similar algorithm
as WRED, but is being used for much simpler different use case (pci bus
congestion).
No one is requesting full RED offload here.. if someone sets the
parameters you can't support you simply won't offload them. And ignore
the parameters which only make sense in software terms. Look at the
docs for mlxsw:
https://github.com/Mellanox/mlxsw/wiki/Queues-Management#offloading-red
It says "not offloaded" in a number of places.
quoted
It cannot be compared to a standard TC capability (RED/WRED), and
defining it as a offload fully controlled by the user will be a big
misuse.
It's generally preferable to implement a subset of exiting well defined
API than create vendor knobs, hence hardly a misuse.
As written above, this is not the case here.
quoted
(for example, drop rate cannot be configured)
I don't know what "configuring drop rate" means in case of RED..
quoted
regarding the PCI config tools, there was a consensus that such tool is
not acceptable as it is not a part of the PCI spec.
As I said, this has nothing to do with PCI being the transport. The
port you're running over could be serial, SPI or anything else. You
have congestion on a port of a device, that's a networking problem.
quoted
Since module param/sysfs/debugfs/etc are no longer acceptable, and
current drivers still desired with a way to do some configurations to
the device/driver which cannot used standard Linux tool or by other
vendors, devlink params was developed (under the assumption that this
tool will be helpful for those needs, and those only).
From my perspective, Devlink is the tool to configure the device for
handling such unexpected bugs, i.e "PCIe buffer congestion handling
workaround".
Hm. Are you calling it a bug because you had to work around silicon
limitation in firmware? Hm. I'm very intrigued by the framing :)
From: Jakub Kicinski <hidden> Date: 2018-08-07 03:00:52
On Mon, 6 Aug 2018 16:01:25 +0300, Eran Ben Elisha wrote:
quoted
quoted
Hi Dave,
I would like to re-state that this feature was not meant to be a generic
one. This feature was added in order to resolve a HW bug which exist in
a small portion of our devices.
Would you mind describing the HW bug in more detail? To a outside
reviewer it really looks like you're adding a feature. What are you
working around? Is the lack of full AQM on the PCIe side of the chip
considered a bug?
In multiple function environment, there is an issue with buffer
allocation per function which may lead to starvation.
Multi-function? I thought you have a PF per uplink on all mlx5
silicon. Does the problem occur in single host scenarios as well?
What if with single function the host is too slow with taking packets
off the RX ring? Can the problem occur? Would this feature help in
such scenario as well?
There is an HW WA for mitigate this starvation by identifying this
state and apply early drop/mark.
If I understand you correctly you presently have one shared buffer with
no way to place limits (quotas) on how much of it can be consumed by a
traffic for a single PF. It remains unclear why this have not been a
problem for you until now.
To avoid the starvation you are adding AQM which is a *feature* that
may help you avoid queue build up in the NIC. But even if you could
place quotas, why would you not expose the AQM scheme? It looks very
useful.
quoted
quoted
Those params will be used only on those current HWs and won't be in
use for our future devices.
I'm glad that is your plan today, however, customers may get used to
the simple interface you're adding now. This means the API you are
adding is effectively becoming an API other drivers may need to
implement to keep compatibility with someone's proprietary
orchestration.
This issue was refactored, thus no need to have this WA at all in
future NICs. So I don't believe we will end up in the situation you are
describing. It is less likely that other vendors will be facing the
same issue and will have to support such param. it was burn out of a
bug and not as a feature which other may follow.
Sure other vendors may have buffers quotas configurable by e.g.
devlink-sb. But the AQM you are adding is a feature which is
potentially already supported by others.
quoted
quoted
During the discussions, several alternatives where offered to be
used by various members of the community. These alternatives
includes TC and enhancements to PCI configuration tools.
Regarding the TC, from my perspective, this is not an option as:
1) The HW mechanism handles multiple functions and therefore
cannot be configured on as a regular TC
Could you elaborate? What are the multiple functions? You seem to
be adding a knob to enable ECN marking and a knob for choosing
between some predefined slopes.
PSB, The sloped are dynamic and enabled in a dynamic way.
Indeed, we are adding a very specific knob for very non standard
specific issue which can be used in addition to standard ECN marking.
quoted
In what way would your solution not behave like a RED offload?
Existing Algo (RED, PIE, etc) are static, configurable. Our HW WA is
dynamic (dynamic slope), adjusted and auto enabled.
You mean like Adaptive RED? The lack of documentation is making this
conversation harder to have. What's dynamic and aggressive? These
are not antonyms. Are the parameters to the algorithm configurable?
Will you not want to expose the actual threshold and adjustment values
so the customers can tweak them on their own depending on the workload?
quoted
With TC offload you'd also get a well-defined set of statistics, I
presume right now you're planning on adding a set of ethtool -S
counters?
quoted
2) No PF + representors modeling can be applied here, this is a
MultiHost environment where one host is not aware to the other
hosts, and each is running on its own pci/driver. It is a device
working mode configuration.
Yes, the multihost part makes it less pleasant. But this is a
problem we have to tackle separately, at some point. It's not a
center of attention here.
Agree, however the multihost part makes it non-transparent if we
chose a solution which is not based on direct vendor configuration.
This will lead to a bad user experience.
In my experience multi-host is not a major issue in practice. And
switchdev mode gives some visibility into statistics of others hosts
etc., which people appreciate.