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 B69198D3D for ; Mon, 26 Oct 2015 17:36:48 +0100 (CET) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP; 26 Oct 2015 09:36:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,201,1444719600"; d="scan'208";a="835367981" Received: from unknown (HELO [10.252.26.253]) ([10.252.26.253]) by orsmga002.jf.intel.com with ESMTP; 26 Oct 2015 09:36:46 -0700 Message-ID: <562E569D.2020808@intel.com> Date: Mon, 26 Oct 2015 16:36:45 +0000 From: Remy Horton Organization: Intel Shannon Limited User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 MIME-Version: 1.0 To: "Wiles, Keith" , "dev@dpdk.org" References: <1443603881-4700-1-git-send-email-remy.horton@intel.com> <1443603881-4700-2-git-send-email-remy.horton@intel.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 1/3] rte: add keep alive functionality 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: Mon, 26 Oct 2015 16:36:49 -0000 'noon, On 23/10/2015 15:27, Wiles, Keith wrote: >> + uint32_t __rte_cache_aligned state_flags[RTE_KEEPALIVE_MAXCORES]; > Normally I see the __rte_cache_aligned at the end of the line before > the ‘;’ did you have a reason to have it here? If not then I would > move it to the end to look the same as the others. I did a quick grop > in the code and that is the normal location. > > My next question is why not align the whole, which would do the same > thing. I did not check the compiler output, but I was thinking it > would possible leave gaps in the structure for bytes we can not use > normally, but maybe that is not a problem. Each element of state_flags is assigned to a different LCore, so they have to be individually cache-aligned. The gaps it leaves behind are unavoidable. > Next it appears the state_flags is only being set to 0-3, which means > it does not need to be a uint43_t, but could be a uint8_t, correct? Yes, but since it all needs to be cache aligned anyway, wouldn't actually gain anything. >> + keepcfg = malloc(sizeof(struct rte_keepalive)); >> + if (keepcfg != NULL) { >> + for (idx_core = 0; idx_core < RTE_KEEPALIVE_MAXCORES; idx_core++) { >> + keepcfg->state_flags[idx_core] = 0; >> + keepcfg->active_cores[idx_core] = 0; >> + } > > Could you have done a calloc then you do not need the for loop to zero stuff? Could do. It was written this way because the function originally took a structure rather than allocate one. ..Remy