From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 218405690 for ; Mon, 16 Mar 2015 21:21:42 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP; 16 Mar 2015 13:18:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,411,1422950400"; d="scan'208";a="468054734" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by FMSMGA003.fm.intel.com with ESMTP; 16 Mar 2015 13:14:30 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.218]) by IRSMSX151.ger.corp.intel.com ([169.254.4.7]) with mapi id 14.03.0195.001; Mon, 16 Mar 2015 20:21:40 +0000 From: "Dumitrescu, Cristian" To: Stephen Hemminger Thread-Topic: [PATCH v2 5/6] rte_sched: allow reading statistics without clearing Thread-Index: AQHQW01BWBc5LZFsfEaJqAZ+MDSn9J0ZObsggABB/wCABhBNcIAACBAAgAAATWA= Date: Mon, 16 Mar 2015 20:21:39 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D89126323235E5@IRSMSX108.ger.corp.intel.com> References: <1426004018-25948-1-git-send-email-stephen@networkplumber.org> <1426004018-25948-6-git-send-email-stephen@networkplumber.org> <3EB4FA525960D640B5BDFFD6A3D89126323225F4@IRSMSX108.ger.corp.intel.com> <20150312160626.47315f49@urahara> <3EB4FA525960D640B5BDFFD6A3D8912632323539@IRSMSX108.ger.corp.intel.com> <20150316131112.6a945025@urahara> In-Reply-To: <20150316131112.6a945025@urahara> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.181] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" , Stephen Hemminger Subject: Re: [dpdk-dev] [PATCH v2 5/6] rte_sched: allow reading statistics without clearing 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, 16 Mar 2015 20:21:43 -0000 > -----Original Message----- > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Monday, March 16, 2015 8:11 PM > To: Dumitrescu, Cristian > Cc: dev@dpdk.org; Stephen Hemminger > Subject: Re: [PATCH v2 5/6] rte_sched: allow reading statistics without > clearing >=20 > On Mon, 16 Mar 2015 19:58:51 +0000 > "Dumitrescu, Cristian" wrote: >=20 > > > > > > > -----Original Message----- > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > > > Sent: Thursday, March 12, 2015 11:06 PM > > > To: Dumitrescu, Cristian > > > Cc: dev@dpdk.org; Stephen Hemminger > > > Subject: Re: [PATCH v2 5/6] rte_sched: allow reading statistics witho= ut > > > clearing > > > > > > On Thu, 12 Mar 2015 19:28:11 +0000 > > > "Dumitrescu, Cristian" wrote: > > > > > > > Hi Stephen, > > > > > > > > Thank you for adding flexibility over clearing the stats or not. > > > > > > > > I have one concern though: why change the stats read API to add a > clear > > > parameter rather than keep prototype for the stats functions unchange= d > and > > > add the flag as part of the port creation parameters in struct > > > rte_sched_port_params? This parameter could be saved into the interna= l > > > struct rte_sched_port, which is passed (as a pointer) to the stats re= ad > > > functions. In my opinion, this approach is slightly more elegant and = it > keeps > > > the changes to a minimum. > > > > > > > > int > > > > rte_sched_queue_read_stats(struct rte_sched_port *port, > > > > uint32_t queue_id, > > > > struct rte_sched_queue_stats *stats, > > > > uint16_t *qlen) > > > > { > > > > ... > > > > if (port->clear_stats_on_read) > > > > memset(...); > > > > } > > > > > > > > I made this suggestion during the previous round, but I did not get= any > > > opinion from you on it yet. > > > > > > > > Regards, > > > > Cristian > > > > > > I rejected the config parameter idea because I don't like it is incon= sistent > > > with other statistics in DPDK and in related software. There is not a > > > config parameter that changes what BSD or Linux kernel API does. > > > > Your approach has the advantage of being able to clear/not clear the st= ats > per each read operation rather than configuring the behavior globally. I = think > this approach allows for the ultimate flexibility, so I am OK to go with = it. > > > > > > > > The only reason for keeping the read and clear in one operation is > > > because you like it, and there are somebody might have built code > > > that expects it. > > > > > > > Clearing the stats with a delay after the stats have been read is prone= to a > race condition, as during this time more packets could be processed, and > these packets will not show up in the counters that the user read. > > I think it depends on the need of each particular application whether t= his > race condition is important or not: if the counters are read rarely (e.g.= once > per day) and only course accuracy is required, the error is probably irre= levant; > if the app is looking for fine great accuracy (e.g. rate measurement, > debugging, etc), then the error is not allowed. You seem to favour the > former and ignore the later case. > > > > > > > Changing the function signature is a nice red flag so that people wil= l > > > notice at change. > > > > There is a small API change here. I am OK with it for the above reasons= , > provided that there are no objections on this from other contributors. > > >=20 > If you really wanted a fast/safe read and clear operation, how about usin= g > exchange instruction to exchange in a zero? When stats read is decoupled from stats clear, the exchange operation canno= t fix this. To your point: when stats are cleared on read, would it make sense to use t= he exchange operation? In my opinion, it does not make sense, as the rte_sc= hed API is not thread safe (for performance reasons), which is explicitly = stated. As clearly documented, the scheduler enqueue and dequeue operations= need to run on the same CPU core. Therefore, IMHO having a thread safe sta= ts read function while the main functional API is not thread safe is not go= ing to add value.