[3/3] net/mlx5: fix missing adjustment MPRQ stride devargs

Message ID 20211123183805.2905792-4-michaelba@nvidia.com (mailing list archive)
State Accepted, archived
Delegated to: Raslan Darawsheh
Headers
Series fix MPRQ prepare |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/github-robot: build success github build: passed
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS

Commit Message

Michael Baum Nov. 23, 2021, 6:38 p.m. UTC
From: Michael Baum <michaelba@nvidia.com>

In Multy-Packet RQ creation, the user can choose the number of strides
and their size in bytes. The user updates it using specific devargs for
both of these parameters.
The above two parameters determine the size of the WQE which is actually
their product of multiplication.

If the user selects values that are not in the supported range, the PMD
changes them to default values. However, apart from the range
limitations for each parameter individually there is also a minimum
value on their multiplication. When the user selects values that their
multiplication are lower than minimum value, no adjustment is made and
the creation of the WQE fails.

This patch adds an adjustment in these cases as well. When the user
selects values whose multiplication is lower than the minimum, they are
replaced with the default values.

Fixes: ecb160456aed ("net/mlx5: add device parameter for MPRQ stride size")
Cc: stable@dpdk.org

Signed-off-by: Michael Baum <michaelba@nvidia.com>
Acked-by: Matan Azrad <matan@nvidia.com>
---
 drivers/net/mlx5/linux/mlx5_os.c |  56 +++------
 drivers/net/mlx5/mlx5.h          |   4 +
 drivers/net/mlx5/mlx5_rxq.c      | 209 +++++++++++++++++++++----------
 3 files changed, 159 insertions(+), 110 deletions(-)
  

Comments

Raslan Darawsheh Nov. 23, 2021, 8:41 p.m. UTC | #1
Hi,

> -----Original Message-----
> From: Michael Baum <michaelba@nvidia.com>
> Sent: Tuesday, November 23, 2021 8:38 PM
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@nvidia.com>; Raslan Darawsheh
> <rasland@nvidia.com>; Slava Ovsiienko <viacheslavo@nvidia.com>; Michael
> Baum <michaelba@nvidia.com>; stable@dpdk.org
> Subject: [PATCH 3/3] net/mlx5: fix missing adjustment MPRQ stride devargs
> 
> From: Michael Baum <michaelba@nvidia.com>
> 
> In Multy-Packet RQ creation, the user can choose the number of strides and
> their size in bytes. The user updates it using specific devargs for both of
> these parameters.
> The above two parameters determine the size of the WQE which is actually
> their product of multiplication.
> 
> If the user selects values that are not in the supported range, the PMD
> changes them to default values. However, apart from the range limitations
> for each parameter individually there is also a minimum value on their
> multiplication. When the user selects values that their multiplication are
> lower than minimum value, no adjustment is made and the creation of the
> WQE fails.
> 
> This patch adds an adjustment in these cases as well. When the user selects
> values whose multiplication is lower than the minimum, they are replaced
> with the default values.
> 
> Fixes: ecb160456aed ("net/mlx5: add device parameter for MPRQ stride
> size")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Michael Baum <michaelba@nvidia.com>
> Acked-by: Matan Azrad <matan@nvidia.com>
> ---
>  drivers/net/mlx5/linux/mlx5_os.c |  56 +++------
>  drivers/net/mlx5/mlx5.h          |   4 +
>  drivers/net/mlx5/mlx5_rxq.c      | 209 +++++++++++++++++++++----------
>  3 files changed, 159 insertions(+), 110 deletions(-)
> 
> diff --git a/drivers/net/mlx5/linux/mlx5_os.c
> b/drivers/net/mlx5/linux/mlx5_os.c
> index 70472efc29..3e496d68ea 100644
> --- a/drivers/net/mlx5/linux/mlx5_os.c
> +++ b/drivers/net/mlx5/linux/mlx5_os.c
> @@ -881,10 +881,6 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  	unsigned int mpls_en = 0;
>  	unsigned int swp = 0;
>  	unsigned int mprq = 0;
> -	unsigned int mprq_min_stride_size_n = 0;
> -	unsigned int mprq_max_stride_size_n = 0;
> -	unsigned int mprq_min_stride_num_n = 0;
> -	unsigned int mprq_max_stride_num_n = 0;
>  	struct rte_ether_addr mac;
>  	char name[RTE_ETH_NAME_MAX_LEN];
>  	int own_domain_id = 0;
> @@ -1039,15 +1035,17 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  			mprq_caps.max_single_wqe_log_num_of_strides);
>  		DRV_LOG(DEBUG, "\tsupported_qpts: %d",
>  			mprq_caps.supported_qpts);
> +		DRV_LOG(DEBUG, "\tmin_stride_wqe_log_size: %d",
> +			config->mprq.log_min_stride_wqe_size);
>  		DRV_LOG(DEBUG, "device supports Multi-Packet RQ");
>  		mprq = 1;
> -		mprq_min_stride_size_n =
> +		config->mprq.log_min_stride_size =
>  			mprq_caps.min_single_stride_log_num_of_bytes;
> -		mprq_max_stride_size_n =
> +		config->mprq.log_max_stride_size =
>  			mprq_caps.max_single_stride_log_num_of_bytes;
> -		mprq_min_stride_num_n =
> +		config->mprq.log_min_stride_num =
>  			mprq_caps.min_single_wqe_log_num_of_strides;
> -		mprq_max_stride_num_n =
> +		config->mprq.log_max_stride_num =
>  			mprq_caps.max_single_wqe_log_num_of_strides;
>  	}
>  #endif
> @@ -1548,36 +1546,7 @@ mlx5_dev_spawn(struct rte_device *dpdk_dev,
>  		config->hw_fcs_strip = 0;
>  	DRV_LOG(DEBUG, "FCS stripping configuration is %ssupported",
>  		(config->hw_fcs_strip ? "" : "not "));
> -	if (config->mprq.enabled && mprq) {
> -		if (config->mprq.log_stride_num &&
> -		    (config->mprq.log_stride_num >
> mprq_max_stride_num_n ||
> -		     config->mprq.log_stride_num <
> mprq_min_stride_num_n)) {
> -			config->mprq.log_stride_num =
> -
> RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM,
> -					       mprq_min_stride_num_n),
> -				       mprq_max_stride_num_n);
> -			DRV_LOG(WARNING,
> -				"the number of strides"
> -				" for Multi-Packet RQ is out of range,"
> -				" setting default value (%u)",
> -				1 << config->mprq.log_stride_num);
> -		}
> -		if (config->mprq.log_stride_size &&
> -		    (config->mprq.log_stride_size > mprq_max_stride_size_n
> ||
> -		     config->mprq.log_stride_size < mprq_min_stride_size_n))
> {
> -			config->mprq.log_stride_size =
> -
> RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE,
> -					      mprq_min_stride_size_n),
> -				      mprq_max_stride_size_n);
> -			DRV_LOG(WARNING,
> -				"the size of a stride"
> -				" for Multi-Packet RQ is out of range,"
> -				" setting default value (%u)",
> -				1 << config->mprq.log_stride_size);
> -		}
> -		config->mprq.log_min_stride_size =
> mprq_min_stride_size_n;
> -		config->mprq.log_max_stride_size =
> mprq_max_stride_size_n;
> -	} else if (config->mprq.enabled && !mprq) {
> +	if (config->mprq.enabled && !mprq) {
>  		DRV_LOG(WARNING, "Multi-Packet RQ isn't supported");
>  		config->mprq.enabled = 0;
>  	}
> @@ -2068,7 +2037,8 @@ mlx5_device_bond_pci_match(const char
> *ibdev_name,  }
> 
>  static void
> -mlx5_os_config_default(struct mlx5_dev_config *config)
> +mlx5_os_config_default(struct mlx5_dev_config *config,
> +		       struct mlx5_common_dev_config *cconf)
>  {
>  	memset(config, 0, sizeof(*config));
>  	config->mps = MLX5_ARG_UNSET;
> @@ -2080,6 +2050,10 @@ mlx5_os_config_default(struct mlx5_dev_config
> *config)
>  	config->vf_nl_en = 1;
>  	config->mprq.max_memcpy_len =
> MLX5_MPRQ_MEMCPY_DEFAULT_LEN;
>  	config->mprq.min_rxqs_num = MLX5_MPRQ_MIN_RXQS;
> +	config->mprq.log_min_stride_wqe_size = cconf->devx ?
> +					cconf-
> >hca_attr.log_min_stride_wqe_sz :
> +
> 	MLX5_MPRQ_LOG_MIN_STRIDE_WQE_SIZE;
> +	config->mprq.log_stride_num =
> MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM;
>  	config->dv_esw_en = 1;
>  	config->dv_flow_en = 1;
>  	config->decap_en = 1;
> @@ -2496,7 +2470,7 @@ mlx5_os_pci_probe_pf(struct
> mlx5_common_device *cdev,
>  		uint32_t restore;
> 
>  		/* Default configuration. */
> -		mlx5_os_config_default(&dev_config);
> +		mlx5_os_config_default(&dev_config, &cdev->config);
>  		dev_config.vf = dev_config_vf;
>  		list[i].eth_dev = mlx5_dev_spawn(cdev->dev, &list[i],
>  						 &dev_config, &eth_da);
> @@ -2666,7 +2640,7 @@ mlx5_os_auxiliary_probe(struct
> mlx5_common_device *cdev)
>  	if (ret != 0)
>  		return ret;
>  	/* Set default config data. */
> -	mlx5_os_config_default(&config);
> +	mlx5_os_config_default(&config, &cdev->config);
>  	config.sf = 1;
>  	/* Init spawn data. */
>  	spawn.max_port = 1;
> diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h index
> 4ba90db816..c01fb9566e 100644
> --- a/drivers/net/mlx5/mlx5.h
> +++ b/drivers/net/mlx5/mlx5.h
> @@ -279,6 +279,10 @@ struct mlx5_dev_config {
>  		unsigned int log_stride_size; /* Log size of a stride. */
>  		unsigned int log_min_stride_size; /* Log min size of a
> stride.*/
>  		unsigned int log_max_stride_size; /* Log max size of a
> stride.*/
> +		unsigned int log_min_stride_num; /* Log min num of strides.
> */
> +		unsigned int log_max_stride_num; /* Log max num of
> strides. */
> +		unsigned int log_min_stride_wqe_size;
> +		/* Log min WQE size, (size of single stride)*(num of
> strides).*/
>  		unsigned int max_memcpy_len;
>  		/* Maximum packet size to memcpy Rx packets. */
>  		unsigned int min_rxqs_num;
> diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
> index e76bfaa000..891ac3d874 100644
> --- a/drivers/net/mlx5/mlx5_rxq.c
> +++ b/drivers/net/mlx5/mlx5_rxq.c
> @@ -1528,6 +1528,126 @@ mlx5_max_lro_msg_size_adjust(struct
> rte_eth_dev *dev, uint16_t idx,
>  		priv->max_lro_msg_size * MLX5_LRO_SEG_CHUNK_SIZE);  }
> 
> +/**
> + * Prepare both size and number of stride for Multi-Packet RQ.
> + *
> + * @param dev
> + *   Pointer to Ethernet device.
> + * @param idx
> + *   RX queue index.
> + * @param desc
> + *   Number of descriptors to configure in queue.
> + * @param rx_seg_en
> + *   Indicator if Rx segment enables, if so Multi-Packet RQ doesn't enable.
> + * @param min_mbuf_size
> + *   Non scatter min mbuf size, max_rx_pktlen plus overhead.
> + * @param actual_log_stride_num
> + *   Log number of strides to configure for this queue.
> + * @param actual_log_stride_size
> + *   Log stride size to configure for this queue.
> + *
> + * @return
> + *   0 if Multi-Packet RQ is supported, otherwise -1.
> + */
> +static int
> +mlx5_mprq_prepare(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc,
> +		  bool rx_seg_en, uint32_t min_mbuf_size,
> +		  uint32_t *actual_log_stride_num,
> +		  uint32_t *actual_log_stride_size)
> +{
> +	struct mlx5_priv *priv = dev->data->dev_private;
> +	struct mlx5_dev_config *config = &priv->config;
> +	uint32_t log_min_stride_num = config->mprq.log_min_stride_num;
> +	uint32_t log_max_stride_num = config->mprq.log_max_stride_num;
> +	uint32_t log_def_stride_num =
> +
> 	RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM,
> +					log_min_stride_num),
> +				log_max_stride_num);
> +	uint32_t log_min_stride_size = config->mprq.log_min_stride_size;
> +	uint32_t log_max_stride_size = config->mprq.log_max_stride_size;
> +	uint32_t log_def_stride_size =
> +
> 	RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE,
> +					log_min_stride_size),
> +				log_max_stride_size);
> +	uint32_t log_stride_wqe_size;
> +
> +	if (mlx5_check_mprq_support(dev) != 1 || rx_seg_en)
> +		goto unsupport;
typo in this label name, will fix during integration.
[..]
Kindest regards
Raslan Darawsheh
  
Ferruh Yigit Dec. 7, 2021, 1:40 p.m. UTC | #2
On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
> From: Michael Baum<michaelba@nvidia.com>
> 
> In Multy-Packet RQ creation, the user can choose the number of strides

Multi-Packet ?

> and their size in bytes. The user updates it using specific devargs for
> both of these parameters.
> The above two parameters determine the size of the WQE which is actually
> their product of multiplication.
> 
> If the user selects values that are not in the supported range, the PMD
> changes them to default values. However, apart from the range
> limitations for each parameter individually there is also a minimum
> value on their multiplication. When the user selects values that their
> multiplication are lower than minimum value, no adjustment is made and
> the creation of the WQE fails.
> > This patch adds an adjustment in these cases as well. When the user
> selects values whose multiplication is lower than the minimum, they are
> replaced with the default values.
> 
> Fixes: ecb160456aed ("net/mlx5: add device parameter for MPRQ stride size")
> Cc:stable@dpdk.org
> 

Again, not sure if we can backport this patch, this looks a behavior change more
than a fix.

Previously if the user provided values ends up being invalid, PMD seems
returning error.
With this patch, instead of returning error PMD prefers to use default
values and doesn't return error.

I am not sure if it is correct thing to ignore (adjust) user provided values,
but that can be up to the PMD as long as the behavior is documented.

But I think it is wrong to backport the behavior change.

> Signed-off-by: Michael Baum<michaelba@nvidia.com>
> Acked-by: Matan Azrad<matan@nvidia.com>
> ---
  
Michael Baum Dec. 8, 2021, 12:52 p.m. UTC | #3
On 12/07/2021 3:41 PM, ferruh.yigit@intel.com wrote: 
> 
> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
> > From: Michael Baum<michaelba@nvidia.com>
> >
> > In Multy-Packet RQ creation, the user can choose the number of strides
> 
> Multi-Packet ?

Yes, you're right. It should have been Multi-Packet, thank you for that.

> 
> > and their size in bytes. The user updates it using specific devargs
> > for both of these parameters.
> > The above two parameters determine the size of the WQE which is
> > actually their product of multiplication.
> >
> > If the user selects values that are not in the supported range, the
> > PMD changes them to default values. However, apart from the range
> > limitations for each parameter individually there is also a minimum
> > value on their multiplication. When the user selects values that their
> > multiplication are lower than minimum value, no adjustment is made and
> > the creation of the WQE fails.
> > > This patch adds an adjustment in these cases as well. When the user
> > selects values whose multiplication is lower than the minimum, they
> > are replaced with the default values.
> >
> > Fixes: ecb160456aed ("net/mlx5: add device parameter for MPRQ stride
> > size") Cc:stable@dpdk.org
> >
> 
> Again, not sure if we can backport this patch, this looks a behavior change
> more than a fix.
> 
> Previously if the user provided values ends up being invalid, PMD seems
> returning error.
> With this patch, instead of returning error PMD prefers to use default values
> and doesn't return error.

It isn't behavior change.
It existed before, except that it is concentrated into one function.

> 
> I am not sure if it is correct thing to ignore (adjust) user provided values, but
> that can be up to the PMD as long as the behavior is documented.

Adjustment is the likely thing to do because the range depends on the device and the user does not necessarily know it.
This behavior is documented here https://doc.dpdk.org/guides/nics/mlx5.html#run-time-configuration (Run-time configuration -> Driver options -> mprq_log_stride_num/size)

> 
> But I think it is wrong to backport the behavior change.
> 
> > Signed-off-by: Michael Baum<michaelba@nvidia.com>
> > Acked-by: Matan Azrad<matan@nvidia.com>
> > ---
  
Ferruh Yigit Dec. 8, 2021, 2 p.m. UTC | #4
On 12/8/2021 12:52 PM, Michael Baum wrote:
> 
> On 12/07/2021 3:41 PM, ferruh.yigit@intel.com wrote:
>>
>> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
>>> From: Michael Baum<michaelba@nvidia.com>
>>>
>>> In Multy-Packet RQ creation, the user can choose the number of strides
>>
>> Multi-Packet ?
> 
> Yes, you're right. It should have been Multi-Packet, thank you for that.
> 
>>
>>> and their size in bytes. The user updates it using specific devargs
>>> for both of these parameters.
>>> The above two parameters determine the size of the WQE which is
>>> actually their product of multiplication.
>>>
>>> If the user selects values that are not in the supported range, the
>>> PMD changes them to default values. However, apart from the range
>>> limitations for each parameter individually there is also a minimum
>>> value on their multiplication. When the user selects values that their
>>> multiplication are lower than minimum value, no adjustment is made and
>>> the creation of the WQE fails.
>>>> This patch adds an adjustment in these cases as well. When the user
>>> selects values whose multiplication is lower than the minimum, they
>>> are replaced with the default values.
>>>
>>> Fixes: ecb160456aed ("net/mlx5: add device parameter for MPRQ stride
>>> size") Cc:stable@dpdk.org
>>>
>>
>> Again, not sure if we can backport this patch, this looks a behavior change
>> more than a fix.
>>
>> Previously if the user provided values ends up being invalid, PMD seems
>> returning error.
>> With this patch, instead of returning error PMD prefers to use default values
>> and doesn't return error.
> 
> It isn't behavior change.
> It existed before, except that it is concentrated into one function.
> 
>>
>> I am not sure if it is correct thing to ignore (adjust) user provided values, but
>> that can be up to the PMD as long as the behavior is documented.
> 
> Adjustment is the likely thing to do because the range depends on the device and the user does not necessarily know it.
> This behavior is documented here https://doc.dpdk.org/guides/nics/mlx5.html#run-time-configuration (Run-time configuration -> Driver options -> mprq_log_stride_num/size)
> 

It is documented that adjustments will be done if any specific argument
is not in the range of the device capability.

It is not clear what will happen if the calculated value from both variables
are not valid.
If it is not documented before, and previously it was returning error,
now adjusting values to make it work looks like behavior change to me.

This is more of a process question, than technical detail in the driver,
so @Luca, @Kevin, @Christian, can you please comment? I will follow your
suggestion.


>>
>> But I think it is wrong to backport the behavior change.
>>
>>> Signed-off-by: Michael Baum<michaelba@nvidia.com>
>>> Acked-by: Matan Azrad<matan@nvidia.com>
>>> ---
>
  
Matan Azrad Dec. 8, 2021, 3:40 p.m. UTC | #5
Hi Ferruh

Thanks for the review.

Please see inside some clarifications.

From: Ferruh Yigit
> On 12/8/2021 12:52 PM, Michael Baum wrote:
> >
> > On 12/07/2021 3:41 PM, ferruh.yigit@intel.com wrote:
> >>
> >> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
> >>> From: Michael Baum<michaelba@nvidia.com>
> >>>
> >>> In Multy-Packet RQ creation, the user can choose the number of
> >>> strides
> >>
> >> Multi-Packet ?
> >
> > Yes, you're right. It should have been Multi-Packet, thank you for that.
> >
> >>
> >>> and their size in bytes. The user updates it using specific devargs
> >>> for both of these parameters.
> >>> The above two parameters determine the size of the WQE which is
> >>> actually their product of multiplication.
> >>>
> >>> If the user selects values that are not in the supported range, the
> >>> PMD changes them to default values. However, apart from the range
> >>> limitations for each parameter individually there is also a minimum
> >>> value on their multiplication. When the user selects values that
> >>> their multiplication are lower than minimum value, no adjustment is
> >>> made and the creation of the WQE fails.
> >>>> This patch adds an adjustment in these cases as well. When the user
> >>> selects values whose multiplication is lower than the minimum, they
> >>> are replaced with the default values.
> >>>
> >>> Fixes: ecb160456aed ("net/mlx5: add device parameter for MPRQ stride
> >>> size") Cc:stable@dpdk.org
> >>>
> >>
> >> Again, not sure if we can backport this patch, this looks a behavior
> >> change more than a fix.
> >>
> >> Previously if the user provided values ends up being invalid, PMD
> >> seems returning error.
> >> With this patch, instead of returning error PMD prefers to use
> >> default values and doesn't return error.
> >
> > It isn't behavior change.
> > It existed before, except that it is concentrated into one function.
> >
> >>
> >> I am not sure if it is correct thing to ignore (adjust) user provided
> >> values, but that can be up to the PMD as long as the behavior is
> documented.
> >
> > Adjustment is the likely thing to do because the range depends on the
> device and the user does not necessarily know it.
> > This behavior is documented here
> > https://doc.dpdk.org/guides/nics/mlx5.html#run-time-configuration
> > (Run-time configuration -> Driver options -> mprq_log_stride_num/size)
> >
> 
> It is documented that adjustments will be done if any specific argument is
> not in the range of the device capability.
> 
> It is not clear what will happen if the calculated value from both variables are
> not valid.

The driver should adjust it to a legal value.

> If it is not documented before, and previously it was returning error, now
> adjusting values to make it work looks like behavior change to me.

The driver should not return an error - the driver should adjust to a legal value in case of illegal values by the user.
It is documented in the devargs description.

Not behavior change but a bug fix; previously, the adjustment may return an error(which is a bug) or cause unexpected behavior in the HW(which is an old FW bug).
Now, no error, no unexpected behavior - bug should be fixed for any FW version. 

> This is more of a process question, than technical detail in the driver, so
> @Luca, @Kevin, @Christian, can you please comment? I will follow your
> suggestion.
> 
> 
> >>
> >> But I think it is wrong to backport the behavior change.
> >>
> >>> Signed-off-by: Michael Baum<michaelba@nvidia.com>
> >>> Acked-by: Matan Azrad<matan@nvidia.com>
> >>> ---
> >
  
Kevin Traynor Dec. 9, 2021, 12:33 p.m. UTC | #6
On 08/12/2021 15:40, Matan Azrad wrote:
> Hi Ferruh
> 
> Thanks for the review.
> 
> Please see inside some clarifications.
> 
> From: Ferruh Yigit
>> On 12/8/2021 12:52 PM, Michael Baum wrote:
>>>
>>> On 12/07/2021 3:41 PM, ferruh.yigit@intel.com wrote:
>>>>
>>>> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
>>>>> From: Michael Baum<michaelba@nvidia.com>
>>>>>
>>>>> In Multy-Packet RQ creation, the user can choose the number of
>>>>> strides
>>>>
>>>> Multi-Packet ?
>>>
>>> Yes, you're right. It should have been Multi-Packet, thank you for that.
>>>
>>>>
>>>>> and their size in bytes. The user updates it using specific devargs
>>>>> for both of these parameters.
>>>>> The above two parameters determine the size of the WQE which is
>>>>> actually their product of multiplication.
>>>>>
>>>>> If the user selects values that are not in the supported range, the
>>>>> PMD changes them to default values. However, apart from the range
>>>>> limitations for each parameter individually there is also a minimum
>>>>> value on their multiplication. When the user selects values that
>>>>> their multiplication are lower than minimum value, no adjustment is
>>>>> made and the creation of the WQE fails.
>>>>>> This patch adds an adjustment in these cases as well. When the user
>>>>> selects values whose multiplication is lower than the minimum, they
>>>>> are replaced with the default values.
>>>>>
>>>>> Fixes: ecb160456aed ("net/mlx5: add device parameter for MPRQ stride
>>>>> size") Cc:stable@dpdk.org
>>>>>
>>>>
>>>> Again, not sure if we can backport this patch, this looks a behavior
>>>> change more than a fix.
>>>>
>>>> Previously if the user provided values ends up being invalid, PMD
>>>> seems returning error.
>>>> With this patch, instead of returning error PMD prefers to use
>>>> default values and doesn't return error.
>>>
>>> It isn't behavior change.
>>> It existed before, except that it is concentrated into one function.
>>>
>>>>
>>>> I am not sure if it is correct thing to ignore (adjust) user provided
>>>> values, but that can be up to the PMD as long as the behavior is
>> documented.
>>>
>>> Adjustment is the likely thing to do because the range depends on the
>> device and the user does not necessarily know it.
>>> This behavior is documented here
>>> https://doc.dpdk.org/guides/nics/mlx5.html#run-time-configuration
>>> (Run-time configuration -> Driver options -> mprq_log_stride_num/size)
>>>
>>
>> It is documented that adjustments will be done if any specific argument is
>> not in the range of the device capability.
>>
>> It is not clear what will happen if the calculated value from both variables are
>> not valid.
> 
> The driver should adjust it to a legal value.
> 
>> If it is not documented before, and previously it was returning error, now
>> adjusting values to make it work looks like behavior change to me.
> 
> The driver should not return an error - the driver should adjust to a legal value in case of illegal values by the user.
> It is documented in the devargs description.
> 
> Not behavior change but a bug fix; previously, the adjustment may return an error(which is a bug) or cause unexpected behavior in the HW(which is an old FW bug).
> Now, no error, no unexpected behavior - bug should be fixed for any FW version.
> 

I can understand both arguments. It is a behaviour change as the user 
will see a different behaviour for a given set of values.

However, each parameter is already validated and defaults are provided 
as backup. The combination not being checked could be seen a piece of 
missed validation for those values and a bug. In this case, given it is 
unlikely any user would be happy with the WQE creation failure, i think 
it is ok to backport the missing validation/adjustment.

>> This is more of a process question, than technical detail in the driver, so
>> @Luca, @Kevin, @Christian, can you please comment? I will follow your
>> suggestion.
>>

Thanks for raising it Ferruh.

Kevin.

>>
>>>>
>>>> But I think it is wrong to backport the behavior change.
>>>>
>>>>> Signed-off-by: Michael Baum<michaelba@nvidia.com>
>>>>> Acked-by: Matan Azrad<matan@nvidia.com>
>>>>> ---
>>>
>
  
Ferruh Yigit Dec. 10, 2021, 4:58 p.m. UTC | #7
On 12/9/2021 12:33 PM, Kevin Traynor wrote:
> On 08/12/2021 15:40, Matan Azrad wrote:
>> Hi Ferruh
>>
>> Thanks for the review.
>>
>> Please see inside some clarifications.
>>
>> From: Ferruh Yigit
>>> On 12/8/2021 12:52 PM, Michael Baum wrote:
>>>>
>>>> On 12/07/2021 3:41 PM, ferruh.yigit@intel.com wrote:
>>>>>
>>>>> On 11/23/2021 6:38 PM, michaelba@nvidia.com wrote:
>>>>>> From: Michael Baum<michaelba@nvidia.com>
>>>>>>
>>>>>> In Multy-Packet RQ creation, the user can choose the number of
>>>>>> strides
>>>>>
>>>>> Multi-Packet ?
>>>>
>>>> Yes, you're right. It should have been Multi-Packet, thank you for that.
>>>>
>>>>>
>>>>>> and their size in bytes. The user updates it using specific devargs
>>>>>> for both of these parameters.
>>>>>> The above two parameters determine the size of the WQE which is
>>>>>> actually their product of multiplication.
>>>>>>
>>>>>> If the user selects values that are not in the supported range, the
>>>>>> PMD changes them to default values. However, apart from the range
>>>>>> limitations for each parameter individually there is also a minimum
>>>>>> value on their multiplication. When the user selects values that
>>>>>> their multiplication are lower than minimum value, no adjustment is
>>>>>> made and the creation of the WQE fails.
>>>>>>> This patch adds an adjustment in these cases as well. When the user
>>>>>> selects values whose multiplication is lower than the minimum, they
>>>>>> are replaced with the default values.
>>>>>>
>>>>>> Fixes: ecb160456aed ("net/mlx5: add device parameter for MPRQ stride
>>>>>> size") Cc:stable@dpdk.org
>>>>>>
>>>>>
>>>>> Again, not sure if we can backport this patch, this looks a behavior
>>>>> change more than a fix.
>>>>>
>>>>> Previously if the user provided values ends up being invalid, PMD
>>>>> seems returning error.
>>>>> With this patch, instead of returning error PMD prefers to use
>>>>> default values and doesn't return error.
>>>>
>>>> It isn't behavior change.
>>>> It existed before, except that it is concentrated into one function.
>>>>
>>>>>
>>>>> I am not sure if it is correct thing to ignore (adjust) user provided
>>>>> values, but that can be up to the PMD as long as the behavior is
>>> documented.
>>>>
>>>> Adjustment is the likely thing to do because the range depends on the
>>> device and the user does not necessarily know it.
>>>> This behavior is documented here
>>>> https://doc.dpdk.org/guides/nics/mlx5.html#run-time-configuration
>>>> (Run-time configuration -> Driver options -> mprq_log_stride_num/size)
>>>>
>>>
>>> It is documented that adjustments will be done if any specific argument is
>>> not in the range of the device capability.
>>>
>>> It is not clear what will happen if the calculated value from both variables are
>>> not valid.
>>
>> The driver should adjust it to a legal value.
>>
>>> If it is not documented before, and previously it was returning error, now
>>> adjusting values to make it work looks like behavior change to me.
>>
>> The driver should not return an error - the driver should adjust to a legal value in case of illegal values by the user.
>> It is documented in the devargs description.
>>
>> Not behavior change but a bug fix; previously, the adjustment may return an error(which is a bug) or cause unexpected behavior in the HW(which is an old FW bug).
>> Now, no error, no unexpected behavior - bug should be fixed for any FW version.
>>
> 
> I can understand both arguments. It is a behaviour change as the user will see a different behaviour for a given set of values.
> 
> However, each parameter is already validated and defaults are provided as backup. The combination not being checked could be seen a piece of missed validation for those values and a bug. In this case, given it is unlikely any user would be happy with the WQE creation failure, i think it is ok to backport the missing validation/adjustment.
> 
>>> This is more of a process question, than technical detail in the driver, so
>>> @Luca, @Kevin, @Christian, can you please comment? I will follow your
>>> suggestion.
>>>
> 
> Thanks for raising it Ferruh.
> 


Proceeding with patch then, updated fixes tag for 2/3 in next-net.
  

Patch

diff --git a/drivers/net/mlx5/linux/mlx5_os.c b/drivers/net/mlx5/linux/mlx5_os.c
index 70472efc29..3e496d68ea 100644
--- a/drivers/net/mlx5/linux/mlx5_os.c
+++ b/drivers/net/mlx5/linux/mlx5_os.c
@@ -881,10 +881,6 @@  mlx5_dev_spawn(struct rte_device *dpdk_dev,
 	unsigned int mpls_en = 0;
 	unsigned int swp = 0;
 	unsigned int mprq = 0;
-	unsigned int mprq_min_stride_size_n = 0;
-	unsigned int mprq_max_stride_size_n = 0;
-	unsigned int mprq_min_stride_num_n = 0;
-	unsigned int mprq_max_stride_num_n = 0;
 	struct rte_ether_addr mac;
 	char name[RTE_ETH_NAME_MAX_LEN];
 	int own_domain_id = 0;
@@ -1039,15 +1035,17 @@  mlx5_dev_spawn(struct rte_device *dpdk_dev,
 			mprq_caps.max_single_wqe_log_num_of_strides);
 		DRV_LOG(DEBUG, "\tsupported_qpts: %d",
 			mprq_caps.supported_qpts);
+		DRV_LOG(DEBUG, "\tmin_stride_wqe_log_size: %d",
+			config->mprq.log_min_stride_wqe_size);
 		DRV_LOG(DEBUG, "device supports Multi-Packet RQ");
 		mprq = 1;
-		mprq_min_stride_size_n =
+		config->mprq.log_min_stride_size =
 			mprq_caps.min_single_stride_log_num_of_bytes;
-		mprq_max_stride_size_n =
+		config->mprq.log_max_stride_size =
 			mprq_caps.max_single_stride_log_num_of_bytes;
-		mprq_min_stride_num_n =
+		config->mprq.log_min_stride_num =
 			mprq_caps.min_single_wqe_log_num_of_strides;
-		mprq_max_stride_num_n =
+		config->mprq.log_max_stride_num =
 			mprq_caps.max_single_wqe_log_num_of_strides;
 	}
 #endif
@@ -1548,36 +1546,7 @@  mlx5_dev_spawn(struct rte_device *dpdk_dev,
 		config->hw_fcs_strip = 0;
 	DRV_LOG(DEBUG, "FCS stripping configuration is %ssupported",
 		(config->hw_fcs_strip ? "" : "not "));
-	if (config->mprq.enabled && mprq) {
-		if (config->mprq.log_stride_num &&
-		    (config->mprq.log_stride_num > mprq_max_stride_num_n ||
-		     config->mprq.log_stride_num < mprq_min_stride_num_n)) {
-			config->mprq.log_stride_num =
-			       RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM,
-					       mprq_min_stride_num_n),
-				       mprq_max_stride_num_n);
-			DRV_LOG(WARNING,
-				"the number of strides"
-				" for Multi-Packet RQ is out of range,"
-				" setting default value (%u)",
-				1 << config->mprq.log_stride_num);
-		}
-		if (config->mprq.log_stride_size &&
-		    (config->mprq.log_stride_size > mprq_max_stride_size_n ||
-		     config->mprq.log_stride_size < mprq_min_stride_size_n)) {
-			config->mprq.log_stride_size =
-			      RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE,
-					      mprq_min_stride_size_n),
-				      mprq_max_stride_size_n);
-			DRV_LOG(WARNING,
-				"the size of a stride"
-				" for Multi-Packet RQ is out of range,"
-				" setting default value (%u)",
-				1 << config->mprq.log_stride_size);
-		}
-		config->mprq.log_min_stride_size = mprq_min_stride_size_n;
-		config->mprq.log_max_stride_size = mprq_max_stride_size_n;
-	} else if (config->mprq.enabled && !mprq) {
+	if (config->mprq.enabled && !mprq) {
 		DRV_LOG(WARNING, "Multi-Packet RQ isn't supported");
 		config->mprq.enabled = 0;
 	}
@@ -2068,7 +2037,8 @@  mlx5_device_bond_pci_match(const char *ibdev_name,
 }
 
 static void
-mlx5_os_config_default(struct mlx5_dev_config *config)
+mlx5_os_config_default(struct mlx5_dev_config *config,
+		       struct mlx5_common_dev_config *cconf)
 {
 	memset(config, 0, sizeof(*config));
 	config->mps = MLX5_ARG_UNSET;
@@ -2080,6 +2050,10 @@  mlx5_os_config_default(struct mlx5_dev_config *config)
 	config->vf_nl_en = 1;
 	config->mprq.max_memcpy_len = MLX5_MPRQ_MEMCPY_DEFAULT_LEN;
 	config->mprq.min_rxqs_num = MLX5_MPRQ_MIN_RXQS;
+	config->mprq.log_min_stride_wqe_size = cconf->devx ?
+					cconf->hca_attr.log_min_stride_wqe_sz :
+					MLX5_MPRQ_LOG_MIN_STRIDE_WQE_SIZE;
+	config->mprq.log_stride_num = MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM;
 	config->dv_esw_en = 1;
 	config->dv_flow_en = 1;
 	config->decap_en = 1;
@@ -2496,7 +2470,7 @@  mlx5_os_pci_probe_pf(struct mlx5_common_device *cdev,
 		uint32_t restore;
 
 		/* Default configuration. */
-		mlx5_os_config_default(&dev_config);
+		mlx5_os_config_default(&dev_config, &cdev->config);
 		dev_config.vf = dev_config_vf;
 		list[i].eth_dev = mlx5_dev_spawn(cdev->dev, &list[i],
 						 &dev_config, &eth_da);
@@ -2666,7 +2640,7 @@  mlx5_os_auxiliary_probe(struct mlx5_common_device *cdev)
 	if (ret != 0)
 		return ret;
 	/* Set default config data. */
-	mlx5_os_config_default(&config);
+	mlx5_os_config_default(&config, &cdev->config);
 	config.sf = 1;
 	/* Init spawn data. */
 	spawn.max_port = 1;
diff --git a/drivers/net/mlx5/mlx5.h b/drivers/net/mlx5/mlx5.h
index 4ba90db816..c01fb9566e 100644
--- a/drivers/net/mlx5/mlx5.h
+++ b/drivers/net/mlx5/mlx5.h
@@ -279,6 +279,10 @@  struct mlx5_dev_config {
 		unsigned int log_stride_size; /* Log size of a stride. */
 		unsigned int log_min_stride_size; /* Log min size of a stride.*/
 		unsigned int log_max_stride_size; /* Log max size of a stride.*/
+		unsigned int log_min_stride_num; /* Log min num of strides. */
+		unsigned int log_max_stride_num; /* Log max num of strides. */
+		unsigned int log_min_stride_wqe_size;
+		/* Log min WQE size, (size of single stride)*(num of strides).*/
 		unsigned int max_memcpy_len;
 		/* Maximum packet size to memcpy Rx packets. */
 		unsigned int min_rxqs_num;
diff --git a/drivers/net/mlx5/mlx5_rxq.c b/drivers/net/mlx5/mlx5_rxq.c
index e76bfaa000..891ac3d874 100644
--- a/drivers/net/mlx5/mlx5_rxq.c
+++ b/drivers/net/mlx5/mlx5_rxq.c
@@ -1528,6 +1528,126 @@  mlx5_max_lro_msg_size_adjust(struct rte_eth_dev *dev, uint16_t idx,
 		priv->max_lro_msg_size * MLX5_LRO_SEG_CHUNK_SIZE);
 }
 
+/**
+ * Prepare both size and number of stride for Multi-Packet RQ.
+ *
+ * @param dev
+ *   Pointer to Ethernet device.
+ * @param idx
+ *   RX queue index.
+ * @param desc
+ *   Number of descriptors to configure in queue.
+ * @param rx_seg_en
+ *   Indicator if Rx segment enables, if so Multi-Packet RQ doesn't enable.
+ * @param min_mbuf_size
+ *   Non scatter min mbuf size, max_rx_pktlen plus overhead.
+ * @param actual_log_stride_num
+ *   Log number of strides to configure for this queue.
+ * @param actual_log_stride_size
+ *   Log stride size to configure for this queue.
+ *
+ * @return
+ *   0 if Multi-Packet RQ is supported, otherwise -1.
+ */
+static int
+mlx5_mprq_prepare(struct rte_eth_dev *dev, uint16_t idx, uint16_t desc,
+		  bool rx_seg_en, uint32_t min_mbuf_size,
+		  uint32_t *actual_log_stride_num,
+		  uint32_t *actual_log_stride_size)
+{
+	struct mlx5_priv *priv = dev->data->dev_private;
+	struct mlx5_dev_config *config = &priv->config;
+	uint32_t log_min_stride_num = config->mprq.log_min_stride_num;
+	uint32_t log_max_stride_num = config->mprq.log_max_stride_num;
+	uint32_t log_def_stride_num =
+			RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM,
+					log_min_stride_num),
+				log_max_stride_num);
+	uint32_t log_min_stride_size = config->mprq.log_min_stride_size;
+	uint32_t log_max_stride_size = config->mprq.log_max_stride_size;
+	uint32_t log_def_stride_size =
+			RTE_MIN(RTE_MAX(MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE,
+					log_min_stride_size),
+				log_max_stride_size);
+	uint32_t log_stride_wqe_size;
+
+	if (mlx5_check_mprq_support(dev) != 1 || rx_seg_en)
+		goto unsupport;
+	/* Checks if chosen number of strides is in supported range. */
+	if (config->mprq.log_stride_num > log_max_stride_num ||
+	    config->mprq.log_stride_num < log_min_stride_num) {
+		*actual_log_stride_num = log_def_stride_num;
+		DRV_LOG(WARNING,
+			"Port %u Rx queue %u number of strides for Multi-Packet RQ is out of range, setting default value (%u)",
+			dev->data->port_id, idx, RTE_BIT32(log_def_stride_num));
+	} else {
+		*actual_log_stride_num = config->mprq.log_stride_num;
+	}
+	if (config->mprq.log_stride_size) {
+		/* Checks if chosen size of stride is in supported range. */
+		if (config->mprq.log_stride_size > log_max_stride_size ||
+		    config->mprq.log_stride_size < log_min_stride_size) {
+			*actual_log_stride_size = log_def_stride_size;
+			DRV_LOG(WARNING,
+				"Port %u Rx queue %u size of a stride for Multi-Packet RQ is out of range, setting default value (%u)",
+				dev->data->port_id, idx,
+				RTE_BIT32(log_def_stride_size));
+		} else {
+			*actual_log_stride_size = config->mprq.log_stride_size;
+		}
+	} else {
+		if (min_mbuf_size <= RTE_BIT32(log_max_stride_size))
+			*actual_log_stride_size = log2above(min_mbuf_size);
+		else
+			goto unsupport;
+	}
+	log_stride_wqe_size = *actual_log_stride_num + *actual_log_stride_size;
+	/* Check if WQE buffer size is supported by hardware. */
+	if (log_stride_wqe_size < config->mprq.log_min_stride_wqe_size) {
+		*actual_log_stride_num = log_def_stride_num;
+		*actual_log_stride_size = log_def_stride_size;
+		DRV_LOG(WARNING,
+			"Port %u Rx queue %u size of WQE buffer for Multi-Packet RQ is too small, setting default values (stride_num_n=%u, stride_size_n=%u)",
+			dev->data->port_id, idx, RTE_BIT32(log_def_stride_num),
+			RTE_BIT32(log_def_stride_size));
+		log_stride_wqe_size = log_def_stride_num + log_def_stride_size;
+	}
+	MLX5_ASSERT(log_stride_wqe_size < config->mprq.log_min_stride_wqe_size);
+	if (desc <= RTE_BIT32(*actual_log_stride_num))
+		goto unsupport;
+	if (min_mbuf_size > RTE_BIT32(log_stride_wqe_size)) {
+		DRV_LOG(WARNING, "Port %u Rx queue %u "
+			"Multi-Packet RQ is unsupported, WQE buffer size (%u) "
+			"is smaller than min mbuf size (%u)",
+			dev->data->port_id, idx, RTE_BIT32(log_stride_wqe_size),
+			min_mbuf_size);
+		goto unsupport;
+	}
+	DRV_LOG(DEBUG, "Port %u Rx queue %u "
+		"Multi-Packet RQ is enabled strd_num_n = %u, strd_sz_n = %u",
+		dev->data->port_id, idx, RTE_BIT32(*actual_log_stride_num),
+		RTE_BIT32(*actual_log_stride_size));
+	return 0;
+unsupport:
+	if (config->mprq.enabled)
+		DRV_LOG(WARNING,
+			"Port %u MPRQ is requested but cannot be enabled\n"
+			" (requested: pkt_sz = %u, desc_num = %u,"
+			" rxq_num = %u, stride_sz = %u, stride_num = %u\n"
+			"  supported: min_rxqs_num = %u, min_buf_wqe_sz = %u"
+			" min_stride_sz = %u, max_stride_sz = %u).\n"
+			"Rx segment is %senable.",
+			dev->data->port_id, min_mbuf_size, desc, priv->rxqs_n,
+			RTE_BIT32(config->mprq.log_stride_size),
+			RTE_BIT32(config->mprq.log_stride_num),
+			config->mprq.min_rxqs_num,
+			RTE_BIT32(config->mprq.log_min_stride_wqe_size),
+			RTE_BIT32(config->mprq.log_min_stride_size),
+			RTE_BIT32(config->mprq.log_max_stride_size),
+			rx_seg_en ? "" : "not ");
+	return -1;
+}
+
 /**
  * Create a DPDK Rx queue.
  *
@@ -1565,33 +1685,28 @@  mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 							RTE_PKTMBUF_HEADROOM;
 	unsigned int max_lro_size = 0;
 	unsigned int first_mb_free_size = mb_len - RTE_PKTMBUF_HEADROOM;
-	const int mprq_en = mlx5_check_mprq_support(dev) > 0 && n_seg == 1 &&
-			    !rx_seg[0].offset && !rx_seg[0].length;
-	unsigned int log_mprq_stride_nums = config->mprq.log_stride_num ?
-		config->mprq.log_stride_num : MLX5_MPRQ_DEFAULT_LOG_STRIDE_NUM;
-	unsigned int log_mprq_stride_size = non_scatter_min_mbuf_size <=
-		RTE_BIT32(config->mprq.log_max_stride_size) ?
-		log2above(non_scatter_min_mbuf_size) :
-		MLX5_MPRQ_DEFAULT_LOG_STRIDE_SIZE;
-	unsigned int mprq_stride_cap = (config->mprq.log_stride_num ?
-					RTE_BIT32(config->mprq.log_stride_num) :
-					RTE_BIT32(log_mprq_stride_nums)) *
-				       (config->mprq.log_stride_size ?
-				       RTE_BIT32(config->mprq.log_stride_size) :
-					RTE_BIT32(log_mprq_stride_size));
+	uint32_t mprq_log_actual_stride_num = 0;
+	uint32_t mprq_log_actual_stride_size = 0;
+	bool rx_seg_en = n_seg != 1 || rx_seg[0].offset || rx_seg[0].length;
+	const int mprq_en = !mlx5_mprq_prepare(dev, idx, desc, rx_seg_en,
+					       non_scatter_min_mbuf_size,
+					       &mprq_log_actual_stride_num,
+					       &mprq_log_actual_stride_size);
 	/*
 	 * Always allocate extra slots, even if eventually
 	 * the vector Rx will not be used.
 	 */
 	uint16_t desc_n = desc + config->rx_vec_en * MLX5_VPMD_DESCS_PER_LOOP;
+	size_t alloc_size = sizeof(*tmpl) + desc_n * sizeof(struct rte_mbuf *);
 	const struct rte_eth_rxseg_split *qs_seg = rx_seg;
 	unsigned int tail_len;
 
-	tmpl = mlx5_malloc(MLX5_MEM_RTE | MLX5_MEM_ZERO,
-		sizeof(*tmpl) + desc_n * sizeof(struct rte_mbuf *) +
-		(!!mprq_en) *
-		(desc >> log_mprq_stride_nums) * sizeof(struct mlx5_mprq_buf *),
-		0, socket);
+	if (mprq_en) {
+		/* Trim the number of descs needed. */
+		desc >>= mprq_log_actual_stride_num;
+		alloc_size += desc * sizeof(struct mlx5_mprq_buf *);
+	}
+	tmpl = mlx5_malloc(MLX5_MEM_RTE | MLX5_MEM_ZERO, alloc_size, 0, socket);
 	if (!tmpl) {
 		rte_errno = ENOMEM;
 		return NULL;
@@ -1689,30 +1804,11 @@  mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 	tmpl->socket = socket;
 	if (dev->data->dev_conf.intr_conf.rxq)
 		tmpl->irq = 1;
-	/*
-	 * This Rx queue can be configured as a Multi-Packet RQ if all of the
-	 * following conditions are met:
-	 *  - MPRQ is enabled.
-	 *  - The number of descs is more than the number of strides.
-	 *  - max_rx_pktlen plus overhead is less than the max size
-	 *    of a stride or log_mprq_stride_size is specified by a user.
-	 *    Need to make sure that there are enough strides to encap
-	 *    the maximum packet size in case log_mprq_stride_size is set.
-	 *  Otherwise, enable Rx scatter if necessary.
-	 */
-	if (mprq_en && desc > RTE_BIT32(log_mprq_stride_nums) &&
-	    (non_scatter_min_mbuf_size <=
-	     RTE_BIT32(config->mprq.log_max_stride_size) ||
-	     (config->mprq.log_stride_size &&
-	      non_scatter_min_mbuf_size <= mprq_stride_cap))) {
+	if (mprq_en) {
 		/* TODO: Rx scatter isn't supported yet. */
 		tmpl->rxq.sges_n = 0;
-		/* Trim the number of descs needed. */
-		desc >>= log_mprq_stride_nums;
-		tmpl->rxq.log_strd_num = config->mprq.log_stride_num ?
-			config->mprq.log_stride_num : log_mprq_stride_nums;
-		tmpl->rxq.log_strd_sz = config->mprq.log_stride_size ?
-			config->mprq.log_stride_size : log_mprq_stride_size;
+		tmpl->rxq.log_strd_num = mprq_log_actual_stride_num;
+		tmpl->rxq.log_strd_sz = mprq_log_actual_stride_size;
 		tmpl->rxq.strd_shift_en = MLX5_MPRQ_TWO_BYTE_SHIFT;
 		tmpl->rxq.strd_scatter_en =
 				!!(offloads & RTE_ETH_RX_OFFLOAD_SCATTER);
@@ -1721,11 +1817,6 @@  mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 		max_lro_size = RTE_MIN(max_rx_pktlen,
 				       RTE_BIT32(tmpl->rxq.log_strd_num) *
 				       RTE_BIT32(tmpl->rxq.log_strd_sz));
-		DRV_LOG(DEBUG,
-			"port %u Rx queue %u: Multi-Packet RQ is enabled"
-			" strd_num_n = %u, strd_sz_n = %u",
-			dev->data->port_id, idx,
-			tmpl->rxq.log_strd_num, tmpl->rxq.log_strd_sz);
 	} else if (tmpl->rxq.rxseg_n == 1) {
 		MLX5_ASSERT(max_rx_pktlen <= first_mb_free_size);
 		tmpl->rxq.sges_n = 0;
@@ -1759,24 +1850,6 @@  mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 		tmpl->rxq.sges_n = sges_n;
 		max_lro_size = max_rx_pktlen;
 	}
-	if (config->mprq.enabled && !mlx5_rxq_mprq_enabled(&tmpl->rxq))
-		DRV_LOG(WARNING,
-			"port %u MPRQ is requested but cannot be enabled\n"
-			" (requested: pkt_sz = %u, desc_num = %u,"
-			" rxq_num = %u, stride_sz = %u, stride_num = %u\n"
-			"  supported: min_rxqs_num = %u,"
-			" min_stride_sz = %u, max_stride_sz = %u).",
-			dev->data->port_id, non_scatter_min_mbuf_size,
-			desc, priv->rxqs_n,
-			config->mprq.log_stride_size ?
-				RTE_BIT32(config->mprq.log_stride_size) :
-				RTE_BIT32(log_mprq_stride_size),
-			config->mprq.log_stride_num ?
-				RTE_BIT32(config->mprq.log_stride_num) :
-				RTE_BIT32(log_mprq_stride_nums),
-			config->mprq.min_rxqs_num,
-			RTE_BIT32(config->mprq.log_min_stride_size),
-			RTE_BIT32(config->mprq.log_max_stride_size));
 	DRV_LOG(DEBUG, "port %u maximum number of segments per packet: %u",
 		dev->data->port_id, 1 << tmpl->rxq.sges_n);
 	if (desc % (1 << tmpl->rxq.sges_n)) {
@@ -1834,17 +1907,15 @@  mlx5_rxq_new(struct rte_eth_dev *dev, struct mlx5_rxq_priv *rxq,
 		dev->data->port_id,
 		tmpl->rxq.crc_present ? "disabled" : "enabled",
 		tmpl->rxq.crc_present << 2);
-	/* Save port ID. */
 	tmpl->rxq.rss_hash = !!priv->rss_conf.rss_hf &&
 		(!!(dev->data->dev_conf.rxmode.mq_mode & RTE_ETH_MQ_RX_RSS));
+	/* Save port ID. */
 	tmpl->rxq.port_id = dev->data->port_id;
 	tmpl->sh = priv->sh;
 	tmpl->rxq.mp = rx_seg[0].mp;
 	tmpl->rxq.elts_n = log2above(desc);
-	tmpl->rxq.rq_repl_thresh =
-		MLX5_VPMD_RXQ_RPLNSH_THRESH(desc_n);
-	tmpl->rxq.elts =
-		(struct rte_mbuf *(*)[desc_n])(tmpl + 1);
+	tmpl->rxq.rq_repl_thresh = MLX5_VPMD_RXQ_RPLNSH_THRESH(desc_n);
+	tmpl->rxq.elts = (struct rte_mbuf *(*)[desc_n])(tmpl + 1);
 	tmpl->rxq.mprq_bufs =
 		(struct mlx5_mprq_buf *(*)[desc])(*tmpl->rxq.elts + desc_n);
 	tmpl->rxq.idx = idx;