[dpdk-dev] [PATCH] i40e: workaround for X710 performance issues

[dpdk-dev] [PATCH] i40e: workaround for X710 performance issues

[Helin Zhang]

As the fixes of below performance issues on X710 may not be integrated
in latest version of firmware, a workaround in software PMD is needed.
It is to re-configure 3 specific registers after being initialized.
- Cannot achieve line rate on X710.
- Performance reduction when promiscuous mode is disabled.
Note that this workaround can be removed if the fixes are integrated
in the firmware in future.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 87 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index 008d62c..adaab93 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -198,6 +198,7 @@ static int i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
 				enum rte_filter_type filter_type,
 				enum rte_filter_op filter_op,
 				void *arg);
+static void i40e_configure_registers(struct i40e_hw *hw);
 
 /* Default hash key buffer for RSS */
 static uint32_t rss_key_default[I40E_PFQF_HKEY_MAX_INDEX + 1];
@@ -443,6 +444,15 @@ eth_i40e_dev_init(__rte_unused struct eth_driver *eth_drv,
 	/* Clear PXE mode */
 	i40e_clear_pxe_mode(hw);
 
+	/*
+	 * On X710, as old version of firmwares may have performance issues,
+	 * 3 registers need to be re-configured with new values. And the latest
+	 * version of firmware may not contain the fixes, workaround in SW
+	 * driver is needed. This workaround can be removed when the fixes are
+	 * integrated in firmware in future.
+	 */
+	i40e_configure_registers(hw);
+
 	/* Get hw capabilities */
 	ret = i40e_get_cap(hw);
 	if (ret != I40E_SUCCESS) {
@@ -5294,3 +5304,80 @@ i40e_pctype_to_flowtype(enum i40e_filter_pctype pctype)
 
 	return flowtype_table[pctype];
 }
+
+static int
+i40e_debug_read_register(struct i40e_hw *hw, uint32_t addr, uint64_t *val)
+{
+	struct i40e_aq_desc desc;
+	struct i40e_aqc_debug_reg_read_write *cmd =
+		(struct i40e_aqc_debug_reg_read_write *)&desc.params.raw;
+	enum i40e_status_code status;
+
+	i40e_fill_default_direct_cmd_desc(&desc, i40e_aqc_opc_debug_read_reg);
+	cmd->address = rte_cpu_to_le_32(addr);
+	status = i40e_asq_send_command(hw, &desc, NULL, 0, NULL);
+	if (status < 0)
+		return status;
+
+	*val = ((uint64_t)(rte_le_to_cpu_32(cmd->value_high)) << (CHAR_BIT *
+			sizeof(uint32_t))) + rte_le_to_cpu_32(cmd->value_low);
+
+	return status;
+}
+
+/*
+ * On X710, as old version of firmwares may have performance issues,
+ * 3 registers need to be re-configured with new values. And the latest version
+ * of firmware may not contain the fixes, workaround in SW driver is needed.
+ * This workaround can be removed when the fixes are integrated in firmware in
+ * future.
+ */
+static void
+i40e_configure_registers(struct i40e_hw *hw)
+{
+#define I40E_GL_SWR_PRI_JOIN_MAP_0       0x26CE00
+#define I40E_GL_SWR_PRI_JOIN_MAP_2       0x26CE08
+#define I40E_GL_SWR_PM_UP_THR            0x269FBC
+#define I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE 0x10000200
+#define I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE 0x011f0200
+#define I40E_GL_SWR_PM_UP_THR_VALUE      0x03030303
+
+	static const struct {
+		uint32_t addr;
+		uint64_t val;
+	} reg_table[] = {
+		{I40E_GL_SWR_PRI_JOIN_MAP_0, I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE},
+		{I40E_GL_SWR_PRI_JOIN_MAP_2, I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE},
+		{I40E_GL_SWR_PM_UP_THR, I40E_GL_SWR_PM_UP_THR_VALUE},
+	};
+	uint64_t reg;
+	uint32_t i;
+	int ret;
+
+	/* Below fix is for X710 only */
+	if (i40e_is_40G_device(hw->device_id))
+		return;
+
+	for (i = 0; i < RTE_DIM(reg_table); i++) {
+		ret = i40e_debug_read_register(hw, reg_table[i].addr, &reg);
+		if (ret < 0) {
+			PMD_DRV_LOG(ERR, "Failed to read from 0x%x\n",
+						reg_table[i].addr);
+			break;
+		}
+		PMD_DRV_LOG(DEBUG, "Read from 0x%x: 0x%lx", reg_table[i].addr,
+									reg);
+		if (reg == reg_table[i].val)
+			continue;
+
+		ret = i40e_aq_debug_write_register(hw, reg_table[i].addr,
+						reg_table[i].val, NULL);
+		if (ret < 0) {
+			PMD_DRV_LOG(ERR, "Failed to write 0x%x to 0x%x\n",
+				reg_table[i].val, reg_table[i].addr);
+			break;
+		}
+		PMD_DRV_LOG(DEBUG, "Write to 0x%x: 0x%lx", reg_table[i].addr,
+							reg_table[i].val);
+	}
+}
-- 
1.8.1.4

[dpdk-dev] [PATCH v2] i40e: workaround for X710 performance issues

[Helin Zhang]

As the fixes of below performance issues on X710 may not be integrated
in latest version of firmware, a workaround in software PMD is needed.
It is to re-configure 3 specific registers after being initialized.
- Cannot achieve line rate on X710.
- Performance reduction when promiscuous mode is disabled.
Note that this workaround can be removed if the fixes are integrated
in the firmware in future.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 87 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

v2 changes:
* Added a compile error fix.

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index 008d62c..82c072b 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -198,6 +198,7 @@ static int i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
 				enum rte_filter_type filter_type,
 				enum rte_filter_op filter_op,
 				void *arg);
+static void i40e_configure_registers(struct i40e_hw *hw);
 
 /* Default hash key buffer for RSS */
 static uint32_t rss_key_default[I40E_PFQF_HKEY_MAX_INDEX + 1];
@@ -443,6 +444,15 @@ eth_i40e_dev_init(__rte_unused struct eth_driver *eth_drv,
 	/* Clear PXE mode */
 	i40e_clear_pxe_mode(hw);
 
+	/*
+	 * On X710, as old version of firmwares may have performance issues,
+	 * 3 registers need to be re-configured with new values. And the latest
+	 * version of firmware may not contain the fixes, workaround in SW
+	 * driver is needed. This workaround can be removed when the fixes are
+	 * integrated in firmware in future.
+	 */
+	i40e_configure_registers(hw);
+
 	/* Get hw capabilities */
 	ret = i40e_get_cap(hw);
 	if (ret != I40E_SUCCESS) {
@@ -5294,3 +5304,80 @@ i40e_pctype_to_flowtype(enum i40e_filter_pctype pctype)
 
 	return flowtype_table[pctype];
 }
+
+static int
+i40e_debug_read_register(struct i40e_hw *hw, uint32_t addr, uint64_t *val)
+{
+	struct i40e_aq_desc desc;
+	struct i40e_aqc_debug_reg_read_write *cmd =
+		(struct i40e_aqc_debug_reg_read_write *)&desc.params.raw;
+	enum i40e_status_code status;
+
+	i40e_fill_default_direct_cmd_desc(&desc, i40e_aqc_opc_debug_read_reg);
+	cmd->address = rte_cpu_to_le_32(addr);
+	status = i40e_asq_send_command(hw, &desc, NULL, 0, NULL);
+	if (status < 0)
+		return status;
+
+	*val = ((uint64_t)(rte_le_to_cpu_32(cmd->value_high)) << (CHAR_BIT *
+			sizeof(uint32_t))) + rte_le_to_cpu_32(cmd->value_low);
+
+	return status;
+}
+
+/*
+ * On X710, as old version of firmwares may have performance issues,
+ * 3 registers need to be re-configured with new values. And the latest version
+ * of firmware may not contain the fixes, workaround in SW driver is needed.
+ * This workaround can be removed when the fixes are integrated in firmware in
+ * future.
+ */
+static void
+i40e_configure_registers(struct i40e_hw *hw)
+{
+#define I40E_GL_SWR_PRI_JOIN_MAP_0       0x26CE00
+#define I40E_GL_SWR_PRI_JOIN_MAP_2       0x26CE08
+#define I40E_GL_SWR_PM_UP_THR            0x269FBC
+#define I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE 0x10000200
+#define I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE 0x011f0200
+#define I40E_GL_SWR_PM_UP_THR_VALUE      0x03030303
+
+	static const struct {
+		uint32_t addr;
+		uint64_t val;
+	} reg_table[] = {
+		{I40E_GL_SWR_PRI_JOIN_MAP_0, I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE},
+		{I40E_GL_SWR_PRI_JOIN_MAP_2, I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE},
+		{I40E_GL_SWR_PM_UP_THR, I40E_GL_SWR_PM_UP_THR_VALUE},
+	};
+	uint64_t reg;
+	uint32_t i;
+	int ret;
+
+	/* Below fix is for X710 only */
+	if (i40e_is_40G_device(hw->device_id))
+		return;
+
+	for (i = 0; i < RTE_DIM(reg_table); i++) {
+		ret = i40e_debug_read_register(hw, reg_table[i].addr, &reg);
+		if (ret < 0) {
+			PMD_DRV_LOG(ERR, "Failed to read from 0x%x\n",
+						reg_table[i].addr);
+			break;
+		}
+		PMD_DRV_LOG(DEBUG, "Read from 0x%x: 0x%lx", reg_table[i].addr,
+									reg);
+		if (reg == reg_table[i].val)
+			continue;
+
+		ret = i40e_aq_debug_write_register(hw, reg_table[i].addr,
+						reg_table[i].val, NULL);
+		if (ret < 0) {
+			PMD_DRV_LOG(ERR, "Failed to write 0x%lx to 0x%x\n",
+				reg_table[i].val, reg_table[i].addr);
+			break;
+		}
+		PMD_DRV_LOG(DEBUG, "Write to 0x%x: 0x%lx", reg_table[i].addr,
+							reg_table[i].val);
+	}
+}
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH v2] i40e: workaround for X710 performance issues

[Chen, Jing D]

Hi Helin,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Helin Zhang
> Sent: Monday, December 15, 2014 3:56 PM
> To: dev@dpdk.org
> Cc: Rowden, Aaron F
> Subject: [dpdk-dev] [PATCH v2] i40e: workaround for X710 performance
> issues
> 
> As the fixes of below performance issues on X710 may not be integrated
> in latest version of firmware, a workaround in software PMD is needed.
> It is to re-configure 3 specific registers after being initialized.
> - Cannot achieve line rate on X710.

packet size? 

> - Performance reduction when promiscuous mode is disabled.

You'd better add above descriptions in line with the code.

> Note that this workaround can be removed if the fixes are integrated
> in the firmware in future.
> 

I saw below code applied register setting in case it's 40G device. Can you give
more description on what device this patch would boost performance?
Will 10G fiber interface benefit from the change?

> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  lib/librte_pmd_i40e/i40e_ethdev.c | 87
> +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> v2 changes:
> * Added a compile error fix.
> 
> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> b/lib/librte_pmd_i40e/i40e_ethdev.c
> index 008d62c..82c072b 100644
> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> @@ -198,6 +198,7 @@ static int i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
>  				enum rte_filter_type filter_type,
>  				enum rte_filter_op filter_op,
>  				void *arg);
> +static void i40e_configure_registers(struct i40e_hw *hw);
> 
>  /* Default hash key buffer for RSS */
>  static uint32_t rss_key_default[I40E_PFQF_HKEY_MAX_INDEX + 1];
> @@ -443,6 +444,15 @@ eth_i40e_dev_init(__rte_unused struct eth_driver
> *eth_drv,
>  	/* Clear PXE mode */
>  	i40e_clear_pxe_mode(hw);
> 
> +	/*
> +	 * On X710, as old version of firmwares may have performance issues,
> +	 * 3 registers need to be re-configured with new values. And the
> latest
> +	 * version of firmware may not contain the fixes, workaround in SW
> +	 * driver is needed. This workaround can be removed when the fixes
> are
> +	 * integrated in firmware in future.
> +	 */
> +	i40e_configure_registers(hw);
> +
>  	/* Get hw capabilities */
>  	ret = i40e_get_cap(hw);
>  	if (ret != I40E_SUCCESS) {
> @@ -5294,3 +5304,80 @@ i40e_pctype_to_flowtype(enum
> i40e_filter_pctype pctype)
> 
>  	return flowtype_table[pctype];
>  }
> +
> +static int
> +i40e_debug_read_register(struct i40e_hw *hw, uint32_t addr, uint64_t
> *val)
> +{
> +	struct i40e_aq_desc desc;
> +	struct i40e_aqc_debug_reg_read_write *cmd =
> +		(struct i40e_aqc_debug_reg_read_write
> *)&desc.params.raw;
> +	enum i40e_status_code status;
> +
> +	i40e_fill_default_direct_cmd_desc(&desc,
> i40e_aqc_opc_debug_read_reg);
> +	cmd->address = rte_cpu_to_le_32(addr);
> +	status = i40e_asq_send_command(hw, &desc, NULL, 0, NULL);
> +	if (status < 0)
> +		return status;
> +
> +	*val = ((uint64_t)(rte_le_to_cpu_32(cmd->value_high)) <<
> (CHAR_BIT *
> +			sizeof(uint32_t))) + rte_le_to_cpu_32(cmd-
> >value_low);
> +
> +	return status;
> +}
> +
> +/*
> + * On X710, as old version of firmwares may have performance issues,
> + * 3 registers need to be re-configured with new values. And the latest
> version
> + * of firmware may not contain the fixes, workaround in SW driver is
> needed.
> + * This workaround can be removed when the fixes are integrated in
> firmware in
> + * future.
> + */
> +static void
> +i40e_configure_registers(struct i40e_hw *hw)
> +{
> +#define I40E_GL_SWR_PRI_JOIN_MAP_0       0x26CE00
> +#define I40E_GL_SWR_PRI_JOIN_MAP_2       0x26CE08
> +#define I40E_GL_SWR_PM_UP_THR            0x269FBC
> +#define I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE 0x10000200
> +#define I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE 0x011f0200
> +#define I40E_GL_SWR_PM_UP_THR_VALUE      0x03030303
> +
> +	static const struct {
> +		uint32_t addr;
> +		uint64_t val;
> +	} reg_table[] = {
> +		{I40E_GL_SWR_PRI_JOIN_MAP_0,
> I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE},
> +		{I40E_GL_SWR_PRI_JOIN_MAP_2,
> I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE},
> +		{I40E_GL_SWR_PM_UP_THR,
> I40E_GL_SWR_PM_UP_THR_VALUE},
> +	};
> +	uint64_t reg;
> +	uint32_t i;
> +	int ret;
> +
> +	/* Below fix is for X710 only */
> +	if (i40e_is_40G_device(hw->device_id))
> +		return;
> +
> +	for (i = 0; i < RTE_DIM(reg_table); i++) {
> +		ret = i40e_debug_read_register(hw, reg_table[i].addr, &reg);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Failed to read from 0x%x\n",
> +						reg_table[i].addr);
> +			break;
> +		}
> +		PMD_DRV_LOG(DEBUG, "Read from 0x%x: 0x%lx",
> reg_table[i].addr,
> +									reg);

PRIu64?

> +		if (reg == reg_table[i].val)
> +			continue;
> +
> +		ret = i40e_aq_debug_write_register(hw, reg_table[i].addr,
> +						reg_table[i].val, NULL);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Failed to write 0x%lx to
> 0x%x\n",
> +				reg_table[i].val, reg_table[i].addr);

PRIu64?

> +			break;
> +		}
> +		PMD_DRV_LOG(DEBUG, "Write to 0x%x: 0x%lx",
> reg_table[i].addr,
> +							reg_table[i].val);

PRIu64?

> +	}
> +}

I saw some wrong code alignment above, but not sure how it looks like on 
terminal, please double check.

> --
> 1.8.1.4

Re: [dpdk-dev] [PATCH v2] i40e: workaround for X710 performance issues

[Zhang, Helin]

> -----Original Message-----
> From: Chen, Jing D
> Sent: Tuesday, December 16, 2014 10:29 AM
> To: Zhang, Helin; dev@dpdk.org
> Cc: Rowden, Aaron F
> Subject: RE: [dpdk-dev] [PATCH v2] i40e: workaround for X710 performance
> issues
> 
> Hi Helin,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Helin Zhang
> > Sent: Monday, December 15, 2014 3:56 PM
> > To: dev@dpdk.org
> > Cc: Rowden, Aaron F
> > Subject: [dpdk-dev] [PATCH v2] i40e: workaround for X710 performance
> > issues
> >
> > As the fixes of below performance issues on X710 may not be integrated
> > in latest version of firmware, a workaround in software PMD is needed.
> > It is to re-configure 3 specific registers after being initialized.
> > - Cannot achieve line rate on X710.
> 
> packet size?
For 10G, it is expected to achieve line rate for all packet sizes.

> 
> > - Performance reduction when promiscuous mode is disabled.
> 
> You'd better add above descriptions in line with the code.
It is to re-configure 3 specific registers which is not open in datasheet.
So no need to describe the name and the functionality of those registers in detail.

> 
> > Note that this workaround can be removed if the fixes are integrated
> > in the firmware in future.
> >
> 
> I saw below code applied register setting in case it's 40G device. Can you give
> more description on what device this patch would boost performance?
> Will 10G fiber interface benefit from the change?
It is for X710 only. For XL710, debugging is still in process.

> 
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> > ---
> >  lib/librte_pmd_i40e/i40e_ethdev.c | 87
> > +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >
> > v2 changes:
> > * Added a compile error fix.
> >
> > diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> > b/lib/librte_pmd_i40e/i40e_ethdev.c
> > index 008d62c..82c072b 100644
> > --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> > +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> > @@ -198,6 +198,7 @@ static int i40e_dev_filter_ctrl(struct rte_eth_dev
> *dev,
> >  				enum rte_filter_type filter_type,
> >  				enum rte_filter_op filter_op,
> >  				void *arg);
> > +static void i40e_configure_registers(struct i40e_hw *hw);
> >
> >  /* Default hash key buffer for RSS */  static uint32_t
> > rss_key_default[I40E_PFQF_HKEY_MAX_INDEX + 1]; @@ -443,6 +444,15
> @@
> > eth_i40e_dev_init(__rte_unused struct eth_driver *eth_drv,
> >  	/* Clear PXE mode */
> >  	i40e_clear_pxe_mode(hw);
> >
> > +	/*
> > +	 * On X710, as old version of firmwares may have performance issues,
> > +	 * 3 registers need to be re-configured with new values. And the
> > latest
> > +	 * version of firmware may not contain the fixes, workaround in SW
> > +	 * driver is needed. This workaround can be removed when the fixes
> > are
> > +	 * integrated in firmware in future.
> > +	 */
> > +	i40e_configure_registers(hw);
> > +
> >  	/* Get hw capabilities */
> >  	ret = i40e_get_cap(hw);
> >  	if (ret != I40E_SUCCESS) {
> > @@ -5294,3 +5304,80 @@ i40e_pctype_to_flowtype(enum
> i40e_filter_pctype
> > pctype)
> >
> >  	return flowtype_table[pctype];
> >  }
> > +
> > +static int
> > +i40e_debug_read_register(struct i40e_hw *hw, uint32_t addr, uint64_t
> > *val)
> > +{
> > +	struct i40e_aq_desc desc;
> > +	struct i40e_aqc_debug_reg_read_write *cmd =
> > +		(struct i40e_aqc_debug_reg_read_write
> > *)&desc.params.raw;
> > +	enum i40e_status_code status;
> > +
> > +	i40e_fill_default_direct_cmd_desc(&desc,
> > i40e_aqc_opc_debug_read_reg);
> > +	cmd->address = rte_cpu_to_le_32(addr);
> > +	status = i40e_asq_send_command(hw, &desc, NULL, 0, NULL);
> > +	if (status < 0)
> > +		return status;
> > +
> > +	*val = ((uint64_t)(rte_le_to_cpu_32(cmd->value_high)) <<
> > (CHAR_BIT *
> > +			sizeof(uint32_t))) + rte_le_to_cpu_32(cmd-
> > >value_low);
> > +
> > +	return status;
> > +}
> > +
> > +/*
> > + * On X710, as old version of firmwares may have performance issues,
> > + * 3 registers need to be re-configured with new values. And the
> > +latest
> > version
> > + * of firmware may not contain the fixes, workaround in SW driver is
> > needed.
> > + * This workaround can be removed when the fixes are integrated in
> > firmware in
> > + * future.
> > + */
> > +static void
> > +i40e_configure_registers(struct i40e_hw *hw) {
> > +#define I40E_GL_SWR_PRI_JOIN_MAP_0       0x26CE00
> > +#define I40E_GL_SWR_PRI_JOIN_MAP_2       0x26CE08
> > +#define I40E_GL_SWR_PM_UP_THR            0x269FBC
> > +#define I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE 0x10000200 #define
> > +I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE 0x011f0200
> > +#define I40E_GL_SWR_PM_UP_THR_VALUE      0x03030303
> > +
> > +	static const struct {
> > +		uint32_t addr;
> > +		uint64_t val;
> > +	} reg_table[] = {
> > +		{I40E_GL_SWR_PRI_JOIN_MAP_0,
> > I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE},
> > +		{I40E_GL_SWR_PRI_JOIN_MAP_2,
> > I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE},
> > +		{I40E_GL_SWR_PM_UP_THR,
> > I40E_GL_SWR_PM_UP_THR_VALUE},
> > +	};
> > +	uint64_t reg;
> > +	uint32_t i;
> > +	int ret;
> > +
> > +	/* Below fix is for X710 only */
> > +	if (i40e_is_40G_device(hw->device_id))
> > +		return;
> > +
> > +	for (i = 0; i < RTE_DIM(reg_table); i++) {
> > +		ret = i40e_debug_read_register(hw, reg_table[i].addr, &reg);
> > +		if (ret < 0) {
> > +			PMD_DRV_LOG(ERR, "Failed to read from 0x%x\n",
> > +						reg_table[i].addr);
> > +			break;
> > +		}
> > +		PMD_DRV_LOG(DEBUG, "Read from 0x%x: 0x%lx",
> > reg_table[i].addr,
> > +									reg);
> 
> PRIu64?
Yes, it would be better to use PRIu64.

> 
> > +		if (reg == reg_table[i].val)
> > +			continue;
> > +
> > +		ret = i40e_aq_debug_write_register(hw, reg_table[i].addr,
> > +						reg_table[i].val, NULL);
> > +		if (ret < 0) {
> > +			PMD_DRV_LOG(ERR, "Failed to write 0x%lx to
> > 0x%x\n",
> > +				reg_table[i].val, reg_table[i].addr);
> 
> PRIu64?
> 
> > +			break;
> > +		}
> > +		PMD_DRV_LOG(DEBUG, "Write to 0x%x: 0x%lx",
> > reg_table[i].addr,
> > +							reg_table[i].val);
> 
> PRIu64?
> 
> > +	}
> > +}
> 
> I saw some wrong code alignment above, but not sure how it looks like on
> terminal, please double check.
All passed checks by checkpatch.pl. Email always shows different format than patch itself.

> 
> > --
> > 1.8.1.4

[dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Helin Zhang]

On X710, performance number is far from the expectation on recent
firmware versions. The fix for this issue may not be integrated in
the following firmware version. So the workaround in software driver
is needed. It needs to modify the initial values of 3 internal only
registers. Note that the workaround can be removed when it is fixed
in firmware in the future.

Signed-off-by: Helin Zhang <helin.zhang@intel.com>
---
 lib/librte_pmd_i40e/i40e_ethdev.c | 89 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

v2 changes:
* Added a compile error fix.

v3 changes:
* Used PRIx32 and PRIx64 instead for printing uint32_t and uint64_t
  variables.
* Re-worded annotations, and commit logs.

diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c b/lib/librte_pmd_i40e/i40e_ethdev.c
index 008d62c..624f0ce 100644
--- a/lib/librte_pmd_i40e/i40e_ethdev.c
+++ b/lib/librte_pmd_i40e/i40e_ethdev.c
@@ -198,6 +198,7 @@ static int i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
 				enum rte_filter_type filter_type,
 				enum rte_filter_op filter_op,
 				void *arg);
+static void i40e_configure_registers(struct i40e_hw *hw);
 
 /* Default hash key buffer for RSS */
 static uint32_t rss_key_default[I40E_PFQF_HKEY_MAX_INDEX + 1];
@@ -443,6 +444,16 @@ eth_i40e_dev_init(__rte_unused struct eth_driver *eth_drv,
 	/* Clear PXE mode */
 	i40e_clear_pxe_mode(hw);
 
+	/*
+	 * On X710, performance number is far from the expectation on recent
+	 * firmware versions. The fix for this issue may not be integrated in
+	 * the following firmware version. So the workaround in software driver
+	 * is needed. It needs to modify the initial values of 3 internal only
+	 * registers. Note that the workaround can be removed when it is fixed
+	 * in firmware in the future.
+	 */
+	i40e_configure_registers(hw);
+
 	/* Get hw capabilities */
 	ret = i40e_get_cap(hw);
 	if (ret != I40E_SUCCESS) {
@@ -5294,3 +5305,81 @@ i40e_pctype_to_flowtype(enum i40e_filter_pctype pctype)
 
 	return flowtype_table[pctype];
 }
+
+static int
+i40e_debug_read_register(struct i40e_hw *hw, uint32_t addr, uint64_t *val)
+{
+	struct i40e_aq_desc desc;
+	struct i40e_aqc_debug_reg_read_write *cmd =
+		(struct i40e_aqc_debug_reg_read_write *)&desc.params.raw;
+	enum i40e_status_code status;
+
+	i40e_fill_default_direct_cmd_desc(&desc, i40e_aqc_opc_debug_read_reg);
+	cmd->address = rte_cpu_to_le_32(addr);
+	status = i40e_asq_send_command(hw, &desc, NULL, 0, NULL);
+	if (status < 0)
+		return status;
+
+	*val = ((uint64_t)(rte_le_to_cpu_32(cmd->value_high)) << (CHAR_BIT *
+			sizeof(uint32_t))) + rte_le_to_cpu_32(cmd->value_low);
+
+	return status;
+}
+
+/*
+ * On X710, performance number is far from the expectation on recent firmware
+ * versions. The fix for this issue may not be integrated in the following
+ * firmware version. So the workaround in software driver is needed. It needs
+ * to modify the initial values of 3 internal only registers. Note that the
+ * workaround can be removed when it is fixed in firmware in the future.
+ */
+static void
+i40e_configure_registers(struct i40e_hw *hw)
+{
+#define I40E_GL_SWR_PRI_JOIN_MAP_0       0x26CE00
+#define I40E_GL_SWR_PRI_JOIN_MAP_2       0x26CE08
+#define I40E_GL_SWR_PM_UP_THR            0x269FBC
+#define I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE 0x10000200
+#define I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE 0x011f0200
+#define I40E_GL_SWR_PM_UP_THR_VALUE      0x03030303
+
+	static const struct {
+		uint32_t addr;
+		uint64_t val;
+	} reg_table[] = {
+		{I40E_GL_SWR_PRI_JOIN_MAP_0, I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE},
+		{I40E_GL_SWR_PRI_JOIN_MAP_2, I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE},
+		{I40E_GL_SWR_PM_UP_THR, I40E_GL_SWR_PM_UP_THR_VALUE},
+	};
+	uint64_t reg;
+	uint32_t i;
+	int ret;
+
+	/* Below fix is for X710 only */
+	if (i40e_is_40G_device(hw->device_id))
+		return;
+
+	for (i = 0; i < RTE_DIM(reg_table); i++) {
+		ret = i40e_debug_read_register(hw, reg_table[i].addr, &reg);
+		if (ret < 0) {
+			PMD_DRV_LOG(ERR, "Failed to read from 0x%"PRIx32,
+							reg_table[i].addr);
+			break;
+		}
+		PMD_DRV_LOG(DEBUG, "Read from 0x%"PRIx32": 0x%"PRIx64,
+						reg_table[i].addr, reg);
+		if (reg == reg_table[i].val)
+			continue;
+
+		ret = i40e_aq_debug_write_register(hw, reg_table[i].addr,
+						reg_table[i].val, NULL);
+		if (ret < 0) {
+			PMD_DRV_LOG(ERR, "Failed to write 0x%"PRIx64" to the "
+				"address of 0x%"PRIx32, reg_table[i].val,
+							reg_table[i].addr);
+			break;
+		}
+		PMD_DRV_LOG(DEBUG, "Write 0x%"PRIx64" to the address of "
+			"0x%"PRIx32, reg_table[i].val, reg_table[i].addr);
+	}
+}
-- 
1.8.1.4

Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Thomas Monjalon]

Hi Helin,

First, thank you and Jing for the excellent review process.

2014-12-16 16:23, Helin Zhang:
> On X710, performance number is far from the expectation on recent
> firmware versions. The fix for this issue may not be integrated in
> the following firmware version. So the workaround in software driver
> is needed. It needs to modify the initial values of 3 internal only
> registers. Note that the workaround can be removed when it is fixed
> in firmware in the future.

I want to understand something here. You say the workaround can be removed
when firmware will be fixed. But I suppose that some NICs won't be updated
so the workaround will be needed forever?

-- 
Thomas

Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Chen, Jing D]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Tuesday, December 16, 2014 4:23 PM
> To: dev@dpdk.org
> Cc: Chen, Jing D; Wu, Jingjing; Liu, Jijiang; Cao, Waterman; Lu, Patrick;
> Rowden, Aaron F; Zhang, Helin
> Subject: [PATCH v3] i40e: workaround for X710 performance issues
> 
> On X710, performance number is far from the expectation on recent
> firmware versions. The fix for this issue may not be integrated in
> the following firmware version. So the workaround in software driver
> is needed. It needs to modify the initial values of 3 internal only
> registers. Note that the workaround can be removed when it is fixed
> in firmware in the future.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  lib/librte_pmd_i40e/i40e_ethdev.c | 89
> +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> v2 changes:
> * Added a compile error fix.
> 
> v3 changes:
> * Used PRIx32 and PRIx64 instead for printing uint32_t and uint64_t
>   variables.
> * Re-worded annotations, and commit logs.
> 
> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> b/lib/librte_pmd_i40e/i40e_ethdev.c
> index 008d62c..624f0ce 100644
> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> @@ -198,6 +198,7 @@ static int i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
>  				enum rte_filter_type filter_type,
>  				enum rte_filter_op filter_op,
>  				void *arg);
> +static void i40e_configure_registers(struct i40e_hw *hw);
> 
>  /* Default hash key buffer for RSS */
>  static uint32_t rss_key_default[I40E_PFQF_HKEY_MAX_INDEX + 1];
> @@ -443,6 +444,16 @@ eth_i40e_dev_init(__rte_unused struct eth_driver
> *eth_drv,
>  	/* Clear PXE mode */
>  	i40e_clear_pxe_mode(hw);
> 
> +	/*
> +	 * On X710, performance number is far from the expectation on
> recent
> +	 * firmware versions. The fix for this issue may not be integrated in
> +	 * the following firmware version. So the workaround in software
> driver
> +	 * is needed. It needs to modify the initial values of 3 internal only
> +	 * registers. Note that the workaround can be removed when it is
> fixed
> +	 * in firmware in the future.
> +	 */
> +	i40e_configure_registers(hw);
> +
>  	/* Get hw capabilities */
>  	ret = i40e_get_cap(hw);
>  	if (ret != I40E_SUCCESS) {
> @@ -5294,3 +5305,81 @@ i40e_pctype_to_flowtype(enum
> i40e_filter_pctype pctype)
> 
>  	return flowtype_table[pctype];
>  }
> +
> +static int
> +i40e_debug_read_register(struct i40e_hw *hw, uint32_t addr, uint64_t
> *val)
> +{
> +	struct i40e_aq_desc desc;
> +	struct i40e_aqc_debug_reg_read_write *cmd =
> +		(struct i40e_aqc_debug_reg_read_write
> *)&desc.params.raw;
> +	enum i40e_status_code status;
> +
> +	i40e_fill_default_direct_cmd_desc(&desc,
> i40e_aqc_opc_debug_read_reg);
> +	cmd->address = rte_cpu_to_le_32(addr);
> +	status = i40e_asq_send_command(hw, &desc, NULL, 0, NULL);
> +	if (status < 0)
> +		return status;
> +
> +	*val = ((uint64_t)(rte_le_to_cpu_32(cmd->value_high)) <<
> (CHAR_BIT *
> +			sizeof(uint32_t))) + rte_le_to_cpu_32(cmd-
> >value_low);
> +
> +	return status;
> +}
> +
> +/*
> + * On X710, performance number is far from the expectation on recent
> firmware
> + * versions. The fix for this issue may not be integrated in the following
> + * firmware version. So the workaround in software driver is needed. It
> needs
> + * to modify the initial values of 3 internal only registers. Note that the
> + * workaround can be removed when it is fixed in firmware in the future.
> + */
> +static void
> +i40e_configure_registers(struct i40e_hw *hw)
> +{
> +#define I40E_GL_SWR_PRI_JOIN_MAP_0       0x26CE00
> +#define I40E_GL_SWR_PRI_JOIN_MAP_2       0x26CE08
> +#define I40E_GL_SWR_PM_UP_THR            0x269FBC
> +#define I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE 0x10000200
> +#define I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE 0x011f0200
> +#define I40E_GL_SWR_PM_UP_THR_VALUE      0x03030303
> +
> +	static const struct {
> +		uint32_t addr;
> +		uint64_t val;
> +	} reg_table[] = {
> +		{I40E_GL_SWR_PRI_JOIN_MAP_0,
> I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE},
> +		{I40E_GL_SWR_PRI_JOIN_MAP_2,
> I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE},
> +		{I40E_GL_SWR_PM_UP_THR,
> I40E_GL_SWR_PM_UP_THR_VALUE},
> +	};
> +	uint64_t reg;
> +	uint32_t i;
> +	int ret;
> +
> +	/* Below fix is for X710 only */
> +	if (i40e_is_40G_device(hw->device_id))
> +		return;
> +
> +	for (i = 0; i < RTE_DIM(reg_table); i++) {
> +		ret = i40e_debug_read_register(hw, reg_table[i].addr, &reg);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Failed to read from
> 0x%"PRIx32,
> +							reg_table[i].addr);
> +			break;
> +		}
> +		PMD_DRV_LOG(DEBUG, "Read from 0x%"PRIx32":
> 0x%"PRIx64,
> +						reg_table[i].addr, reg);
> +		if (reg == reg_table[i].val)
> +			continue;
> +
> +		ret = i40e_aq_debug_write_register(hw, reg_table[i].addr,
> +						reg_table[i].val, NULL);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Failed to write 0x%"PRIx64" to
> the "
> +				"address of 0x%"PRIx32, reg_table[i].val,
> +							reg_table[i].addr);
> +			break;
> +		}
> +		PMD_DRV_LOG(DEBUG, "Write 0x%"PRIx64" to the address
> of "
> +			"0x%"PRIx32, reg_table[i].val, reg_table[i].addr);
> +	}
> +}
> --
> 1.8.1.4


Acked-by: Jing Chen <jing.d.chen@intel.com>

Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Zhang, Helin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Tuesday, December 16, 2014 4:37 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org; Rowden, Aaron F; Chen, Jing D
> Subject: Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance
> issues
> 
> Hi Helin,
> 
> First, thank you and Jing for the excellent review process.
> 
> 2014-12-16 16:23, Helin Zhang:
> > On X710, performance number is far from the expectation on recent
> > firmware versions. The fix for this issue may not be integrated in the
> > following firmware version. So the workaround in software driver is
> > needed. It needs to modify the initial values of 3 internal only
> > registers. Note that the workaround can be removed when it is fixed in
> > firmware in the future.
> 
> I want to understand something here. You say the workaround can be removed
> when firmware will be fixed. But I suppose that some NICs won't be updated so
> the workaround will be needed forever?
Yes, it is possible. Let me check with our NIC marketing guys of their firmware update strategy before removing it.
Now it is quite useful for high performance. Thank you very much for pointing out this!

Regards,
Helin

> 
> --
> Thomas

Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Thomas Monjalon]

> > On X710, performance number is far from the expectation on recent
> > firmware versions. The fix for this issue may not be integrated in
> > the following firmware version. So the workaround in software driver
> > is needed. It needs to modify the initial values of 3 internal only
> > registers. Note that the workaround can be removed when it is fixed
> > in firmware in the future.
> > 
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> 
> Acked-by: Jing Chen <jing.d.chen@intel.com>

Applied

Thanks
-- 
Thomas

Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Wu, Jingjing]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Tuesday, December 16, 2014 4:23 PM
> To: dev@dpdk.org
> Cc: Chen, Jing D; Wu, Jingjing; Liu, Jijiang; Cao, Waterman; Lu, Patrick;
> Rowden, Aaron F; Zhang, Helin
> Subject: [PATCH v3] i40e: workaround for X710 performance issues
> 
> On X710, performance number is far from the expectation on recent
> firmware versions. The fix for this issue may not be integrated in the
> following firmware version. So the workaround in software driver is needed.
> It needs to modify the initial values of 3 internal only registers. Note that the
> workaround can be removed when it is fixed in firmware in the future.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  lib/librte_pmd_i40e/i40e_ethdev.c | 89
> +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> v2 changes:
> * Added a compile error fix.
> 
> v3 changes:
> * Used PRIx32 and PRIx64 instead for printing uint32_t and uint64_t
>   variables.
> * Re-worded annotations, and commit logs.
> 
> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> b/lib/librte_pmd_i40e/i40e_ethdev.c
> index 008d62c..624f0ce 100644
> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> @@ -198,6 +198,7 @@ static int i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
>  				enum rte_filter_type filter_type,
>  				enum rte_filter_op filter_op,
>  				void *arg);
> +static void i40e_configure_registers(struct i40e_hw *hw);
> 
>  /* Default hash key buffer for RSS */
>  static uint32_t rss_key_default[I40E_PFQF_HKEY_MAX_INDEX + 1]; @@ -
> 443,6 +444,16 @@ eth_i40e_dev_init(__rte_unused struct eth_driver
> *eth_drv,
>  	/* Clear PXE mode */
>  	i40e_clear_pxe_mode(hw);
> 
> +	/*
> +	 * On X710, performance number is far from the expectation on
> recent
> +	 * firmware versions. The fix for this issue may not be integrated in
> +	 * the following firmware version. So the workaround in software
> driver
> +	 * is needed. It needs to modify the initial values of 3 internal only
> +	 * registers. Note that the workaround can be removed when it is
> fixed
> +	 * in firmware in the future.
> +	 */
> +	i40e_configure_registers(hw);
> +
>  	/* Get hw capabilities */
>  	ret = i40e_get_cap(hw);
>  	if (ret != I40E_SUCCESS) {
> @@ -5294,3 +5305,81 @@ i40e_pctype_to_flowtype(enum
> i40e_filter_pctype pctype)
> 
>  	return flowtype_table[pctype];
>  }
> +
> +static int
> +i40e_debug_read_register(struct i40e_hw *hw, uint32_t addr, uint64_t
> +*val) {
> +	struct i40e_aq_desc desc;
> +	struct i40e_aqc_debug_reg_read_write *cmd =
> +		(struct i40e_aqc_debug_reg_read_write
> *)&desc.params.raw;
> +	enum i40e_status_code status;
> +
> +	i40e_fill_default_direct_cmd_desc(&desc,
> i40e_aqc_opc_debug_read_reg);
> +	cmd->address = rte_cpu_to_le_32(addr);
> +	status = i40e_asq_send_command(hw, &desc, NULL, 0, NULL);
> +	if (status < 0)
> +		return status;
> +
> +	*val = ((uint64_t)(rte_le_to_cpu_32(cmd->value_high)) <<
> (CHAR_BIT *
> +			sizeof(uint32_t))) + rte_le_to_cpu_32(cmd-
> >value_low);
> +

Do we need to add rte_le_to_cpu_64 here?

> +	return status;
> +}
> +
> +/*
> + * On X710, performance number is far from the expectation on recent
> +firmware
> + * versions. The fix for this issue may not be integrated in the
> +following
> + * firmware version. So the workaround in software driver is needed. It
> +needs
> + * to modify the initial values of 3 internal only registers. Note that
> +the
> + * workaround can be removed when it is fixed in firmware in the future.
> + */
> +static void
> +i40e_configure_registers(struct i40e_hw *hw) {
> +#define I40E_GL_SWR_PRI_JOIN_MAP_0       0x26CE00
> +#define I40E_GL_SWR_PRI_JOIN_MAP_2       0x26CE08
> +#define I40E_GL_SWR_PM_UP_THR            0x269FBC
> +#define I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE 0x10000200 #define
> +I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE 0x011f0200
> +#define I40E_GL_SWR_PM_UP_THR_VALUE      0x03030303
> +
> +	static const struct {
> +		uint32_t addr;
> +		uint64_t val;
> +	} reg_table[] = {
> +		{I40E_GL_SWR_PRI_JOIN_MAP_0,
> I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE},
> +		{I40E_GL_SWR_PRI_JOIN_MAP_2,
> I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE},
> +		{I40E_GL_SWR_PM_UP_THR,
> I40E_GL_SWR_PM_UP_THR_VALUE},
> +	};
> +	uint64_t reg;
> +	uint32_t i;
> +	int ret;
> +
> +	/* Below fix is for X710 only */
> +	if (i40e_is_40G_device(hw->device_id))
> +		return;
> +
> +	for (i = 0; i < RTE_DIM(reg_table); i++) {
> +		ret = i40e_debug_read_register(hw, reg_table[i].addr, &reg);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Failed to read from
> 0x%"PRIx32,
> +							reg_table[i].addr);
> +			break;
> +		}
> +		PMD_DRV_LOG(DEBUG, "Read from 0x%"PRIx32":
> 0x%"PRIx64,
> +						reg_table[i].addr, reg);
> +		if (reg == reg_table[i].val)
> +			continue;
> +
> +		ret = i40e_aq_debug_write_register(hw, reg_table[i].addr,
> +						reg_table[i].val, NULL);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Failed to write 0x%"PRIx64" to
> the "
> +				"address of 0x%"PRIx32, reg_table[i].val,
> +							reg_table[i].addr);
> +			break;
> +		}
> +		PMD_DRV_LOG(DEBUG, "Write 0x%"PRIx64" to the address
> of "
> +			"0x%"PRIx32, reg_table[i].val, reg_table[i].addr);
> +	}
> +}
> --
> 1.8.1.4

Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Zhang, Helin]

> -----Original Message-----
> From: Wu, Jingjing
> Sent: Thursday, January 8, 2015 10:44 AM
> To: Zhang, Helin; dev@dpdk.org
> Cc: Chen, Jing D; Liu, Jijiang; Cao, Waterman; Lu, Patrick; Rowden, Aaron F
> Subject: RE: [PATCH v3] i40e: workaround for X710 performance issues
> 
> 
> 
> > -----Original Message-----
> > From: Zhang, Helin
> > Sent: Tuesday, December 16, 2014 4:23 PM
> > To: dev@dpdk.org
> > Cc: Chen, Jing D; Wu, Jingjing; Liu, Jijiang; Cao, Waterman; Lu,
> > Patrick; Rowden, Aaron F; Zhang, Helin
> > Subject: [PATCH v3] i40e: workaround for X710 performance issues
> >
> > On X710, performance number is far from the expectation on recent
> > firmware versions. The fix for this issue may not be integrated in the
> > following firmware version. So the workaround in software driver is needed.
> > It needs to modify the initial values of 3 internal only registers.
> > Note that the workaround can be removed when it is fixed in firmware in the
> future.
> >
> > Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> > ---
> >  lib/librte_pmd_i40e/i40e_ethdev.c | 89
> > +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 89 insertions(+)
> >
> > v2 changes:
> > * Added a compile error fix.
> >
> > v3 changes:
> > * Used PRIx32 and PRIx64 instead for printing uint32_t and uint64_t
> >   variables.
> > * Re-worded annotations, and commit logs.
> >
> > diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> > b/lib/librte_pmd_i40e/i40e_ethdev.c
> > index 008d62c..624f0ce 100644
> > --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> > +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> > @@ -198,6 +198,7 @@ static int i40e_dev_filter_ctrl(struct rte_eth_dev
> *dev,
> >  				enum rte_filter_type filter_type,
> >  				enum rte_filter_op filter_op,
> >  				void *arg);
> > +static void i40e_configure_registers(struct i40e_hw *hw);
> >
> >  /* Default hash key buffer for RSS */  static uint32_t
> > rss_key_default[I40E_PFQF_HKEY_MAX_INDEX + 1]; @@ -
> > 443,6 +444,16 @@ eth_i40e_dev_init(__rte_unused struct eth_driver
> > *eth_drv,
> >  	/* Clear PXE mode */
> >  	i40e_clear_pxe_mode(hw);
> >
> > +	/*
> > +	 * On X710, performance number is far from the expectation on
> > recent
> > +	 * firmware versions. The fix for this issue may not be integrated in
> > +	 * the following firmware version. So the workaround in software
> > driver
> > +	 * is needed. It needs to modify the initial values of 3 internal only
> > +	 * registers. Note that the workaround can be removed when it is
> > fixed
> > +	 * in firmware in the future.
> > +	 */
> > +	i40e_configure_registers(hw);
> > +
> >  	/* Get hw capabilities */
> >  	ret = i40e_get_cap(hw);
> >  	if (ret != I40E_SUCCESS) {
> > @@ -5294,3 +5305,81 @@ i40e_pctype_to_flowtype(enum
> i40e_filter_pctype
> > pctype)
> >
> >  	return flowtype_table[pctype];
> >  }
> > +
> > +static int
> > +i40e_debug_read_register(struct i40e_hw *hw, uint32_t addr, uint64_t
> > +*val) {
> > +	struct i40e_aq_desc desc;
> > +	struct i40e_aqc_debug_reg_read_write *cmd =
> > +		(struct i40e_aqc_debug_reg_read_write
> > *)&desc.params.raw;
> > +	enum i40e_status_code status;
> > +
> > +	i40e_fill_default_direct_cmd_desc(&desc,
> > i40e_aqc_opc_debug_read_reg);
> > +	cmd->address = rte_cpu_to_le_32(addr);
> > +	status = i40e_asq_send_command(hw, &desc, NULL, 0, NULL);
> > +	if (status < 0)
> > +		return status;
> > +
> > +	*val = ((uint64_t)(rte_le_to_cpu_32(cmd->value_high)) <<
> > (CHAR_BIT *
> > +			sizeof(uint32_t))) + rte_le_to_cpu_32(cmd-
> > >value_low);
> > +
> 
> Do we need to add rte_le_to_cpu_64 here?
No, we don't need one more conversion, as 32 bits value are already 'cpu' byte order.

Thanks,
Helin

> 
> > +	return status;
> > +}
> > +
> > +/*
> > + * On X710, performance number is far from the expectation on recent
> > +firmware
> > + * versions. The fix for this issue may not be integrated in the
> > +following
> > + * firmware version. So the workaround in software driver is needed.
> > +It needs
> > + * to modify the initial values of 3 internal only registers. Note
> > +that the
> > + * workaround can be removed when it is fixed in firmware in the future.
> > + */
> > +static void
> > +i40e_configure_registers(struct i40e_hw *hw) {
> > +#define I40E_GL_SWR_PRI_JOIN_MAP_0       0x26CE00
> > +#define I40E_GL_SWR_PRI_JOIN_MAP_2       0x26CE08
> > +#define I40E_GL_SWR_PM_UP_THR            0x269FBC
> > +#define I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE 0x10000200 #define
> > +I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE 0x011f0200
> > +#define I40E_GL_SWR_PM_UP_THR_VALUE      0x03030303
> > +
> > +	static const struct {
> > +		uint32_t addr;
> > +		uint64_t val;
> > +	} reg_table[] = {
> > +		{I40E_GL_SWR_PRI_JOIN_MAP_0,
> > I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE},
> > +		{I40E_GL_SWR_PRI_JOIN_MAP_2,
> > I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE},
> > +		{I40E_GL_SWR_PM_UP_THR,
> > I40E_GL_SWR_PM_UP_THR_VALUE},
> > +	};
> > +	uint64_t reg;
> > +	uint32_t i;
> > +	int ret;
> > +
> > +	/* Below fix is for X710 only */
> > +	if (i40e_is_40G_device(hw->device_id))
> > +		return;
> > +
> > +	for (i = 0; i < RTE_DIM(reg_table); i++) {
> > +		ret = i40e_debug_read_register(hw, reg_table[i].addr, &reg);
> > +		if (ret < 0) {
> > +			PMD_DRV_LOG(ERR, "Failed to read from
> > 0x%"PRIx32,
> > +							reg_table[i].addr);
> > +			break;
> > +		}
> > +		PMD_DRV_LOG(DEBUG, "Read from 0x%"PRIx32":
> > 0x%"PRIx64,
> > +						reg_table[i].addr, reg);
> > +		if (reg == reg_table[i].val)
> > +			continue;
> > +
> > +		ret = i40e_aq_debug_write_register(hw, reg_table[i].addr,
> > +						reg_table[i].val, NULL);
> > +		if (ret < 0) {
> > +			PMD_DRV_LOG(ERR, "Failed to write 0x%"PRIx64" to
> > the "
> > +				"address of 0x%"PRIx32, reg_table[i].val,
> > +							reg_table[i].addr);
> > +			break;
> > +		}
> > +		PMD_DRV_LOG(DEBUG, "Write 0x%"PRIx64" to the address
> > of "
> > +			"0x%"PRIx32, reg_table[i].val, reg_table[i].addr);
> > +	}
> > +}
> > --
> > 1.8.1.4

Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Wu, Jingjing]

> -----Original Message-----
> From: Zhang, Helin
> Sent: Tuesday, December 16, 2014 4:23 PM
> To: dev@dpdk.org
> Cc: Chen, Jing D; Wu, Jingjing; Liu, Jijiang; Cao, Waterman; Lu, Patrick;
> Rowden, Aaron F; Zhang, Helin
> Subject: [PATCH v3] i40e: workaround for X710 performance issues
> 
> On X710, performance number is far from the expectation on recent
> firmware versions. The fix for this issue may not be integrated in the
> following firmware version. So the workaround in software driver is needed.
> It needs to modify the initial values of 3 internal only registers. Note that the
> workaround can be removed when it is fixed in firmware in the future.
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> ---
>  lib/librte_pmd_i40e/i40e_ethdev.c | 89
> +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> v2 changes:
> * Added a compile error fix.
> 
> v3 changes:
> * Used PRIx32 and PRIx64 instead for printing uint32_t and uint64_t
>   variables.
> * Re-worded annotations, and commit logs.
> 
> diff --git a/lib/librte_pmd_i40e/i40e_ethdev.c
> b/lib/librte_pmd_i40e/i40e_ethdev.c
> index 008d62c..624f0ce 100644
> --- a/lib/librte_pmd_i40e/i40e_ethdev.c
> +++ b/lib/librte_pmd_i40e/i40e_ethdev.c
> @@ -198,6 +198,7 @@ static int i40e_dev_filter_ctrl(struct rte_eth_dev *dev,
>  				enum rte_filter_type filter_type,
>  				enum rte_filter_op filter_op,
>  				void *arg);
> +static void i40e_configure_registers(struct i40e_hw *hw);
> 
>  /* Default hash key buffer for RSS */
>  static uint32_t rss_key_default[I40E_PFQF_HKEY_MAX_INDEX + 1]; @@ -
> 443,6 +444,16 @@ eth_i40e_dev_init(__rte_unused struct eth_driver
> *eth_drv,
>  	/* Clear PXE mode */
>  	i40e_clear_pxe_mode(hw);
> 
> +	/*
> +	 * On X710, performance number is far from the expectation on
> recent
> +	 * firmware versions. The fix for this issue may not be integrated in
> +	 * the following firmware version. So the workaround in software
> driver
> +	 * is needed. It needs to modify the initial values of 3 internal only
> +	 * registers. Note that the workaround can be removed when it is
> fixed
> +	 * in firmware in the future.
> +	 */
> +	i40e_configure_registers(hw);
> +
>  	/* Get hw capabilities */
>  	ret = i40e_get_cap(hw);
>  	if (ret != I40E_SUCCESS) {
> @@ -5294,3 +5305,81 @@ i40e_pctype_to_flowtype(enum
> i40e_filter_pctype pctype)
> 
>  	return flowtype_table[pctype];
>  }
> +
> +static int
> +i40e_debug_read_register(struct i40e_hw *hw, uint32_t addr, uint64_t
> +*val) {
> +	struct i40e_aq_desc desc;
> +	struct i40e_aqc_debug_reg_read_write *cmd =
> +		(struct i40e_aqc_debug_reg_read_write
> *)&desc.params.raw;
> +	enum i40e_status_code status;
> +
> +	i40e_fill_default_direct_cmd_desc(&desc,
> i40e_aqc_opc_debug_read_reg);
> +	cmd->address = rte_cpu_to_le_32(addr);
> +	status = i40e_asq_send_command(hw, &desc, NULL, 0, NULL);
> +	if (status < 0)
> +		return status;
> +
> +	*val = ((uint64_t)(rte_le_to_cpu_32(cmd->value_high)) <<
> (CHAR_BIT *
> +			sizeof(uint32_t))) + rte_le_to_cpu_32(cmd-
> >value_low);
> +
> +	return status;
> +}
> +
> +/*
> + * On X710, performance number is far from the expectation on recent
> +firmware
> + * versions. The fix for this issue may not be integrated in the
> +following
> + * firmware version. So the workaround in software driver is needed. It
> +needs
> + * to modify the initial values of 3 internal only registers. Note that
> +the
> + * workaround can be removed when it is fixed in firmware in the future.
> + */
> +static void
> +i40e_configure_registers(struct i40e_hw *hw) {
> +#define I40E_GL_SWR_PRI_JOIN_MAP_0       0x26CE00
> +#define I40E_GL_SWR_PRI_JOIN_MAP_2       0x26CE08
> +#define I40E_GL_SWR_PM_UP_THR            0x269FBC
> +#define I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE 0x10000200 #define
> +I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE 0x011f0200
> +#define I40E_GL_SWR_PM_UP_THR_VALUE      0x03030303
> +
> +	static const struct {
> +		uint32_t addr;
> +		uint64_t val;
> +	} reg_table[] = {
> +		{I40E_GL_SWR_PRI_JOIN_MAP_0,
> I40E_GL_SWR_PRI_JOIN_MAP_0_VALUE},
> +		{I40E_GL_SWR_PRI_JOIN_MAP_2,
> I40E_GL_SWR_PRI_JOIN_MAP_2_VALUE},
> +		{I40E_GL_SWR_PM_UP_THR,
> I40E_GL_SWR_PM_UP_THR_VALUE},
> +	};
> +	uint64_t reg;
> +	uint32_t i;
> +	int ret;
> +
> +	/* Below fix is for X710 only */
> +	if (i40e_is_40G_device(hw->device_id))
> +		return;
> +
> +	for (i = 0; i < RTE_DIM(reg_table); i++) {
> +		ret = i40e_debug_read_register(hw, reg_table[i].addr, &reg);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Failed to read from
> 0x%"PRIx32,
> +							reg_table[i].addr);
> +			break;
> +		}
> +		PMD_DRV_LOG(DEBUG, "Read from 0x%"PRIx32":
> 0x%"PRIx64,
> +						reg_table[i].addr, reg);
> +		if (reg == reg_table[i].val)
> +			continue;
> +
> +		ret = i40e_aq_debug_write_register(hw, reg_table[i].addr,
> +						reg_table[i].val, NULL);
> +		if (ret < 0) {
> +			PMD_DRV_LOG(ERR, "Failed to write 0x%"PRIx64" to
> the "
> +				"address of 0x%"PRIx32, reg_table[i].val,
> +							reg_table[i].addr);
> +			break;
> +		}
> +		PMD_DRV_LOG(DEBUG, "Write 0x%"PRIx64" to the address
> of "
> +			"0x%"PRIx32, reg_table[i].val, reg_table[i].addr);
> +	}
> +}
> --
> 1.8.1.4

Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Thomas Monjalon]

2015-01-09 05:29, Wu, Jingjing:
> Acked-by: Jingjing Wu <jingjing.wu@intel.com>

Jingjing, this patch is already applied in version 1.8.0:
	http://dpdk.org/ml/archives/dev/2014-December/010164.html

-- 
Thomas

Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Zhang, Helin]

Hi Thomas, Jingjing

I guess Jingjing intended to ack another patch, which is for XL710 performance enhancement. Thanks for the good catch!

Regards,
Helin

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, January 9, 2015 11:48 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org; Zhang, Helin; Rowden, Aaron F
> Subject: Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance
> issues
> 
> 2015-01-09 05:29, Wu, Jingjing:
> > Acked-by: Jingjing Wu <jingjing.wu@intel.com>
> 
> Jingjing, this patch is already applied in version 1.8.0:
> 	http://dpdk.org/ml/archives/dev/2014-December/010164.html
> 
> --
> Thomas

Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance issues

[Wu, Jingjing]

Thanks, Thomas.

I supposed to review another patch for XL710 performance. Due to the similar patch subject, I reviewed this applied patch.

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, January 09, 2015 11:48 PM
> To: Wu, Jingjing
> Cc: dev@dpdk.org; Zhang, Helin; Rowden, Aaron F
> Subject: Re: [dpdk-dev] [PATCH v3] i40e: workaround for X710 performance
> issues
> 
> 2015-01-09 05:29, Wu, Jingjing:
> > Acked-by: Jingjing Wu <jingjing.wu@intel.com>
> 
> Jingjing, this patch is already applied in version 1.8.0:
> 	http://dpdk.org/ml/archives/dev/2014-December/010164.html
> 
> --
> Thomas