From mboxrd@z Thu Jan 1 00:00:00 1970 From: Remy Horton Subject: Re: [PATCH v2 1/3] rte: add keep alive functionality Date: Mon, 26 Oct 2015 16:36:45 +0000 Message-ID: <562E569D.2020808@intel.com> References: <1443603881-4700-1-git-send-email-remy.horton@intel.com> <1443603881-4700-2-git-send-email-remy.horton@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable To: "Wiles, Keith" , "dev@dpdk.org" 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) In-Reply-To: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" '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 =E2=80=98;=E2=80=99 did you have a reason to have it here? If not t= hen 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=20 have to be individually cache-aligned. The gaps it leaves behind are=20 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=20 actually gain anything. >> + keepcfg =3D malloc(sizeof(struct rte_keepalive)); >> + if (keepcfg !=3D NULL) { >> + for (idx_core =3D 0; idx_core < RTE_KEEPALIVE_MAXCORES; idx_core++)= { >> + keepcfg->state_flags[idx_core] =3D 0; >> + keepcfg->active_cores[idx_core] =3D 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=20 structure rather than allocate one. ..Remy