From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 415941094 for ; Tue, 17 Jan 2017 16:37:14 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga102.fm.intel.com with ESMTP; 17 Jan 2017 07:37:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,245,1477983600"; d="scan'208";a="1113957077" Received: from rhorton-mobl.ger.corp.intel.com (HELO [163.33.230.196]) ([163.33.230.196]) by fmsmga002.fm.intel.com with ESMTP; 17 Jan 2017 07:37:13 -0800 To: "Van Haaren, Harry" , "dev@dpdk.org" References: <1484583573-30163-1-git-send-email-remy.horton@intel.com> <1484583573-30163-4-git-send-email-remy.horton@intel.com> Cc: Thomas Monjalon From: Remy Horton Organization: Intel Shannon Limited Message-ID: Date: Tue, 17 Jan 2017 15:37:12 +0000 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v7 3/6] lib: add bitrate statistics library X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jan 2017 15:37:15 -0000 On 17/01/2017 11:16, Van Haaren, Harry wrote: [..] >> +struct rte_stats_bitrates_s * >> +rte_stats_bitrate_create(void) >> +{ >> + return rte_zmalloc(NULL, sizeof(struct rte_stats_bitrates_s), >> + RTE_CACHE_LINE_SIZE); >> +} > > Is the socket relevant here? Perhaps pass socket_id to the function, > and use rte_zmalloc_socket(). This function has no way of > initializing bitrate structs on two different sockets, using one a > single setup thread. Not an issue in this case. It is expected that the thread that calls this creator will also be the thread that clocks the library (i.e. calling rte_stats_bitrate_calc()), and that one instance handles all ports. The memory block is not accessed outside of rte_stats_bitrate_calc(). >> + /* The +-50 fixes integer rounding during divison */ >> + if (delta > 0) >> + delta = (delta * alpha_percent + 50) / 100; >> + else >> + delta = (delta * alpha_percent - 50) / 100; >> + port_data->ewma_ibits += delta; > > The integer +50 feels a bit odd, I'm not opposed to this if it works though. It was based on feedback regarding roundoffs when doing integer divides. Something about rounding to nearest integer rather than towards zero if I remember correctly.. >> +/** >> + * Bitrate statistics data structure. >> + * This data structure is intentionally opaque. >> + */ >> +struct rte_stats_bitrates_s; > > _s question as previously highlighted. Since this is a public structure, removed the _s prefix in this case. Looking over the coding guidelines I can't see anything explicitly against the notation, but existing structures have not used it. ..Remy