[PATCH] Change min/max to float numbers

[PATCH] Change min/max to float numbers

[Rafael Folco]

Make min/max consistent with avg by recording the latency samples
as float numbers instead of integers.

Signed-off-by: Rafael Folco <rfolco@redhat.com>
---
 src/oslat/oslat.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
index 9e5a08a..6d4a9cb 100644
--- a/src/oslat/oslat.c
+++ b/src/oslat/oslat.c
@@ -159,9 +159,9 @@ struct thread {
 	stamp_t              frc_stop;
 	cycles_t             runtime;
 	stamp_t              *buckets;
-	uint64_t             minlat;
+	double               minlat;
 	/* Maximum latency detected */
-	uint64_t             maxlat;
+	double               maxlat;
 	/*
 	 * The extra part of the interruptions that cannot be put into even the
 	 * biggest bucket.  We'll use this to calculate a more accurate average at
@@ -330,12 +330,11 @@ static float cycles_to_sec(const struct thread *t, uint64_t cycles)
 static void insert_bucket(struct thread *t, stamp_t value)
 {
 	int index;
-	unsigned int lat;
 	uint64_t extra;
-	double us;
+	double lat, us;
 
-	lat = (value * g.unit_per_us) / t->counter_mhz;
-	us = (double)lat / g.unit_per_us;
+	lat = (double)(value * g.unit_per_us) / t->counter_mhz;
+	us = lat / g.unit_per_us;
 	if (!g.preheat && g.trace_threshold && us >= g.trace_threshold) {
 		char *line = "%s: Trace threshold (%d us) triggered on cpu %d with %.*f us!\n";
 		tracemark(line, g.app_name, g.trace_threshold, t->core_i,
@@ -526,10 +525,10 @@ static void write_summary(struct thread *t)
 			 (j == g.bucket_size - 1) ? " (including overflows)" : "");
 	}
 
-	putfieldp("Minimum", t[i].minlat, " (us)");
+	putfield("Minimum", t[i].minlat, ".3lf", " (us)");
 	putfield("Average", t[i].average, ".3lf", " (us)");
-	putfieldp("Maximum", t[i].maxlat, " (us)");
-	putfieldp("Max-Min", t[i].maxlat - t[i].minlat, " (us)");
+	putfield("Maximum", t[i].maxlat, ".3lf", " (us)");
+	putfield("Max-Min", t[i].maxlat - t[i].minlat, ".3lf", " (us)");
 	putfield("Duration", cycles_to_sec(&(t[i]), t[i].runtime),
 		 ".3f", " (sec)");
 	printf("\n");
@@ -547,9 +546,9 @@ static void write_summary_json(FILE *f, void *data)
 		fprintf(f, "    \"%lu\": {\n", i);
 		fprintf(f, "      \"cpu\": %d,\n", t[i].core_i);
 		fprintf(f, "      \"freq\": %d,\n", t[i].counter_mhz);
-		fprintf(f, "      \"min\": %" PRIu64 ",\n", t[i].minlat);
+		fprintf(f, "      \"min\": %3lf,\n", t[i].minlat);
 		fprintf(f, "      \"avg\": %3lf,\n", t[i].average);
-		fprintf(f, "      \"max\": %" PRIu64 ",\n", t[i].maxlat);
+		fprintf(f, "      \"max\": %3lf,\n", t[i].maxlat);
 		fprintf(f, "      \"duration\": %.3f,\n",
 			cycles_to_sec(&(t[i]), t[i].runtime));
 		fprintf(f, "      \"histogram\": {");
-- 
2.47.1

Re: [PATCH] Change min/max to float numbers

[Sebastian Andrzej Siewior]

On 2025-02-06 06:16:22 [-0300], Rafael Folco wrote:
> Make min/max consistent with avg by recording the latency samples
> as float numbers instead of integers.

Why?

> Signed-off-by: Rafael Folco <rfolco@redhat.com>

Sebastian

Re: [PATCH] Change min/max to float numbers

[Rafael Folco]

On 06/02/25 09:00, Sebastian Andrzej Siewior wrote:
> On 2025-02-06 06:16:22 [-0300], Rafael Folco wrote:
>> Make min/max consistent with avg by recording the latency samples
>> as float numbers instead of integers.
> 
> Why?

- More precise numbers of Max/Min for extreme low latency cases
- Distinguish zero latency in bucket 001 (us)
- Distinguish values from upper/lower boundaries in a bucket
- Clarify confusion on average (round up) vs maximum in the next bucket (casting)

Instead of:
        Core:    3 4 5
Counter Freq:    2100 2100 2100 (MHz)
    001 (us):    14880514 14880447 14880442
    002 (us):    0 0 0
    003 (us):    0 0 0
    004 (us):    0 0 0
    005 (us):    0 0 0 (including overflows)
     Minimum:    0 0 0 (us)
     Average:    1.000 1.000 1.000 (us)
     Maximum:    0 0 0 (us)
     Max-Min:    0 0 0 (us)

We'd see:
        Core:    3 4 5
Counter Freq:    2100 2100 2100 (MHz)
    001 (us):    12578842 12491108 12578752
    002 (us):    0 0 0
    003 (us):    0 0 0
    004 (us):    0 0 0
    005 (us):    0 0 0 (including overflows)
     Minimum:    0.033 0.035 0.035 (us)
     Average:    1.000 1.000 1.000 (us)
     Maximum:    0.129 0.153 0.134 (us)
     Max-Min:    0.095 0.118 0.099 (us)


> 
>> Signed-off-by: Rafael Folco <rfolco@redhat.com>
> 
> Sebastian
> 

Re: [PATCH] Change min/max to float numbers

[Sebastian Andrzej Siewior]

On 2025-02-06 12:02:49 [-0300], Rafael Folco wrote:
> On 06/02/25 09:00, Sebastian Andrzej Siewior wrote:
> > On 2025-02-06 06:16:22 [-0300], Rafael Folco wrote:
> >> Make min/max consistent with avg by recording the latency samples
> >> as float numbers instead of integers.
> > 
> > Why?
> 
> - More precise numbers of Max/Min for extreme low latency cases
> - Distinguish zero latency in bucket 001 (us)
> - Distinguish values from upper/lower boundaries in a bucket
> - Clarify confusion on average (round up) vs maximum in the next bucket (casting)
> 
> Instead of:
>         Core:    3 4 5
> Counter Freq:    2100 2100 2100 (MHz)
>     001 (us):    14880514 14880447 14880442
>     002 (us):    0 0 0
>     003 (us):    0 0 0
>     004 (us):    0 0 0
>     005 (us):    0 0 0 (including overflows)
>      Minimum:    0 0 0 (us)
>      Average:    1.000 1.000 1.000 (us)
>      Maximum:    0 0 0 (us)
>      Max-Min:    0 0 0 (us)
> 
> We'd see:
>         Core:    3 4 5
> Counter Freq:    2100 2100 2100 (MHz)
>     001 (us):    12578842 12491108 12578752
>     002 (us):    0 0 0
>     003 (us):    0 0 0
>     004 (us):    0 0 0
>     005 (us):    0 0 0 (including overflows)
>      Minimum:    0.033 0.035 0.035 (us)
>      Average:    1.000 1.000 1.000 (us)
>      Maximum:    0.129 0.153 0.134 (us)
>      Max-Min:    0.095 0.118 0.099 (us)

so core 3 has min 33ns and max 129ns while 1us on average?

Sebastian

Re: [PATCH] Change min/max to float numbers

[Rafael Folco]

On 06/02/25 12:09, Sebastian Andrzej Siewior wrote:
> On 2025-02-06 12:02:49 [-0300], Rafael Folco wrote:
>> On 06/02/25 09:00, Sebastian Andrzej Siewior wrote:
>>> On 2025-02-06 06:16:22 [-0300], Rafael Folco wrote:
>>>> Make min/max consistent with avg by recording the latency samples
>>>> as float numbers instead of integers.
>>>
>>> Why?
>>
>> - More precise numbers of Max/Min for extreme low latency cases
>> - Distinguish zero latency in bucket 001 (us)
>> - Distinguish values from upper/lower boundaries in a bucket
>> - Clarify confusion on average (round up) vs maximum in the next bucket (casting)
>>
>> Instead of:
>>         Core:    3 4 5
>> Counter Freq:    2100 2100 2100 (MHz)
>>     001 (us):    14880514 14880447 14880442
>>     002 (us):    0 0 0
>>     003 (us):    0 0 0
>>     004 (us):    0 0 0
>>     005 (us):    0 0 0 (including overflows)
>>      Minimum:    0 0 0 (us)
>>      Average:    1.000 1.000 1.000 (us)
>>      Maximum:    0 0 0 (us)
>>      Max-Min:    0 0 0 (us)
>>
>> We'd see:
>>         Core:    3 4 5
>> Counter Freq:    2100 2100 2100 (MHz)
>>     001 (us):    12578842 12491108 12578752
>>     002 (us):    0 0 0
>>     003 (us):    0 0 0
>>     004 (us):    0 0 0
>>     005 (us):    0 0 0 (including overflows)
>>      Minimum:    0.033 0.035 0.035 (us)
>>      Average:    1.000 1.000 1.000 (us)
>>      Maximum:    0.129 0.153 0.134 (us)
>>      Max-Min:    0.095 0.118 0.099 (us)
> 
> so core 3 has min 33ns and max 129ns while 1us on average?
Yes, because avg rounds up.
There are some other potential fixes coming like:
- stop rounding up avg
- splitting bucket 001 (us) which is bigger (0 - 1.99999)
- adding the max bucket to the end (do not limit to 32us)

... but these are out of scope for this change. 

Folco
> 
> Sebastian
> 

Re: [PATCH] Change min/max to float numbers

[Sebastian Andrzej Siewior]

On 2025-02-06 12:24:42 [-0300], Rafael Folco wrote:
> > so core 3 has min 33ns and max 129ns while 1us on average?
> Yes, because avg rounds up.
> There are some other potential fixes coming like:
> - stop rounding up avg
> - splitting bucket 001 (us) which is bigger (0 - 1.99999)
> - adding the max bucket to the end (do not limit to 32us)
> 
> ... but these are out of scope for this change. 

I'm not totally against floating point numbers here but you should add
the bullet numbers to your patch description. Also you might lose
precision over time if you deal with floating point.

However what you just showed me looks like the statistics part is
working right. I mean max lower than average. At least it is above min.

> Folco

Sebastian

Re: [PATCH] Change min/max to float numbers

[Rafael Folco]

On 06/02/25 12:36, Sebastian Andrzej Siewior wrote:
> On 2025-02-06 12:24:42 [-0300], Rafael Folco wrote:
>>> so core 3 has min 33ns and max 129ns while 1us on average?
>> Yes, because avg rounds up.
>> There are some other potential fixes coming like:
>> - stop rounding up avg
>> - splitting bucket 001 (us) which is bigger (0 - 1.99999)
>> - adding the max bucket to the end (do not limit to 32us)
>>
>> ... but these are out of scope for this change. 
> 
> I'm not totally against floating point numbers here but you should add
> the bullet numbers to your patch description. Also you might lose
> precision over time if you deal with floating point.
I will add more context to the patch description.

> 
> However what you just showed me looks like the statistics part is
> working right. I mean max lower than average. At least it is above min.
This patch just reduces this distortion with Avg=1.000 with Max=0.153. Still not ideal though, but better than Avg=1.000 and Max=0. The idea was to propose another change for Avg, like Avg=0.087 instead. I can include it here if that's the case.

Thanks,

Folco


> 
>> Folco
> 
> Sebastian
> 

Re: [PATCH] Change min/max to float numbers

[Crystal Wood]

On Thu, 2025-02-06 at 12:02 -0300, Rafael Folco wrote:
> On 06/02/25 09:00, Sebastian Andrzej Siewior wrote:
> > On 2025-02-06 06:16:22 [-0300], Rafael Folco wrote:
> > > Make min/max consistent with avg by recording the latency samples
> > > as float numbers instead of integers.
> > 
> > Why?
> 
> - More precise numbers of Max/Min for extreme low latency cases
> - Distinguish zero latency in bucket 001 (us)
> - Distinguish values from upper/lower boundaries in a bucket
> - Clarify confusion on average (round up) vs maximum in the next bucket (casting)

If you want more precision, set --bucket-width to something lower than
1000 ns.  That will cause everything to be reported with ns precision
(though maybe not quite ns accuracy, depending on the hardware).  We
were hesitant to make ns precision the default in case it misleads
people about the accuracy (plus I didn't know if the change might
confuse some tools that consume the output).

I don't see the need to change things here -- and I definitely don't see
the need to hold on to the unit_per_us stuff if we're going to always
print ns resolution, regardless of whether we use fp.

-Crystal

Re: [PATCH] Change min/max to float numbers

[Rafael Folco]

On 06/02/25 18:23, Crystal Wood wrote:
> On Thu, 2025-02-06 at 12:02 -0300, Rafael Folco wrote:
>> On 06/02/25 09:00, Sebastian Andrzej Siewior wrote:
>>> On 2025-02-06 06:16:22 [-0300], Rafael Folco wrote:
>>>> Make min/max consistent with avg by recording the latency samples
>>>> as float numbers instead of integers.
>>>
>>> Why?
>>
>> - More precise numbers of Max/Min for extreme low latency cases
>> - Distinguish zero latency in bucket 001 (us)
>> - Distinguish values from upper/lower boundaries in a bucket
>> - Clarify confusion on average (round up) vs maximum in the next bucket (casting)
> 
> If you want more precision, set --bucket-width to something lower than
> 1000 ns.  That will cause everything to be reported with ns precision
> (though maybe not quite ns accuracy, depending on the hardware).  We
> were hesitant to make ns precision the default in case it misleads
> people about the accuracy (plus I didn't know if the change might
> confuse some tools that consume the output).
Yes, it works for precision. Average is higher than max with default bucket width, though.
> 
> I don't see the need to change things here -- and I definitely don't see
> the need to hold on to the unit_per_us stuff if we're going to always
> print ns resolution, regardless of whether we use fp.
Ack
Thanks for reviewing.

-Folco
> 
> -Crystal
> 

Re: [PATCH] Change min/max to float numbers

[Crystal Wood]

On Fri, 2025-02-07 at 11:20 -0300, Rafael Folco wrote:
> On 06/02/25 18:23, Crystal Wood wrote:
> > On Thu, 2025-02-06 at 12:02 -0300, Rafael Folco wrote:
> > > On 06/02/25 09:00, Sebastian Andrzej Siewior wrote:
> > > > On 2025-02-06 06:16:22 [-0300], Rafael Folco wrote:
> > > > > Make min/max consistent with avg by recording the latency samples
> > > > > as float numbers instead of integers.
> > > > 
> > > > Why?
> > > 
> > > - More precise numbers of Max/Min for extreme low latency cases
> > > - Distinguish zero latency in bucket 001 (us)
> > > - Distinguish values from upper/lower boundaries in a bucket
> > > - Clarify confusion on average (round up) vs maximum in the next bucket (casting)
> > 
> > If you want more precision, set --bucket-width to something lower than
> > 1000 ns.  That will cause everything to be reported with ns precision
> > (though maybe not quite ns accuracy, depending on the hardware).  We
> > were hesitant to make ns precision the default in case it misleads
> > people about the accuracy (plus I didn't know if the change might
> > confuse some tools that consume the output).
> Yes, it works for precision. Average is higher than max with default bucket width, though.

This is due to the average being calculated from the histogram (rounding
to the top of the bucket), while min/max is the direct value.  Maybe we
should keep a running total to compute average from?

We may also want to think about whether we should unconditionally use
ns.

-Crystal