Re: [PATCH] net/ixgbe: consider DCB/VMDq conf when getting RSS conf

[zhoumin <zhoumin@loongson.cn>]

Hi Qi,

Thanks your kind review.

On Thu, Apr 27, 2023 at 7:01PM, Zhang, Qi Z wrote:

>> -----Original Message-----
>> From: Min Zhou <zhoumin@loongson.cn>
>> Sent: Wednesday, April 12, 2023 6:02 PM
>> To: Yang, Qiming <qiming.yang@intel.com>; Wu, Wenjun1
>> <wenjun1.wu@intel.com>; zhoumin@loongson.cn
>> Cc: dev@dpdk.org; maobibo@loongson.cn
>> Subject: [PATCH] net/ixgbe: consider DCB/VMDq conf when getting RSS conf
>>
>> The mrqe field of MRQC register is an enum. From the Intel 82599 datasheet,
>> we know that these values below for the mrqe field are all related to RSS
>> configuration:
>> 	0000b = RSS disabled.
>> 	0001b = RSS only -- Single set of RSS 16 queues.
>> 	0010b = DCB enabled and RSS disabled -- 8 TCs, each allocated 1
>> queue.
>> 	0011b = DCB enabled and RSS disabled -- 4 TCs, each allocated 1
>> queue.
>> 	0100b = DCB and RSS -- 8 TCs, each allocated 16 RSS queues.
>> 	0101b = DCB and RSS -- 4 TCs, each allocated 16 RSS queues.
>> 	1000b = Virtualization only -- 64 pools, no RSS, each pool allocated
>> 		2 queues.
>> 	1010b = Virtualization and RSS -- 32 pools, each allocated 4 RSS
>> queues.
>> 	1011b = Virtualization and RSS -- 64 pools, each allocated 2 RSS
>> queues.
>>
>> The ixgbe pmd will check whether the rss is enabled or not when getting rss
>> conf. So, beside comparing the value of mrqe field with xxx0b and xxx1b, we
>> also needto consider the other configurations, such as DCB + RSS or VMDQ +
>> RSS. Otherwise, we may not get the correct rss conf in some cases, such as
>> when we use DCB and RSS with 8 TCs which corresponds to 0100b for the
>> mrqe field.
>>
>> Signed-off-by: Min Zhou <zhoumin@loongson.cn>
>> ---
>>   drivers/net/ixgbe/ixgbe_rxtx.c | 91 ++++++++++++++++++++++++++++++----
>>   1 file changed, 80 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
>> index c9d6ca9efe..1eff0053ed 100644
>> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
>> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
>> @@ -3461,18 +3461,89 @@ static uint8_t rss_intel_key[40] = {
>>   	0x6A, 0x42, 0xB7, 0x3B, 0xBE, 0xAC, 0x01, 0xFA,  };
>>
>> +/*
>> + * This function removes the rss configuration in the mrqe field of
>> +MRQC
>> + * register and tries to maintain other configurations in the field,
>> +such
>> + * DCB and Virtualization.
>> + *
>> + * The MRQC register supplied in section 7.1.2.8.3 of the Intel 82599
>> datasheet.
>> + * From the datasheet, we know that the mrqe field is an enum. So,
>> +masking the
>> + * mrqe field with '~IXGBE_MRQC_RSSEN' may not completely disable rss
>> + * configuration. For example, the value of mrqe is equal to 0101b when
>> +DCB and
>> + * RSS with 4 TCs configured, however 'mrqe &= ~0x01' is equal to 0100b
>> +which
>> + * corresponds to DCB and RSS with 8 TCs.
>> + */
>> +static void
>> +ixgbe_mrqc_rss_remove(struct ixgbe_hw *hw) {
>> +	uint32_t mrqc;
>> +	uint32_t mrqc_reg;
>> +	uint32_t mrqe_val;
>> +
>> +	mrqc_reg = ixgbe_mrqc_reg_get(hw->mac.type);
>> +	mrqc = IXGBE_READ_REG(hw, mrqc_reg);
>> +	mrqe_val = mrqc & IXGBE_MRQC_MRQE_MASK;
>> +
>> +	switch (mrqe_val) {
>> +	case IXGBE_MRQC_RSSEN:
>> +		/* Completely disable rss */
>> +		mrqe_val = 0;
>> +		break;
>> +	case IXGBE_MRQC_RTRSS8TCEN:
>> +		mrqe_val = IXGBE_MRQC_RT8TCEN;
>> +		break;
>> +	case IXGBE_MRQC_RTRSS4TCEN:
>> +		mrqe_val = IXGBE_MRQC_RT4TCEN;
>> +		break;
>> +	case IXGBE_MRQC_VMDQRSS64EN:
>> +	/* FIXME. Can 32 pools with rss convert to 64 pools without rss? */
>> +	case IXGBE_MRQC_VMDQRSS32EN:
> better not change the pool number, can we just print a warning and break?


Yes. The implicit changing of the pool number is not a good way. I think 
just printing a warning and keeping the 'mrqe' field unchanged  may be 
better.

I will do that in the v2 patch.


> Otherwise
> Acked-by: Qi Zhang <qi.z.zhang@intel.com>
>
Best regards,

Min