From: "Martin Pieuchot" <mpi@grenadille.net>
Subject: Re: dt: Count skipped clock cycles as dropped  events
To: "Christian Ludwig" <cludwig@genua.de>
Cc: tech@openbsd.org
Date: Thu, 07 Nov 2024 10:36:17 +0100

On 05/11/24(Tue) 14:07, Christian Ludwig wrote:
> If the clock interrupt advances for more than one tick, we obviously
> lost a number of clock ticks. That might happen in a virtual machine
> that does not get scheduled in time. There is no point in creating a
> cluster of events with basically the same timestamp and identical stack
> traces. Generate only one event instead and drop all skipped ticks.

My understanding of the current behavior is that if a periodic timer is
scheduled with a small resolution, depending on the load, it might be
blocked by other workload (locks, spl...).

I agree that for profiling purposes it is not useful to generate the
same event multiple times.  However I'd find useful to have a custom
error counter such that we can figure out that the clock resolution
is too small with the current workload.

I'd also suggest to add another error counter for recursion we prevented
in the precedent diff.  Would you agree?

> ---
>  sys/dev/dt/dt_dev.c          | 12 ++++++++++--
>  sys/dev/dt/dt_prov_profile.c | 17 ++++++++++-------
>  sys/dev/dt/dtvar.h           |  1 +
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/sys/dev/dt/dt_dev.c b/sys/dev/dt/dt_dev.c
> index 75fbbba6a575..286bda80992d 100644
> --- a/sys/dev/dt/dt_dev.c
> +++ b/sys/dev/dt/dt_dev.c
> @@ -725,6 +725,15 @@ dt_pcb_purge(struct dt_pcb_list *plist)
>  	}
>  }
>  
> +void
> +dt_pcb_ring_drop(struct dt_pcb *dp, unsigned int drop)
> +{
> +	struct dt_cpubuf *dc = &dp->dp_sc->ds_cpu[cpu_number()];
> +
> +	dc->dc_dropevt += drop;
> +	membar_producer();
> +}
> +
>  /*
>   * Get a reference to the next free event state from the ring.
>   */
> @@ -747,8 +756,7 @@ dt_pcb_ring_get(struct dt_pcb *dp, int profiling)
>  	distance = prod - cons;
>  	if (distance == 1 || distance == (1 - DT_EVTRING_SIZE)) {
>  		/* read(2) isn't finished */
> -		dc->dc_dropevt++;
> -		membar_producer();
> +		dt_pcb_ring_drop(dp, 1);
>  
>  		dc->dc_inevt = 0;
>  		return NULL;
> diff --git a/sys/dev/dt/dt_prov_profile.c b/sys/dev/dt/dt_prov_profile.c
> index 62900152e147..5e4d9f1a2081 100644
> --- a/sys/dev/dt/dt_prov_profile.c
> +++ b/sys/dev/dt/dt_prov_profile.c
> @@ -101,15 +101,18 @@ dt_prov_profile_alloc(struct dt_probe *dtp, struct dt_softc *sc,
>  void
>  dt_clock(struct clockrequest *cr, void *cf, void *arg)
>  {
> -	uint64_t count, i;
> +	uint64_t count;
>  	struct dt_evt *dtev;
>  	struct dt_pcb *dp = arg;
>  
>  	count = clockrequest_advance(cr, dp->dp_nsecs);
> -	for (i = 0; i < count; i++) {
> -		dtev = dt_pcb_ring_get(dp, 1);
> -		if (dtev == NULL)
> -			return;
> -		dt_pcb_ring_consume(dp, dtev);
> -	}
> +	if (count == 0)
> +		return;
> +	else if (count > 1)
> +		dt_pcb_ring_drop(dp, count - 1);
> +
> +	dtev = dt_pcb_ring_get(dp, 1);
> +	if (dtev == NULL)
> +		return;
> +	dt_pcb_ring_consume(dp, dtev);
>  }
> diff --git a/sys/dev/dt/dtvar.h b/sys/dev/dt/dtvar.h
> index c6cfa7f66b3b..a1f6037e3233 100644
> --- a/sys/dev/dt/dtvar.h
> +++ b/sys/dev/dt/dtvar.h
> @@ -179,6 +179,7 @@ struct dt_pcb	*dt_pcb_alloc(struct dt_probe *, struct dt_softc *);
>  void		 dt_pcb_free(struct dt_pcb *);
>  void		 dt_pcb_purge(struct dt_pcb_list *);
>  
> +void		 dt_pcb_ring_drop(struct dt_pcb *, unsigned int);
>  struct dt_evt	*dt_pcb_ring_get(struct dt_pcb *, int);
>  void		 dt_pcb_ring_consume(struct dt_pcb *, struct dt_evt *);
>  
> -- 
> 2.34.1
>