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

[Kevin Traynor <ktraynor@redhat.com>]

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>
>>>>> ---
>>>
>