[dpdk-dev] [PATCH] drivers/i40e: fix the hash filter invalid calculation in X722

Ferruh Yigit ferruh.yigit at intel.com
Fri Sep 30 11:09:24 CEST 2016


Hi Jingjing,

On 9/30/2016 7:05 AM, Wu, Jingjing wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Friday, September 30, 2016 2:16 AM
>> To: Guo, Jia; Zhang, Helin; Wu, Jingjing
>> Cc: dev at dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] drivers/i40e: fix the hash filter invalid
>> calculation in X722
>>
>> On 9/26/2016 11:51 AM, Jeff Guo wrote:
>>> As X722 extracts IPv4 header to Field Vector different with
>>> XL710/X710, need to corresponding to modify the fields of IPv4 header
>>> in input set to map different default Field Vector Table of different nics.
>>>
>>> Signed-off-by: Jeff Guo <jia.guo at intel.com>

<...>

>>> +#ifdef X722_SUPPORT
>>> +/* Source IPv4 address for X722 */
>>> +#define I40E_X722_REG_INSET_L3_SRC_IP4           0x0006000000000000ULL
>>
>> These macros defined here within "#ifdef X722_SUPPORT" and later used
>> unconditionally, this will cause a compile error when "X722_SUPPORT" not
>> defined.
> These macros defined are used in condition later. 

No they are not used in condition, please check that macros have been
used in inset_map1[] without an ifdef.

> Maybe the compile error
> Already exist in current driver. We need to check that and fix this.

And yes compile error already exist in driver! but lets not add a new ones.

>>
>>> +/* Destination IPv4 address for X722 */
>>> +#define I40E_X722_REG_INSET_L3_DST_IP4           0x0000060000000000ULL
>>> +/* IPv4 Type of Service (TOS) */
>>> +#define I40E_X722_REG_INSET_L3_IP4_TOS           0x0040000000000000ULL
>>
>> This value seems same as I40E_REG_INSET_L3_IP4_TOS, why creating a X722
>> version of this?
>>
>>> +/* IPv4 Protocol */
>>> +#define I40E_X722_REG_INSET_L3_IP4_PROTO
>> 0x0010000000000000ULL
>>> +/* IPv4 Time to Live */
>>> +#define I40E_X722_REG_INSET_L3_IP4_TTL           0x0010000000000000ULL
>>> +#endif

Overall, these new values seems like not conflicting with existing
values, can you please confirm that, if this is the case why not define
these values without ifdef?

<...>

>>
>> ---->
>>> +		{I40E_INSET_IPV4_SRC, I40E_X722_REG_INSET_L3_SRC_IP4},
>>> +		{I40E_INSET_IPV4_DST, I40E_X722_REG_INSET_L3_DST_IP4},
>>> +		{I40E_INSET_IPV4_TOS, I40E_X722_REG_INSET_L3_IP4_TOS},
>>> +		{I40E_INSET_IPV4_PROTO,
>> I40E_X722_REG_INSET_L3_IP4_PROTO},
>>> +		{I40E_INSET_IPV4_TTL, I40E_X722_REG_INSET_L3_IP4_TTL},
>> <----
>> Since limited number of values are different from inset_map[], and most of
>> them are duplication, is it possible to prevent duplication?
> Didn't find and proposal on that. Because we need to support
> I40E_X722_REG_INSET_XX and I40E_REG_INSET_XX at the same time. So it
> Cannot be achieved by #ifdef and #endif.

It doesn't have to be with #ifdef #endif, whole array duplicated for 5
changes, there must be a way to remove duplication. First thing I can
think of is extracting common part into a new array?

<...>

>>>  	/* Translate input set to register aware inset */
>>> +#ifdef X722_SUPPORT
>>> +	if (type == I40E_MAC_X722) {
>>> +		for (i = 0; i < RTE_DIM(inset_map1); i++) {
>>> +			if (input & inset_map1[i].inset)
>>> +				val |= inset_map1[i].inset_reg;
>>> +		}
>>> +	} else {
>>> +		for (i = 0; i < RTE_DIM(inset_map); i++) {
>>> +			if (input & inset_map[i].inset)
>>> +				val |= inset_map[i].inset_reg;
>>> +		}
>>> +	}
>>> +#else
>>>  	for (i = 0; i < RTE_DIM(inset_map); i++) {
>>>  		if (input & inset_map[i].inset)
>>>  			val |= inset_map[i].inset_reg;
>>>  	}
>>> +#endif
>>
>> What about something like this, to prevent duplication:
>>
>> inset_map_x = inset_map;
>>
>> #ifdef X722_SUPPORT
>> if (type == I40E_MAC_X722)
>> 	inset_map_x = inset_map1;
>> #endif
>>
>> for (i = 0; i < RTE_DIM(inset_map_x); i++) {
>> 	if (input & inset_map_x[i].inset)
>> 		val |= inset_map_x[i].inset_reg;
>> }
>>
> Also thought about that way, but if X722_SUPPORT is not set
> Compile error will report because of unused parameter.

I believe there won't be a unused parameter warning for above suggestion
but the intention here is to simplify the logic, which I think can be
done, it doesn't have to be like suggested way.




More information about the dev mailing list