From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 4081D5922 for ; Fri, 30 Sep 2016 11:09:27 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP; 30 Sep 2016 02:09:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,271,1473145200"; d="scan'208";a="767491903" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.221.70]) ([10.237.221.70]) by FMSMGA003.fm.intel.com with ESMTP; 30 Sep 2016 02:09:25 -0700 To: "Wu, Jingjing" , "Guo, Jia" , "Zhang, Helin" References: <1474887098-115474-1-git-send-email-jia.guo@intel.com> <9BB6961774997848B5B42BEC655768F80E274D11@SHSMSX103.ccr.corp.intel.com> Cc: "dev@dpdk.org" From: Ferruh Yigit Message-ID: Date: Fri, 30 Sep 2016 10:09:24 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <9BB6961774997848B5B42BEC655768F80E274D11@SHSMSX103.ccr.corp.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] drivers/i40e: fix the hash filter invalid calculation in X722 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 30 Sep 2016 09:09:27 -0000 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 <...> >>> +#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.