From: Ferruh Yigit <ferruh.yigit@intel.com>
To: "Wu, Jingjing" <jingjing.wu@intel.com>,
"Guo, Jia" <jia.guo@intel.com>,
"Zhang, Helin" <helin.zhang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH] drivers/i40e: fix the hash filter invalid calculation in X722
Date: Fri, 30 Sep 2016 10:09:24 +0100 [thread overview]
Message-ID: <fed728a1-100f-ccc1-21fa-6cac2b7efaca@intel.com> (raw)
In-Reply-To: <9BB6961774997848B5B42BEC655768F80E274D11@SHSMSX103.ccr.corp.intel.com>
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@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@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.
next prev parent reply other threads:[~2016-09-30 9:09 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-26 10:51 Jeff Guo
2016-09-29 6:29 ` Wu, Jingjing
2016-09-29 18:15 ` Ferruh Yigit
2016-09-30 6:05 ` Wu, Jingjing
2016-09-30 9:09 ` Ferruh Yigit [this message]
2016-10-16 1:40 ` [dpdk-dev] [PATCH v2 1/2] drivers/i40e: fix X722 macro absence result in compile Jeff Guo
2016-10-16 1:40 ` [dpdk-dev] [PATCH v2 2/2] drivers/i40e: fix the hash filter invalid calculation in X722 Jeff Guo
2016-10-18 16:25 ` Ferruh Yigit
2016-10-20 2:48 ` [dpdk-dev] [PATCH] net/i40e: " Jeff Guo
2016-10-24 9:10 ` Wu, Jingjing
2016-10-25 2:11 ` Guo, Jia
2016-10-25 2:26 ` [dpdk-dev] [PATCH v4] " Jeff Guo
2016-10-25 2:42 ` [dpdk-dev] [PATCH v4] net/i40e: fix hash filter invalid issue " Jeff Guo
2016-10-25 10:22 ` Wu, Jingjing
2016-10-25 12:29 ` Bruce Richardson
2016-10-16 13:31 ` [dpdk-dev] [PATCH v2 1/2] drivers/i40e: fix X722 macro absence result in compile Ananyev, Konstantin
2016-10-17 7:44 ` Guo, Jia
2016-10-17 9:54 ` Ananyev, Konstantin
2016-10-17 10:14 ` Chilikin, Andrey
2016-10-18 16:22 ` Ferruh Yigit
2016-10-19 6:10 ` Guo, Jia
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fed728a1-100f-ccc1-21fa-6cac2b7efaca@intel.com \
--to=ferruh.yigit@intel.com \
--cc=dev@dpdk.org \
--cc=helin.zhang@intel.com \
--cc=jia.guo@intel.com \
--cc=jingjing.wu@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).