Although using integer comparison to compare doubles kind of
works, it's annoying to see domains slightly out of order when
sorting by cpu%.
Add a compare_dbl() function and update compare_cpu_pct() to
call it.
Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
---
tools/xentop/xentop.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
index 545bd5e96d..99199caec9 100644
--- a/tools/xentop/xentop.c
+++ b/tools/xentop/xentop.c
@@ -85,6 +85,7 @@ static void set_delay(const char *value);
static void set_prompt(const char *new_prompt, void (*func)(const char *));
static int handle_key(int);
static int compare(unsigned long long, unsigned long long);
+static int compare_dbl(double, double);
static int compare_domains(xenstat_domain **, xenstat_domain **);
static unsigned long long tot_net_bytes( xenstat_domain *, int);
static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long long *);
@@ -422,6 +423,16 @@ static int compare(unsigned long long i1, unsigned long long i2)
return 0;
}
+/* Compares two double precision numbers, returning -1,0,1 for <,=,> */
+static int compare_dbl(double d1, double d2)
+{
+ if(d1 < d2)
+ return -1;
+ if(d1 > d2)
+ return 1;
+ return 0;
+}
+
/* Comparison function for use with qsort. Compares two domains using the
* current sort field. */
static int compare_domains(xenstat_domain **domain1, xenstat_domain **domain2)
@@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain *domain)
static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain *domain2)
{
- return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
+ return -compare_dbl(get_cpu_pct(domain1), get_cpu_pct(domain2));
}
/* Prints cpu percentage statistic */
--
2.39.2On 14/05/2024 9:13 am, Leigh Brown wrote:
> Although using integer comparison to compare doubles kind of
> works, it's annoying to see domains slightly out of order when
> sorting by cpu%.
>
> Add a compare_dbl() function and update compare_cpu_pct() to
> call it.
>
> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
> ---
> tools/xentop/xentop.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
> index 545bd5e96d..99199caec9 100644
> --- a/tools/xentop/xentop.c
> +++ b/tools/xentop/xentop.c
> @@ -85,6 +85,7 @@ static void set_delay(const char *value);
> static void set_prompt(const char *new_prompt, void (*func)(const char *));
> static int handle_key(int);
> static int compare(unsigned long long, unsigned long long);
> +static int compare_dbl(double, double);
> static int compare_domains(xenstat_domain **, xenstat_domain **);
> static unsigned long long tot_net_bytes( xenstat_domain *, int);
> static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long long *);
> @@ -422,6 +423,16 @@ static int compare(unsigned long long i1, unsigned long long i2)
> return 0;
> }
>
> +/* Compares two double precision numbers, returning -1,0,1 for <,=,> */
> +static int compare_dbl(double d1, double d2)
> +{
> + if(d1 < d2)
> + return -1;
> + if(d1 > d2)
> + return 1;
> + return 0;
> +}
> +
> /* Comparison function for use with qsort. Compares two domains using the
> * current sort field. */
> static int compare_domains(xenstat_domain **domain1, xenstat_domain **domain2)
> @@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain *domain)
>
> static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain *domain2)
> {
> - return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
> + return -compare_dbl(get_cpu_pct(domain1), get_cpu_pct(domain2));
Oh, we were doing an implicit double->unsigned long long conversion.
Over the range 0.0 to 100.0, that ought to work as expected. What kind
of out-of-order are you seeing?
Nevertheless, this should comparison should clearly be done using
doubles. AFACT, get_cpu_pct() shouldn't ever return a NaN, so I think
this simple form is fine.
Oleksii: This is another bugfix to xentop, and should be considered for
4.19 at this point.
~Andrew
On 2024-05-14 13:07, Andrew Cooper wrote:
> On 14/05/2024 9:13 am, Leigh Brown wrote:
>> Although using integer comparison to compare doubles kind of
>> works, it's annoying to see domains slightly out of order when
>> sorting by cpu%.
>>
>> Add a compare_dbl() function and update compare_cpu_pct() to
>> call it.
>>
>> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
>> ---
>> tools/xentop/xentop.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
>> index 545bd5e96d..99199caec9 100644
>> --- a/tools/xentop/xentop.c
>> +++ b/tools/xentop/xentop.c
>> @@ -85,6 +85,7 @@ static void set_delay(const char *value);
>> static void set_prompt(const char *new_prompt, void (*func)(const
>> char *));
>> static int handle_key(int);
>> static int compare(unsigned long long, unsigned long long);
>> +static int compare_dbl(double, double);
>> static int compare_domains(xenstat_domain **, xenstat_domain **);
>> static unsigned long long tot_net_bytes( xenstat_domain *, int);
>> static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long long
>> *);
>> @@ -422,6 +423,16 @@ static int compare(unsigned long long i1,
>> unsigned long long i2)
>> return 0;
>> }
>>
>> +/* Compares two double precision numbers, returning -1,0,1 for <,=,>
>> */
>> +static int compare_dbl(double d1, double d2)
>> +{
>> + if(d1 < d2)
>> + return -1;
>> + if(d1 > d2)
>> + return 1;
>> + return 0;
>> +}
>> +
>> /* Comparison function for use with qsort. Compares two domains
>> using the
>> * current sort field. */
>> static int compare_domains(xenstat_domain **domain1, xenstat_domain
>> **domain2)
>> @@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain *domain)
>>
>> static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain
>> *domain2)
>> {
>> - return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
>> + return -compare_dbl(get_cpu_pct(domain1), get_cpu_pct(domain2));
>
> Oh, we were doing an implicit double->unsigned long long conversion.
> Over the range 0.0 to 100.0, that ought to work as expected. What kind
> of out-of-order are you seeing?
>
> Nevertheless, this should comparison should clearly be done using
> doubles. AFACT, get_cpu_pct() shouldn't ever return a NaN, so I think
> this simple form is fine.
>
> Oleksii: This is another bugfix to xentop, and should be considered for
> 4.19 at this point.
>
> ~Andrew
Perhaps I overthought it, and this approach might be better:
--- a/tools/xentop/xentop.c
+++ b/tools/xentop/xentop.c
@@ -523,7 +523,8 @@ static double get_cpu_pct(xenstat_domain *domain)
static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain
*domain2)
{
- return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
+ return -compare(get_cpu_pct(domain1) * 100.0,
+ get_cpu_pct(domain2) * 100.0);
}
/* Prints cpu percentage statistic */
Hello,
On 2024-05-14 13:07, Andrew Cooper wrote:
> On 14/05/2024 9:13 am, Leigh Brown wrote:
>> Although using integer comparison to compare doubles kind of
>> works, it's annoying to see domains slightly out of order when
>> sorting by cpu%.
>>
>> Add a compare_dbl() function and update compare_cpu_pct() to
>> call it.
>>
>> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
>> ---
>> tools/xentop/xentop.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
>> index 545bd5e96d..99199caec9 100644
>> --- a/tools/xentop/xentop.c
>> +++ b/tools/xentop/xentop.c
>> @@ -85,6 +85,7 @@ static void set_delay(const char *value);
>> static void set_prompt(const char *new_prompt, void (*func)(const
>> char *));
>> static int handle_key(int);
>> static int compare(unsigned long long, unsigned long long);
>> +static int compare_dbl(double, double);
>> static int compare_domains(xenstat_domain **, xenstat_domain **);
>> static unsigned long long tot_net_bytes( xenstat_domain *, int);
>> static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long long
>> *);
>> @@ -422,6 +423,16 @@ static int compare(unsigned long long i1,
>> unsigned long long i2)
>> return 0;
>> }
>>
>> +/* Compares two double precision numbers, returning -1,0,1 for <,=,>
>> */
>> +static int compare_dbl(double d1, double d2)
>> +{
>> + if(d1 < d2)
>> + return -1;
>> + if(d1 > d2)
>> + return 1;
>> + return 0;
>> +}
>> +
>> /* Comparison function for use with qsort. Compares two domains
>> using the
>> * current sort field. */
>> static int compare_domains(xenstat_domain **domain1, xenstat_domain
>> **domain2)
>> @@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain *domain)
>>
>> static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain
>> *domain2)
>> {
>> - return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
>> + return -compare_dbl(get_cpu_pct(domain1), get_cpu_pct(domain2));
>
> Oh, we were doing an implicit double->unsigned long long conversion.
> Over the range 0.0 to 100.0, that ought to work as expected. What kind
> of out-of-order are you seeing?
Without patch:
xentop - 13:29:01 Xen 4.18.2
13 domains: 1 running, 12 blocked, 0 paused, 0 crashed, 0 dying, 0
shutdown
Mem: 67030640k total, 33097800k used, 33932840k free CPUs: 24 @
3693MHz
NAME STATE CPU(sec) CPU(%) MEM(k) MEM(%) MAXMEM(k)
MAXMEM(%)
icecream --b--- 2597 6.6 4194368 6.3 4195328
6.3
xendd --b--- 4016 5.4 524268 0.8 525312
0.8
Domain-0 -----r 1059 1.7 1048576 1.6 1048576
1.6
neon --b--- 826 1.1 2097216 3.1 2098176
3.1
blender --b--- 121 0.2 1048640 1.6 1049600
1.6
bread --b--- 69 0.1 524352 0.8 525312
0.8
bob --b--- 502 0.3 16777284 25.0 16778240
25.0
cheese --b--- 225 0.5 1048384 1.6 1049600
1.6
cassini --b--- 489 0.4 3145792 4.7 3146752
4.7
chickpea --b--- 67 0.1 524352 0.8 525312
0.8
lentil --b--- 67 0.1 262208 0.4 263168
0.4
fusilli --b--- 159 0.2 524352 0.8 525312
0.8
pizza --b--- 359 0.5 524352 0.8 525312
0.8
With patch:
xentop - 13:30:17 Xen 4.18.2
13 domains: 1 running, 12 blocked, 0 paused, 0 crashed, 0 dying, 0
shutdown
Mem: 67030640k total, 33097788k used, 33932852k free CPUs: 24 @
3693MHz
NAME STATE CPU(sec) CPU(%) MEM(k) MEM(%) MAXMEM(k)
MAXMEM(%)
xendd --b--- 4020 5.7 524268 0.8 525312
0.8
icecream --b--- 2600 3.8 4194368 6.3 4195328
6.3
Domain-0 -----r 1060 1.5 1048576 1.6 1048576
1.6
neon --b--- 827 1.1 2097216 3.1 2098176
3.1
cheese --b--- 225 0.7 1048384 1.6 1049600
1.6
pizza --b--- 359 0.5 524352 0.8 525312
0.8
cassini --b--- 490 0.4 3145792 4.7 3146752
4.7
fusilli --b--- 159 0.2 524352 0.8 525312
0.8
bob --b--- 502 0.2 16777284 25.0 16778240
25.0
blender --b--- 121 0.2 1048640 1.6 1049600
1.6
bread --b--- 69 0.1 524352 0.8 525312
0.8
chickpea --b--- 67 0.1 524352 0.8 525312
0.8
lentil --b--- 67 0.1 262208 0.4 263168
0.4
> Nevertheless, this should comparison should clearly be done using
> doubles. AFACT, get_cpu_pct() shouldn't ever return a NaN, so I think
> this simple form is fine.
>
> Oleksii: This is another bugfix to xentop, and should be considered for
> 4.19 at this point.
>
> ~Andrew
Regards,
Leigh.
On 14/05/2024 1:36 pm, Leigh Brown wrote:
> Hello,
>
> On 2024-05-14 13:07, Andrew Cooper wrote:
>> On 14/05/2024 9:13 am, Leigh Brown wrote:
>>> Although using integer comparison to compare doubles kind of
>>> works, it's annoying to see domains slightly out of order when
>>> sorting by cpu%.
>>>
>>> Add a compare_dbl() function and update compare_cpu_pct() to
>>> call it.
>>>
>>> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
>>> ---
>>> tools/xentop/xentop.c | 13 ++++++++++++-
>>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
>>> index 545bd5e96d..99199caec9 100644
>>> --- a/tools/xentop/xentop.c
>>> +++ b/tools/xentop/xentop.c
>>> @@ -85,6 +85,7 @@ static void set_delay(const char *value);
>>> static void set_prompt(const char *new_prompt, void (*func)(const
>>> char *));
>>> static int handle_key(int);
>>> static int compare(unsigned long long, unsigned long long);
>>> +static int compare_dbl(double, double);
>>> static int compare_domains(xenstat_domain **, xenstat_domain **);
>>> static unsigned long long tot_net_bytes( xenstat_domain *, int);
>>> static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long long *);
>>> @@ -422,6 +423,16 @@ static int compare(unsigned long long i1,
>>> unsigned long long i2)
>>> return 0;
>>> }
>>>
>>> +/* Compares two double precision numbers, returning -1,0,1 for <,=,> */
>>> +static int compare_dbl(double d1, double d2)
>>> +{
>>> + if(d1 < d2)
>>> + return -1;
>>> + if(d1 > d2)
>>> + return 1;
>>> + return 0;
>>> +}
>>> +
>>> /* Comparison function for use with qsort. Compares two domains
>>> using the
>>> * current sort field. */
>>> static int compare_domains(xenstat_domain **domain1, xenstat_domain
>>> **domain2)
>>> @@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain *domain)
>>>
>>> static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain
>>> *domain2)
>>> {
>>> - return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
>>> + return -compare_dbl(get_cpu_pct(domain1), get_cpu_pct(domain2));
>>
>> Oh, we were doing an implicit double->unsigned long long conversion.
>> Over the range 0.0 to 100.0, that ought to work as expected. What kind
>> of out-of-order are you seeing?
>
> Without patch:
>
> xentop - 13:29:01 Xen 4.18.2
> 13 domains: 1 running, 12 blocked, 0 paused, 0 crashed, 0 dying, 0 shutdown
> Mem: 67030640k total, 33097800k used, 33932840k free CPUs: 24 @ 3693MHz
> NAME STATE CPU(sec) CPU(%) MEM(k) MEM(%) MAXMEM(k) MAXMEM(%)
> icecream --b--- 2597 6.6 4194368 6.3 4195328 6.3
> xendd --b--- 4016 5.4 524268 0.8 525312 0.8
> Domain-0 -----r 1059 1.7 1048576 1.6 1048576 1.6
> neon --b--- 826 1.1 2097216 3.1 2098176 3.1
> blender --b--- 121 0.2 1048640 1.6 1049600 1.6
> bread --b--- 69 0.1 524352 0.8 525312 0.8
> bob --b--- 502 0.3 16777284 25.0 16778240 25.0
> cheese --b--- 225 0.5 1048384 1.6 1049600 1.6
> cassini --b--- 489 0.4 3145792 4.7 3146752 4.7
> chickpea --b--- 67 0.1 524352 0.8 525312 0.8
> lentil --b--- 67 0.1 262208 0.4 263168 0.4
> fusilli --b--- 159 0.2 524352 0.8 525312 0.8
> pizza --b--- 359 0.5 524352 0.8 525312 0.8
>
> With patch:
>
> xentop - 13:30:17 Xen 4.18.2
> 13 domains: 1 running, 12 blocked, 0 paused, 0 crashed, 0 dying, 0 shutdown
> Mem: 67030640k total, 33097788k used, 33932852k free CPUs: 24 @ 3693MHz
> NAME STATE CPU(sec) CPU(%) MEM(k) MEM(%) MAXMEM(k) MAXMEM(%)
> xendd --b--- 4020 5.7 524268 0.8 525312 0.8
> icecream --b--- 2600 3.8 4194368 6.3 4195328 6.3
> Domain-0 -----r 1060 1.5 1048576 1.6 1048576 1.6
> neon --b--- 827 1.1 2097216 3.1 2098176 3.1
> cheese --b--- 225 0.7 1048384 1.6 1049600 1.6
> pizza --b--- 359 0.5 524352 0.8 525312 0.8
> cassini --b--- 490 0.4 3145792 4.7 3146752 4.7
> fusilli --b--- 159 0.2 524352 0.8 525312 0.8
> bob --b--- 502 0.2 16777284 25.0 16778240 25.0
> blender --b--- 121 0.2 1048640 1.6 1049600 1.6
> bread --b--- 69 0.1 524352 0.8 525312 0.8
> chickpea --b--- 67 0.1 524352 0.8 525312 0.8
> lentil --b--- 67 0.1 262208 0.4 263168 0.4
Ah, so it's the rounding, and a straight cast discards the fractional part.
I think your patch is fine, although it could do with a mention of why
this goes wrong in the commit message. I'm happy to adjust on commit.
~Andrew
On Tue, 2024-05-14 at 14:52 +0100, Andrew Cooper wrote:
> On 14/05/2024 1:36 pm, Leigh Brown wrote:
> > Hello,
> >
> > On 2024-05-14 13:07, Andrew Cooper wrote:
> > > On 14/05/2024 9:13 am, Leigh Brown wrote:
> > > > Although using integer comparison to compare doubles kind of
> > > > works, it's annoying to see domains slightly out of order when
> > > > sorting by cpu%.
> > > >
> > > > Add a compare_dbl() function and update compare_cpu_pct() to
> > > > call it.
> > > >
> > > > Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
> > > > ---
> > > > tools/xentop/xentop.c | 13 ++++++++++++-
> > > > 1 file changed, 12 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
> > > > index 545bd5e96d..99199caec9 100644
> > > > --- a/tools/xentop/xentop.c
> > > > +++ b/tools/xentop/xentop.c
> > > > @@ -85,6 +85,7 @@ static void set_delay(const char *value);
> > > > static void set_prompt(const char *new_prompt, void
> > > > (*func)(const
> > > > char *));
> > > > static int handle_key(int);
> > > > static int compare(unsigned long long, unsigned long long);
> > > > +static int compare_dbl(double, double);
> > > > static int compare_domains(xenstat_domain **, xenstat_domain
> > > > **);
> > > > static unsigned long long tot_net_bytes( xenstat_domain *,
> > > > int);
> > > > static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long
> > > > long *);
> > > > @@ -422,6 +423,16 @@ static int compare(unsigned long long i1,
> > > > unsigned long long i2)
> > > > return 0;
> > > > }
> > > >
> > > > +/* Compares two double precision numbers, returning -1,0,1 for
> > > > <,=,> */
> > > > +static int compare_dbl(double d1, double d2)
> > > > +{
> > > > + if(d1 < d2)
> > > > + return -1;
> > > > + if(d1 > d2)
> > > > + return 1;
> > > > + return 0;
> > > > +}
> > > > +
> > > > /* Comparison function for use with qsort. Compares two
> > > > domains
> > > > using the
> > > > * current sort field. */
> > > > static int compare_domains(xenstat_domain **domain1,
> > > > xenstat_domain
> > > > **domain2)
> > > > @@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain
> > > > *domain)
> > > >
> > > > static int compare_cpu_pct(xenstat_domain *domain1,
> > > > xenstat_domain
> > > > *domain2)
> > > > {
> > > > - return -compare(get_cpu_pct(domain1),
> > > > get_cpu_pct(domain2));
> > > > + return -compare_dbl(get_cpu_pct(domain1),
> > > > get_cpu_pct(domain2));
> > >
> > > Oh, we were doing an implicit double->unsigned long long
> > > conversion.
> > > Over the range 0.0 to 100.0, that ought to work as expected.
> > > What kind
> > > of out-of-order are you seeing?
> >
> > Without patch:
> >
> > xentop - 13:29:01 Xen 4.18.2
> > 13 domains: 1 running, 12 blocked, 0 paused, 0 crashed, 0 dying, 0
> > shutdown
> > Mem: 67030640k total, 33097800k used, 33932840k free CPUs: 24 @
> > 3693MHz
> > NAME STATE CPU(sec) CPU(%) MEM(k) MEM(%) MAXMEM(k)
> > MAXMEM(%)
> > icecream --b--- 2597 6.6 4194368 6.3
> > 4195328 6.3
> > xendd --b--- 4016 5.4 524268 0.8
> > 525312 0.8
> > Domain-0 -----r 1059 1.7 1048576 1.6
> > 1048576 1.6
> > neon --b--- 826 1.1 2097216 3.1
> > 2098176 3.1
> > blender --b--- 121 0.2 1048640 1.6
> > 1049600 1.6
> > bread --b--- 69 0.1 524352 0.8
> > 525312 0.8
> > bob --b--- 502 0.3 16777284 25.0
> > 16778240 25.0
> > cheese --b--- 225 0.5 1048384 1.6
> > 1049600 1.6
> > cassini --b--- 489 0.4 3145792 4.7
> > 3146752 4.7
> > chickpea --b--- 67 0.1 524352 0.8
> > 525312 0.8
> > lentil --b--- 67 0.1 262208 0.4
> > 263168 0.4
> > fusilli --b--- 159 0.2 524352 0.8
> > 525312 0.8
> > pizza --b--- 359 0.5 524352 0.8
> > 525312 0.8
> >
> > With patch:
> >
> > xentop - 13:30:17 Xen 4.18.2
> > 13 domains: 1 running, 12 blocked, 0 paused, 0 crashed, 0 dying, 0
> > shutdown
> > Mem: 67030640k total, 33097788k used, 33932852k free CPUs: 24 @
> > 3693MHz
> > NAME STATE CPU(sec) CPU(%) MEM(k) MEM(%) MAXMEM(k)
> > MAXMEM(%)
> > xendd --b--- 4020 5.7 524268 0.8
> > 525312 0.8
> > icecream --b--- 2600 3.8 4194368 6.3
> > 4195328 6.3
> > Domain-0 -----r 1060 1.5 1048576 1.6
> > 1048576 1.6
> > neon --b--- 827 1.1 2097216 3.1
> > 2098176 3.1
> > cheese --b--- 225 0.7 1048384 1.6
> > 1049600 1.6
> > pizza --b--- 359 0.5 524352 0.8
> > 525312 0.8
> > cassini --b--- 490 0.4 3145792 4.7
> > 3146752 4.7
> > fusilli --b--- 159 0.2 524352 0.8
> > 525312 0.8
> > bob --b--- 502 0.2 16777284 25.0
> > 16778240 25.0
> > blender --b--- 121 0.2 1048640 1.6
> > 1049600 1.6
> > bread --b--- 69 0.1 524352 0.8
> > 525312 0.8
> > chickpea --b--- 67 0.1 524352 0.8
> > 525312 0.8
> > lentil --b--- 67 0.1 262208 0.4
> > 263168 0.4
>
>
> Ah, so it's the rounding, and a straight cast discards the fractional
> part.
>
> I think your patch is fine, although it could do with a mention of
> why
> this goes wrong in the commit message. I'm happy to adjust on
> commit.
Feel free to merge it as I am considering it as bugfix:
Release-acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
~ Oleksii
>
> ~Andrew
On 14.05.2024 14:07, Andrew Cooper wrote:
> On 14/05/2024 9:13 am, Leigh Brown wrote:
>> Although using integer comparison to compare doubles kind of
>> works, it's annoying to see domains slightly out of order when
>> sorting by cpu%.
>>
>> Add a compare_dbl() function and update compare_cpu_pct() to
>> call it.
>>
>> Signed-off-by: Leigh Brown <leigh@solinno.co.uk>
>> ---
>> tools/xentop/xentop.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
>> index 545bd5e96d..99199caec9 100644
>> --- a/tools/xentop/xentop.c
>> +++ b/tools/xentop/xentop.c
>> @@ -85,6 +85,7 @@ static void set_delay(const char *value);
>> static void set_prompt(const char *new_prompt, void (*func)(const char *));
>> static int handle_key(int);
>> static int compare(unsigned long long, unsigned long long);
>> +static int compare_dbl(double, double);
>> static int compare_domains(xenstat_domain **, xenstat_domain **);
>> static unsigned long long tot_net_bytes( xenstat_domain *, int);
>> static bool tot_vbd_reqs(xenstat_domain *, int, unsigned long long *);
>> @@ -422,6 +423,16 @@ static int compare(unsigned long long i1, unsigned long long i2)
>> return 0;
>> }
>>
>> +/* Compares two double precision numbers, returning -1,0,1 for <,=,> */
>> +static int compare_dbl(double d1, double d2)
>> +{
>> + if(d1 < d2)
>> + return -1;
>> + if(d1 > d2)
>> + return 1;
>> + return 0;
>> +}
>> +
>> /* Comparison function for use with qsort. Compares two domains using the
>> * current sort field. */
>> static int compare_domains(xenstat_domain **domain1, xenstat_domain **domain2)
>> @@ -523,7 +534,7 @@ static double get_cpu_pct(xenstat_domain *domain)
>>
>> static int compare_cpu_pct(xenstat_domain *domain1, xenstat_domain *domain2)
>> {
>> - return -compare(get_cpu_pct(domain1), get_cpu_pct(domain2));
>> + return -compare_dbl(get_cpu_pct(domain1), get_cpu_pct(domain2));
>
> Oh, we were doing an implicit double->unsigned long long conversion.
> Over the range 0.0 to 100.0, that ought to work as expected. What kind
> of out-of-order are you seeing?
>
> Nevertheless, this should comparison should clearly be done using
> doubles. AFACT, get_cpu_pct() shouldn't ever return a NaN, so I think
> this simple form is fine.
Just for completeness: INF would be similarly an issue, but hopefully cannot
come back from get_cpu_pct() either.
Jan
© 2016 - 2024 Red Hat, Inc.