From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 63CCC5952 for ; Thu, 12 Mar 2015 20:28:15 +0100 (CET) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP; 12 Mar 2015 12:28:14 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,390,1422950400"; d="scan'208";a="679305746" Received: from irsmsx102.ger.corp.intel.com ([163.33.3.155]) by fmsmga001.fm.intel.com with ESMTP; 12 Mar 2015 12:28:12 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.218]) by IRSMSX102.ger.corp.intel.com ([169.254.2.69]) with mapi id 14.03.0195.001; Thu, 12 Mar 2015 19:28:12 +0000 From: "Dumitrescu, Cristian" To: Stephen Hemminger Thread-Topic: [PATCH v2 5/6] rte_sched: allow reading statistics without clearing Thread-Index: AQHQW01BWBc5LZFsfEaJqAZ+MDSn9J0ZObsg Date: Thu, 12 Mar 2015 19:28:11 +0000 Message-ID: <3EB4FA525960D640B5BDFFD6A3D89126323225F4@IRSMSX108.ger.corp.intel.com> References: <1426004018-25948-1-git-send-email-stephen@networkplumber.org> <1426004018-25948-6-git-send-email-stephen@networkplumber.org> In-Reply-To: <1426004018-25948-6-git-send-email-stephen@networkplumber.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] 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: Thu, 12 Mar 2015 19:28:16 -0000 > -----Original Message----- > From: Stephen Hemminger [mailto:stephen@networkplumber.org] > Sent: Tuesday, March 10, 2015 4:14 PM > To: Dumitrescu, Cristian > Cc: dev@dpdk.org; Stephen Hemminger; Stephen Hemminger > Subject: [PATCH v2 5/6] rte_sched: allow reading statistics without clear= ing >=20 > From: Stephen Hemminger >=20 > The rte_sched statistics API should allow reading statistics without > clearing. Make auto-clear optional. >=20 > Signed-off-by: Stephen Hemminger > --- > v2 -- change to flag for clear from separate read/clear >=20 > app/test/test_sched.c | 4 ++-- > examples/qos_sched/stats.c | 16 +++++++++++----- > lib/librte_sched/rte_sched.c | 44 ++++++++++++++++++++++----------------= - > ----- > lib/librte_sched/rte_sched.h | 18 ++++++++++-------- > 4 files changed, 45 insertions(+), 37 deletions(-) >=20 > diff --git a/app/test/test_sched.c b/app/test/test_sched.c > index 60c62de..3c9c9e3 100644 > --- a/app/test/test_sched.c > +++ b/app/test/test_sched.c > @@ -208,13 +208,13 @@ test_sched(void) >=20 > struct rte_sched_subport_stats subport_stats; > uint32_t tc_ov; > - rte_sched_subport_read_stats(port, SUBPORT, &subport_stats, > &tc_ov); > + rte_sched_subport_read_stats(port, SUBPORT, &subport_stats, > &tc_ov, 1); > #if 0 > TEST_ASSERT_EQUAL(subport_stats.n_pkts_tc[TC-1], 10, "Wrong > subport stats\n"); > #endif > struct rte_sched_queue_stats queue_stats; > uint16_t qlen; > - rte_sched_queue_read_stats(port, QUEUE, &queue_stats, &qlen); > + rte_sched_queue_read_stats(port, QUEUE, &queue_stats, &qlen, > 1); > #if 0 > TEST_ASSERT_EQUAL(queue_stats.n_pkts, 10, "Wrong queue > stats\n"); > #endif > diff --git a/examples/qos_sched/stats.c b/examples/qos_sched/stats.c > index b4db7b5..88caa54 100644 > --- a/examples/qos_sched/stats.c > +++ b/examples/qos_sched/stats.c > @@ -61,7 +61,7 @@ qavg_q(uint8_t port_id, uint32_t subport_id, uint32_t > pipe_id, uint8_t tc, uint8 > average =3D 0; >=20 > for (count =3D 0; count < qavg_ntimes; count++) { > - rte_sched_queue_read_stats(port, queue_id, &stats, &qlen= ); > + rte_sched_queue_read_stats(port, queue_id, &stats, &qlen= , 1); > average +=3D qlen; > usleep(qavg_period); > } > @@ -99,7 +99,9 @@ qavg_tcpipe(uint8_t port_id, uint32_t subport_id, > uint32_t pipe_id, uint8_t tc) > for (count =3D 0; count < qavg_ntimes; count++) { > part_average =3D 0; > for (i =3D 0; i < RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS; i+= +) { > - rte_sched_queue_read_stats(port, queue_id + (tc = * > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS + i), &stats, &qlen); > + rte_sched_queue_read_stats(port, > + queue_id + (tc * > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS + i), > + &stats, &qlen, 1); > part_average +=3D qlen; > } > average +=3D part_average / > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS; > @@ -138,7 +140,8 @@ qavg_pipe(uint8_t port_id, uint32_t subport_id, > uint32_t pipe_id) > for (count =3D 0; count < qavg_ntimes; count++) { > part_average =3D 0; > for (i =3D 0; i < RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE * > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS; i++) { > - rte_sched_queue_read_stats(port, queue_id + i, &= stats, > &qlen); > + rte_sched_queue_read_stats(port, queue_id + i, > + &stats, &qlen, 1); > part_average +=3D qlen; > } > average +=3D part_average / > (RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE * > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS); > @@ -178,7 +181,9 @@ qavg_tcsubport(uint8_t port_id, uint32_t subport_id, > uint8_t tc) > queue_id =3D RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE = * > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS * (subport_id * > port_params.n_pipes_per_subport + i); >=20 > for (j =3D 0; j < RTE_SCHED_QUEUES_PER_TRAFFIC_C= LASS; j++) { > - rte_sched_queue_read_stats(port, queue_i= d + (tc * > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS + j), &stats, &qlen); > + rte_sched_queue_read_stats(port, > + queue_id + (tc * > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS + j), > + &stats, &qlen, 1); > part_average +=3D qlen; > } > } > @@ -220,7 +225,8 @@ qavg_subport(uint8_t port_id, uint32_t subport_id) > queue_id =3D RTE_SCHED_TRAFFIC_CLASSES_PER_PIPE = * > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS * (subport_id * > port_params.n_pipes_per_subport + i); >=20 > for (j =3D 0; j < RTE_SCHED_TRAFFIC_CLASSES_PER_= PIPE * > RTE_SCHED_QUEUES_PER_TRAFFIC_CLASS; j++) { > - rte_sched_queue_read_stats(port, queue_i= d + j, &stats, > &qlen); > + rte_sched_queue_read_stats(port, queue_i= d + j, > + &stats, &qlen, 1); > part_average +=3D qlen; > } > } > diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c > index 2071414..74d0e0a 100644 > --- a/lib/librte_sched/rte_sched.c > +++ b/lib/librte_sched/rte_sched.c > @@ -909,56 +909,56 @@ rte_sched_pipe_config(struct rte_sched_port > *port, >=20 > int > rte_sched_subport_read_stats(struct rte_sched_port *port, > - uint32_t subport_id, > - struct rte_sched_subport_stats *stats, > - uint32_t *tc_ov) > + uint32_t subport_id, > + struct rte_sched_subport_stats *stats, > + uint32_t *tc_ov, int clear) > { > struct rte_sched_subport *s; >=20 > /* Check user parameters */ > - if ((port =3D=3D NULL) || > - (subport_id >=3D port->n_subports_per_port) || > - (stats =3D=3D NULL) || > - (tc_ov =3D=3D NULL)) { > + if (port =3D=3D NULL || subport_id >=3D port->n_subports_per_port) > return -1; > - } > + > s =3D port->subport + subport_id; >=20 > /* Copy subport stats and clear */ > - memcpy(stats, &s->stats, sizeof(struct rte_sched_subport_stats)); > - memset(&s->stats, 0, sizeof(struct rte_sched_subport_stats)); > + if (stats) > + *stats =3D s->stats; > + if (clear) > + memset(&s->stats, 0, sizeof(struct > rte_sched_subport_stats)); >=20 > /* Subport TC ovesubscription status */ > - *tc_ov =3D s->tc_ov; > + if (tc_ov) > + *tc_ov =3D s->tc_ov; >=20 > return 0; > } >=20 > int > rte_sched_queue_read_stats(struct rte_sched_port *port, > - uint32_t queue_id, > - struct rte_sched_queue_stats *stats, > - uint16_t *qlen) > + uint32_t queue_id, > + struct rte_sched_queue_stats *stats, > + uint16_t *qlen, int clear) > { > struct rte_sched_queue *q; > struct rte_sched_queue_extra *qe; >=20 > /* Check user parameters */ > - if ((port =3D=3D NULL) || > - (queue_id >=3D rte_sched_port_queues_per_port(port)) || > - (stats =3D=3D NULL) || > - (qlen =3D=3D NULL)) { > + if (port =3D=3D NULL || queue_id >=3D > rte_sched_port_queues_per_port(port)) > return -1; > - } > + > q =3D port->queue + queue_id; > qe =3D port->queue_extra + queue_id; >=20 > /* Copy queue stats and clear */ > - memcpy(stats, &qe->stats, sizeof(struct rte_sched_queue_stats)); > - memset(&qe->stats, 0, sizeof(struct rte_sched_queue_stats)); > + if (stats) > + *stats =3D qe->stats; > + if (clear) > + memset(&qe->stats, 0, sizeof(struct > rte_sched_queue_stats)); >=20 > /* Queue length */ > - *qlen =3D q->qw - q->qr; > + if (qlen) > + *qlen =3D q->qw - q->qr; >=20 > return 0; > } > diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h > index e9bf18a..f0bf088 100644 > --- a/lib/librte_sched/rte_sched.h > +++ b/lib/librte_sched/rte_sched.h > @@ -308,14 +308,15 @@ rte_sched_port_get_memory_footprint(struct > rte_sched_port_params *params); > * @param tc_ov > * Pointer to pre-allocated 4-entry array where the oversubscription s= tatus > for > * each of the 4 subport traffic classes should be stored. > + * @parm clear > + * Reset statistics after read > * @return > * 0 upon success, error code otherwise > */ > int > -rte_sched_subport_read_stats(struct rte_sched_port *port, > - uint32_t subport_id, > - struct rte_sched_subport_stats *stats, > - uint32_t *tc_ov); > +rte_sched_subport_read_stats(struct rte_sched_port *port, uint32_t > subport_id, > + struct rte_sched_subport_stats *stats, > + uint32_t *tc_ov, int clear); >=20 > /** > * Hierarchical scheduler queue statistics read > @@ -329,14 +330,15 @@ rte_sched_subport_read_stats(struct > rte_sched_port *port, > * counters should be stored > * @param qlen > * Pointer to pre-allocated variable where the current queue length sh= ould > be stored. > + * @parm clear > + * Reset statistics after read > * @return > * 0 upon success, error code otherwise > */ > int > -rte_sched_queue_read_stats(struct rte_sched_port *port, > - uint32_t queue_id, > - struct rte_sched_queue_stats *stats, > - uint16_t *qlen); > +rte_sched_queue_read_stats(struct rte_sched_port *port, uint32_t > queue_id, > + struct rte_sched_queue_stats *stats, > + uint16_t *qlen, int clear); >=20 > /* > * Run-time > -- > 2.1.4 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 par= ameter rather than keep prototype for the stats functions unchanged and add= the flag as part of the port creation parameters in struct rte_sched_port_= params? This parameter could be saved into the internal struct rte_sched_po= rt, which is passed (as a pointer) to the stats read functions. In my opini= on, this approach is slightly more elegant and it keeps the changes to a mi= nimum. 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 opi= nion from you on it yet. Regards, Cristian