From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id B57822BF2 for ; Tue, 17 Jan 2017 14:40:17 +0100 (CET) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga104.jf.intel.com with ESMTP; 17 Jan 2017 05:40:16 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,244,1477983600"; d="scan'208";a="49685518" Received: from rhorton-mobl.ger.corp.intel.com (HELO [163.33.230.196]) ([163.33.230.196]) by orsmga004.jf.intel.com with ESMTP; 17 Jan 2017 05:40:15 -0800 To: "Van Haaren, Harry" , "dev@dpdk.org" References: <1484583573-30163-1-git-send-email-remy.horton@intel.com> <1484583573-30163-2-git-send-email-remy.horton@intel.com> Cc: Thomas Monjalon From: Remy Horton Organization: Intel Shannon Limited Message-ID: <0d2e03a5-3eb0-ec3f-9fe7-82e905d3adb3@intel.com> Date: Tue, 17 Jan 2017 13:40:14 +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 1/6] lib: add information metrics 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 13:40:18 -0000 On 17/01/2017 11:01, Van Haaren, Harry wrote: >> -----Original Message----- >> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton [..] > Nit: why the _s in rte_metrics_meta_s? Is there a reason why struct > rte_metrics_meta { is not suitable? Same question for > rte_metrics_data_s. Think it was originally to avoid a namespace collision, and I left the suffix in because this is an internal data-structure. >> + if (memzone == NULL) >> + rte_exit(EXIT_FAILURE, "Unable to allocate stats memzone\n"); >> + stats = memzone->addr; >> + memset(stats, 0, sizeof(struct rte_metrics_data_s)); >> + rte_spinlock_init(&stats->lock); > > Should this spinlock be initialized before the creation of the > memzone? Creating the memzone first, and later initializing the > locking-structure for it feels racey to me. The lock should probably > be taken and unlocked again after init and memset. Problem is the lock being part of the reserved memzone allocation. It is in there so that secondary processes can use the lock. > Nit: would rte_metrics_reg_name() be a better function name? [..] > Nit: would rte_metrics_reg_names() be a better function name? Agree. Done. > Would rte_metrics_update_single() be a better function name? [..] > Would rte_metrics_update() be a better function name? Think rte_metrics_update_value & rte_metrics_update_values would make a better pair. My preference at the moment is not to nominate a preferred one. >> + if (port_id != RTE_METRICS_GLOBAL && >> + (port_id < 0 || port_id > RTE_MAX_ETHPORTS)) >> + return -EINVAL; >> + >> + rte_metrics_init(); > > See above comments on rte_metrics_init() taking a socket_id parameter. Here any core could call update_metrics(), and if the library was not yet initialized, the memory for metrics would end up on the socket of this core. This should be avoided by > A) adding socket_id parameter to the metrics_init() function > B) Demanding that metrics_init() is called by the application before any core uses update_metrics() Done. Should also help alleviate the race. > What is a "set border"? I think this ensures that one set of metrics > cannot overwrite the index's of another metrics set - correct? It is intended to stop things like: idx1 = rte_metrics_reg_names(some_names, 2); idx2 = rte_metrics_reg_names(more_names, 3); ... rte_metrics_update_values(port_id, idx1, &new_values, 5); as the above assumes idx2=idx1+2 which is not guaranteed in concurrent enviornments ..Remy