[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