[dpdk-dev,v2,4/7] rte_sched: don't clear statistics when read

  Hi Stephen,

What is the reason not to clear statistics on read? Do you have a use-case / justification for it?

(BTW, I see you added the reset functions, but was it also your intention to remove the memset to 0 from the stats read functions? :) )

Regards,
Cristian

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
Sent: Thursday, February 5, 2015 6:14 AM
To: dev@dpdk.org
Cc: Stephen Hemminger
Subject: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read

From: Stephen Hemminger <shemming@brocade.com>

Make rte_sched statistics API work like the ethernet statistics API.
Don't auto-clear statistics.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_sched/rte_sched.c | 30 ++++++++++++++++++++++++++++++
 lib/librte_sched/rte_sched.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
  

On Mon, 9 Feb 2015 22:48:36 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> Hi Stephen,
> 
> What is the reason not to clear statistics on read? Do you have a use-case / justification for it?
> 
> (BTW, I see you added the reset functions, but was it also your intention to remove the memset to 0 from the stats read functions? :) )
> 
> Regards,
> Cristian

Read and clear is a non-standard model. Interface statistics are not read/clear.
We have lots of scripts that read statistics. Users don't like it if when 
stastics disappear.
  

On Mon, Feb 09, 2015 at 10:48:36PM +0000, Dumitrescu, Cristian wrote:
> Hi Stephen,
> 
> What is the reason not to clear statistics on read? Do you have a use-case / justification for it?
> 
> (BTW, I see you added the reset functions, but was it also your intention to remove the memset to 0 from the stats read functions? :) )
> 
> Regards,
> Cristian
> 
Its the difference between a hardware and a software interface.  Hardware stats
are often read-clear, but software hides that, making stats continuous.
Exposing it is atypical for a software stack.
Neil

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Thursday, February 5, 2015 6:14 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger
> Subject: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when read
> 
> From: Stephen Hemminger <shemming@brocade.com>
> 
> Make rte_sched statistics API work like the ethernet statistics API.
> Don't auto-clear statistics.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_sched/rte_sched.c | 30 ++++++++++++++++++++++++++++++
>  lib/librte_sched/rte_sched.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/lib/librte_sched/rte_sched.c b/lib/librte_sched/rte_sched.c
> index 8cb8bf1..d891e50 100644
> --- a/lib/librte_sched/rte_sched.c
> +++ b/lib/librte_sched/rte_sched.c
> @@ -935,6 +935,21 @@ rte_sched_subport_read_stats(struct rte_sched_port *port,
>  }
>  
>  int
> +rte_sched_subport_stats_reset(struct rte_sched_port *port,
> +			      uint32_t subport_id)
> +{
> +	struct rte_sched_subport *s;
> +
> +	/* Check user parameters */
> +	if (port == NULL || subport_id >= port->n_subports_per_port)
> +		return -1;
> +
> +	s = port->subport + subport_id;
> +	memset(&s->stats, 0, sizeof(struct rte_sched_subport_stats));
> +	return 0;
> +}
> +
> +int
>  rte_sched_queue_read_stats(struct rte_sched_port *port,
>  	uint32_t queue_id,
>  	struct rte_sched_queue_stats *stats,
> @@ -963,6 +978,21 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
>  	return 0;
>  }
>  
> +int
> +rte_sched_queue_stats_reset(struct rte_sched_port *port,
> +			    uint32_t queue_id)
> +{
> +	struct rte_sched_queue_extra *qe;
> +
> +	/* Check user parameters */
> +	if (port == NULL || queue_id >= rte_sched_port_queues_per_port(port))
> +		return -1;
> +
> +	qe = port->queue_extra + queue_id;
> +	memset(&qe->stats, 0, sizeof(struct rte_sched_queue_stats));
> +	return 0;
> +}
> +
>  static inline uint32_t
>  rte_sched_port_qindex(struct rte_sched_port *port, uint32_t subport, uint32_t pipe, uint32_t traffic_class, uint32_t queue)
>  {
> diff --git a/lib/librte_sched/rte_sched.h b/lib/librte_sched/rte_sched.h
> index e9bf18a..3d007e4 100644
> --- a/lib/librte_sched/rte_sched.h
> +++ b/lib/librte_sched/rte_sched.h
> @@ -317,6 +317,21 @@ rte_sched_subport_read_stats(struct rte_sched_port *port,
>  	struct rte_sched_subport_stats *stats,
>  	uint32_t *tc_ov);
>  
> +
> +/**
> + * Hierarchical scheduler subport statistics reset
> + *
> + * @param port
> + *   Handle to port scheduler instance
> + * @param subport_id
> + *   Subport ID
> + * @return
> + *   0 upon success, error code otherwise
> + */
> +int
> +rte_sched_subport_stats_reset(struct rte_sched_port *port,
> +			      uint32_t subport_id);
> +
>  /**
>   * Hierarchical scheduler queue statistics read
>   *
> @@ -338,6 +353,20 @@ rte_sched_queue_read_stats(struct rte_sched_port *port,
>  	struct rte_sched_queue_stats *stats,
>  	uint16_t *qlen);
>  
> +/**
> + * Hierarchical scheduler queue statistics reset
> + *
> + * @param port
> + *   Handle to port scheduler instance
> + * @param queue_id
> + *   Queue ID within port scheduler
> + * @return
> + *   0 upon success, error code otherwise
> + */
> +int
> +rte_sched_queue_stats_reset(struct rte_sched_port *port,
> +			    uint32_t queue_id);
> +
>  /*
>   * Run-time
>   *
> -- 
> 2.1.4
> 
> --------------------------------------------------------------
> Intel Shannon Limited
> Registered in Ireland
> Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
> Registered Number: 308263
> Business address: Dromore House, East Park, Shannon, Co. Clare
> 
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
> 
> 
>
  

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, February 9, 2015 10:55 PM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when
> read
> 
> On Mon, 9 Feb 2015 22:48:36 +0000
> "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> 
> > Hi Stephen,
> >
> > What is the reason not to clear statistics on read? Do you have a use-case /
> justification for it?
> >
> > (BTW, I see you added the reset functions, but was it also your intention to
> remove the memset to 0 from the stats read functions? :) )
> >
> > Regards,
> > Cristian
> 
> Read and clear is a non-standard model. Interface statistics are not
> read/clear.
> We have lots of scripts that read statistics. Users don't like it if when
> stastics disappear.

Stephen, I suggest adding a new build-time configuration option for the librte_sched library in config/common_* files: CONFIG_RTE_SCHED_STATS_CLEAR_ON_READ.
- When set to YES, we clear the stats counters on read. When set to NO, we do not clear stats counters on read;
- The default value for this configuration option should be YES, in order to preserve the backward compatibility with the current behavior;
- The stats reset functions introduced by this patch should always be enabled/compiled into the code, regardless of whether this new option is set to YES or NO.

What do you think?

Regards,
Cristian


--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
  

On Fri, 20 Feb 2015 18:32:03 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> Stephen, I suggest adding a new build-time configuration option for the librte_sched library in config/common_* files: CONFIG_RTE_SCHED_STATS_CLEAR_ON_READ.

Build time config options do not work for distributions.
  

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Friday, February 20, 2015 7:53 PM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when
> read
> 
> On Fri, 20 Feb 2015 18:32:03 +0000
> "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> 
> > Stephen, I suggest adding a new build-time configuration option for the
> librte_sched library in config/common_* files:
> CONFIG_RTE_SCHED_STATS_CLEAR_ON_READ.
> 
> Build time config options do not work for distributions.

Why?

This does not affect the API, as the new API functions are always compiled in, and the prototypes are not changed, and no data structures are affected.
This only changes the behavior of certain functions, so that user can select which mode it needs.
It also preserves backward compatibility.

We have so many compilation options in config file, why is this one different?

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
  

2015-02-20 20:23, Dumitrescu, Cristian:
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > On Fri, 20 Feb 2015 18:32:03 +0000
> > "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> > 
> > > Stephen, I suggest adding a new build-time configuration option for the
> > > librte_sched library in config/common_* files:
> > > CONFIG_RTE_SCHED_STATS_CLEAR_ON_READ.
> > 
> > Build time config options do not work for distributions.
> 
> Why?
> 
> This does not affect the API, as the new API functions are always compiled
> in, and the prototypes are not changed, and no data structures are affected.

Behaviour is an important part of the API. Think comments as part of the API.

> This only changes the behavior of certain functions, so that user can
> select which mode it needs.

When user doesn't or cannot rebuild, he has no choice.

> It also preserves backward compatibility.
> 
> We have so many compilation options in config file, why is this one different?

We must remove and avoid build-time options.
The only ones which might be acceptable are the ones which allow more
performance by disabling some features.

> This e-mail and any attachments may contain confidential material for the
> sole use of the intended recipient(s). Any review or distribution by others
> is strictly prohibited. If you are not the intended recipient, please contact
> the sender and delete all copies.

Please ask to your administrator to remove this disclaimer.
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, February 20, 2015 9:01 PM
> To: Dumitrescu, Cristian
> Cc: dev@dpdk.org; Stephen Hemminger
> Subject: Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when
> read
> 
> 2015-02-20 20:23, Dumitrescu, Cristian:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > On Fri, 20 Feb 2015 18:32:03 +0000
> > > "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> > >
> > > > Stephen, I suggest adding a new build-time configuration option for the
> > > > librte_sched library in config/common_* files:
> > > > CONFIG_RTE_SCHED_STATS_CLEAR_ON_READ.
> > >
> > > Build time config options do not work for distributions.
> >
> > Why?
> >
> > This does not affect the API, as the new API functions are always compiled
> > in, and the prototypes are not changed, and no data structures are
> affected.
> 
> Behaviour is an important part of the API. Think comments as part of the API.

Makes sense.

> 
> > This only changes the behavior of certain functions, so that user can
> > select which mode it needs.
> 
> When user doesn't or cannot rebuild, he has no choice.
> 
> > It also preserves backward compatibility.
> >
> > We have so many compilation options in config file, why is this one
> different?
> 
> We must remove and avoid build-time options.
> The only ones which might be acceptable are the ones which allow more
> performance by disabling some features.

Agree.
Stephen, how about a run-time solution (I agree it would be much better, why did I not consider this in the first place?) of adding a new bool parameter in struct rte_sched_port_params: clear_stats_on_reset?
Both stats read function get the port handle (struct rte_sched_port *) as parameter, so there should be no ripple effect to propagate this flag.

> 
> > This e-mail and any attachments may contain confidential material for the
> > sole use of the intended recipient(s). Any review or distribution by others
> > is strictly prohibited. If you are not the intended recipient, please contact
> > the sender and delete all copies.
> 
> Please ask to your administrator to remove this disclaimer.

I did earlier, this is still work in progress unfortunately.

--------------------------------------------------------------
Intel Shannon Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
Business address: Dromore House, East Park, Shannon, Co. Clare

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
  

On Fri, 20 Feb 2015 21:28:55 +0000
"Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:

> Agree.
> Stephen, how about a run-time solution (I agree it would be much better, why did I not consider this in the first place?) of adding a new bool parameter in struct rte_sched_port_params: clear_stats_on_reset?
> Both stats read function get the port handle (struct rte_sched_port *) as parameter, so there should be no ripple effect to propagate this flag.

Why not read_and_clear function if absolutely necessary.
  

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, February 21, 2015 1:53 AM
> To: Dumitrescu, Cristian
> Cc: Thomas Monjalon; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 4/7] rte_sched: don't clear statistics when
> read
> 
> On Fri, 20 Feb 2015 21:28:55 +0000
> "Dumitrescu, Cristian" <cristian.dumitrescu@intel.com> wrote:
> 
> > Agree.
> > Stephen, how about a run-time solution (I agree it would be much better,
> why did I not consider this in the first place?) of adding a new bool parameter
> in struct rte_sched_port_params: clear_stats_on_reset?
> > Both stats read function get the port handle (struct rte_sched_port *) as
> parameter, so there should be no ripple effect to propagate this flag.
> 
> Why not read_and_clear function if absolutely necessary.

I am not sure I understand what you mean. Are you suggesting different set of stats read functions, one that is read and clear, while the other one is read without clear?

Personally, I think we should avoid proliferating the number of stats functions, I would keep a single set of stats read functions, which can clear the stats or not, depending on behaviour configured per rte_sched object at creation time. Basically, based on the value of configuration parameter struct rte_sched_params::clear_stats_on_reset, the stats read functions do clear the counters or not. In my opinion, this allows a clean init-time selection of the required behaviour, and it also provides backward compatibility. Any issues with this approach?