* [PATCH] xfs_io: fix operation time reporting
@ 2018-03-01 3:56 Dave Chinner
2018-03-02 4:00 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2018-03-01 3:56 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
CUrrently the 100th/sec units always report zero, such as:
32 MiB, 8192 ops; 0:00:21.00 (1.476 MiB/sec and 377.9260 ops/sec)
^^
This is incorrect. Fix the maths that is wrong by removing all the
unnecesary floating point maths and just using basic integer
division...
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
libxcmd/input.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/libxcmd/input.c b/libxcmd/input.c
index 441bb2fbbf34..6e7a8c9822ee 100644
--- a/libxcmd/input.c
+++ b/libxcmd/input.c
@@ -154,9 +154,10 @@ tdiv(double value, struct timeval tv)
return value / ((double)tv.tv_sec + ((double)tv.tv_usec / 1000000.0));
}
-#define HOURS(sec) ((sec) / (60 * 60))
-#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
-#define SECONDS(sec) ((sec) % 60)
+#define HOURS(sec) ((sec) / (60 * 60))
+#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
+#define SECONDS(sec) ((sec) % 60)
+#define USEC_TO_100THS(usec) ((usec) / 1000 / 10)
void
timestr(
@@ -165,14 +166,12 @@ timestr(
size_t size,
int format)
{
- double usec = (double)tv->tv_usec / 1000000.0;
-
if (format & TERSE_FIXED_TIME) {
if (!HOURS(tv->tv_sec)) {
snprintf(ts, size, "%u:%02u.%02u",
(unsigned int) MINUTES(tv->tv_sec),
(unsigned int) SECONDS(tv->tv_sec),
- (unsigned int) usec * 100);
+ (unsigned int) USEC_TO_100THS(tv->tv_usec));
return;
}
format |= VERBOSE_FIXED_TIME; /* fallback if hours needed */
@@ -183,9 +182,10 @@ timestr(
(unsigned int) HOURS(tv->tv_sec),
(unsigned int) MINUTES(tv->tv_sec),
(unsigned int) SECONDS(tv->tv_sec),
- (unsigned int) usec * 100);
+ (unsigned int) USEC_TO_100THS(tv->tv_usec));
} else {
- snprintf(ts, size, "0.%04u sec", (unsigned int) usec * 10000);
+ snprintf(ts, size, "0.%04u sec",
+ (unsigned int) tv->tv_usec / 100);
}
}
--
2.16.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_io: fix operation time reporting
2018-03-01 3:56 [PATCH] xfs_io: fix operation time reporting Dave Chinner
@ 2018-03-02 4:00 ` Eric Sandeen
2018-03-02 21:49 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2018-03-02 4:00 UTC (permalink / raw)
To: Dave Chinner, linux-xfs
On 2/28/18 9:56 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> CUrrently the 100th/sec units always report zero, such as:
>
> 32 MiB, 8192 ops; 0:00:21.00 (1.476 MiB/sec and 377.9260 ops/sec)
> ^^
>
> This is incorrect. Fix the maths that is wrong by removing all the
> unnecesary floating point maths and just using basic integer
> division...
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
> libxcmd/input.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/libxcmd/input.c b/libxcmd/input.c
> index 441bb2fbbf34..6e7a8c9822ee 100644
> --- a/libxcmd/input.c
> +++ b/libxcmd/input.c
> @@ -154,9 +154,10 @@ tdiv(double value, struct timeval tv)
> return value / ((double)tv.tv_sec + ((double)tv.tv_usec / 1000000.0));
> }
>
> -#define HOURS(sec) ((sec) / (60 * 60))
> -#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
> -#define SECONDS(sec) ((sec) % 60)
> +#define HOURS(sec) ((sec) / (60 * 60))
> +#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
> +#define SECONDS(sec) ((sec) % 60)
> +#define USEC_TO_100THS(usec) ((usec) / 1000 / 10)
I guess this works but I expected to convert "microseconds to 100ths"
via a conversion like:
usec * 1sec/1000000usec * 100 hundredths/1sec
so just for readability I'd have expected:
#define USEC_TO_100THS(usec) ((usec) / 1000000 * 100)
or possibly just
#define USEC_TO_100THS(usec) ((usec) / 10000)
... I'm confused by your choice of orders of magnitude above even
though it works - it seems a bit random to divide it that way,
unless I'm missing something?
(I had a physics teacher who drilled THINK UNITS BEFORE YOU THINK
NUMBERS" into my head an I still do) :)
> void
> timestr(
> @@ -165,14 +166,12 @@ timestr(
> size_t size,
> int format)
> {
> - double usec = (double)tv->tv_usec / 1000000.0;
> -
> if (format & TERSE_FIXED_TIME) {
> if (!HOURS(tv->tv_sec)) {
> snprintf(ts, size, "%u:%02u.%02u",
> (unsigned int) MINUTES(tv->tv_sec),
> (unsigned int) SECONDS(tv->tv_sec),
> - (unsigned int) usec * 100);
> + (unsigned int) USEC_TO_100THS(tv->tv_usec));
> return;
> }
> format |= VERBOSE_FIXED_TIME; /* fallback if hours needed */
> @@ -183,9 +182,10 @@ timestr(
> (unsigned int) HOURS(tv->tv_sec),
> (unsigned int) MINUTES(tv->tv_sec),
> (unsigned int) SECONDS(tv->tv_sec),
> - (unsigned int) usec * 100);
> + (unsigned int) USEC_TO_100THS(tv->tv_usec));
> } else {
> - snprintf(ts, size, "0.%04u sec", (unsigned int) usec * 10000);
> + snprintf(ts, size, "0.%04u sec",
> + (unsigned int) tv->tv_usec / 100);
USEC_TO_10000THS() for consistency ?
> }
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_io: fix operation time reporting
2018-03-02 4:00 ` Eric Sandeen
@ 2018-03-02 21:49 ` Dave Chinner
2018-03-02 21:52 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2018-03-02 21:49 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
On Thu, Mar 01, 2018 at 10:00:52PM -0600, Eric Sandeen wrote:
> On 2/28/18 9:56 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > CUrrently the 100th/sec units always report zero, such as:
> >
> > 32 MiB, 8192 ops; 0:00:21.00 (1.476 MiB/sec and 377.9260 ops/sec)
> > ^^
> >
> > This is incorrect. Fix the maths that is wrong by removing all the
> > unnecesary floating point maths and just using basic integer
> > division...
> >
> > Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> > ---
> > libxcmd/input.c | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/libxcmd/input.c b/libxcmd/input.c
> > index 441bb2fbbf34..6e7a8c9822ee 100644
> > --- a/libxcmd/input.c
> > +++ b/libxcmd/input.c
> > @@ -154,9 +154,10 @@ tdiv(double value, struct timeval tv)
> > return value / ((double)tv.tv_sec + ((double)tv.tv_usec / 1000000.0));
> > }
> >
> > -#define HOURS(sec) ((sec) / (60 * 60))
> > -#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
> > -#define SECONDS(sec) ((sec) % 60)
> > +#define HOURS(sec) ((sec) / (60 * 60))
> > +#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
> > +#define SECONDS(sec) ((sec) % 60)
> > +#define USEC_TO_100THS(usec) ((usec) / 1000 / 10)
>
> I guess this works but I expected to convert "microseconds to 100ths"
> via a conversion like:
>
> usec * 1sec/1000000usec * 100 hundredths/1sec
That's what the old code tried to do with floating point math and
casts, but that didn't work.
> so just for readability I'd have expected:
>
> #define USEC_TO_100THS(usec) ((usec) / 1000000 * 100)
Welcome to Integer Math 101: Integer Division. You'll always get
zero, because N / M = 0 when N < M.
> or possibly just
> #define USEC_TO_100THS(usec) ((usec) / 10000)
>
> ... I'm confused by your choice of orders of magnitude above even
> though it works - it seems a bit random to divide it that way,
> unless I'm missing something?
>
> (I had a physics teacher who drilled THINK UNITS BEFORE YOU THINK
> NUMBERS" into my head an I still do) :)
Yeah, I did. I did us->ms first, then ms->100ths. i.e.:
milliseconds = usec / 1000
hundreths = ms / 10.
This should be obvious to anyone who uses the metric system for
units of measurement. I guess I should have used "centisecs". :P
> > void
> > timestr(
> > @@ -165,14 +166,12 @@ timestr(
> > size_t size,
> > int format)
> > {
> > - double usec = (double)tv->tv_usec / 1000000.0;
> > -
> > if (format & TERSE_FIXED_TIME) {
> > if (!HOURS(tv->tv_sec)) {
> > snprintf(ts, size, "%u:%02u.%02u",
> > (unsigned int) MINUTES(tv->tv_sec),
> > (unsigned int) SECONDS(tv->tv_sec),
> > - (unsigned int) usec * 100);
> > + (unsigned int) USEC_TO_100THS(tv->tv_usec));
> > return;
> > }
> > format |= VERBOSE_FIXED_TIME; /* fallback if hours needed */
> > @@ -183,9 +182,10 @@ timestr(
> > (unsigned int) HOURS(tv->tv_sec),
> > (unsigned int) MINUTES(tv->tv_sec),
> > (unsigned int) SECONDS(tv->tv_sec),
> > - (unsigned int) usec * 100);
> > + (unsigned int) USEC_TO_100THS(tv->tv_usec));
> > } else {
> > - snprintf(ts, size, "0.%04u sec", (unsigned int) usec * 10000);
> > + snprintf(ts, size, "0.%04u sec",
> > + (unsigned int) tv->tv_usec / 100);
>
> USEC_TO_10000THS() for consistency ?
Too many bloody zeros. I'm just going to convert them all ti
milliseconds and be done with it.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_io: fix operation time reporting
2018-03-02 21:49 ` Dave Chinner
@ 2018-03-02 21:52 ` Eric Sandeen
0 siblings, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2018-03-02 21:52 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On 3/2/18 3:49 PM, Dave Chinner wrote:
> On Thu, Mar 01, 2018 at 10:00:52PM -0600, Eric Sandeen wrote:
>> On 2/28/18 9:56 PM, Dave Chinner wrote:
>>> From: Dave Chinner <dchinner@redhat.com>
>>>
>>> CUrrently the 100th/sec units always report zero, such as:
>>>
>>> 32 MiB, 8192 ops; 0:00:21.00 (1.476 MiB/sec and 377.9260 ops/sec)
>>> ^^
>>>
>>> This is incorrect. Fix the maths that is wrong by removing all the
>>> unnecesary floating point maths and just using basic integer
>>> division...
>>>
>>> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
>>> ---
>>> libxcmd/input.c | 16 ++++++++--------
>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/libxcmd/input.c b/libxcmd/input.c
>>> index 441bb2fbbf34..6e7a8c9822ee 100644
>>> --- a/libxcmd/input.c
>>> +++ b/libxcmd/input.c
>>> @@ -154,9 +154,10 @@ tdiv(double value, struct timeval tv)
>>> return value / ((double)tv.tv_sec + ((double)tv.tv_usec / 1000000.0));
>>> }
>>>
>>> -#define HOURS(sec) ((sec) / (60 * 60))
>>> -#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
>>> -#define SECONDS(sec) ((sec) % 60)
>>> +#define HOURS(sec) ((sec) / (60 * 60))
>>> +#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
>>> +#define SECONDS(sec) ((sec) % 60)
>>> +#define USEC_TO_100THS(usec) ((usec) / 1000 / 10)
>>
>> I guess this works but I expected to convert "microseconds to 100ths"
>> via a conversion like:
>>
>> usec * 1sec/1000000usec * 100 hundredths/1sec
>
> That's what the old code tried to do with floating point math and
> casts, but that didn't work.
>
>> so just for readability I'd have expected:
>>
>> #define USEC_TO_100THS(usec) ((usec) / 1000000 * 100)
>
> Welcome to Integer Math 101: Integer Division. You'll always get
> zero, because N / M = 0 when N < M.
hm, right.
>> or possibly just
>> #define USEC_TO_100THS(usec) ((usec) / 10000)
>>
>> ... I'm confused by your choice of orders of magnitude above even
>> though it works - it seems a bit random to divide it that way,
>> unless I'm missing something?
>>
>> (I had a physics teacher who drilled THINK UNITS BEFORE YOU THINK
>> NUMBERS" into my head an I still do) :)
>
> Yeah, I did. I did us->ms first, then ms->100ths. i.e.:
>
> milliseconds = usec / 1000
> hundreths = ms / 10.
>
> This should be obvious to anyone who uses the metric system for
> units of measurement.
I see what you did there!
ok.
Well, whatever. I can merge it as is if you like, and hope the
next person to read it is smarter than I am. Shouldn't be hard.
-Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] xfs_io: fix operation time reporting
@ 2018-03-27 7:26 Dave Chinner
2018-03-27 18:28 ` Eric Sandeen
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2018-03-27 7:26 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
Currently the 100th/sec units always report zero, such as:
32 MiB, 8192 ops; 0:00:21.00 (1.476 MiB/sec and 377.9260 ops/sec)
^^
This is incorrect.
Fix it by reporting milliseconds to be consistent with other
time recording utilities. This means we can just use simple
integer divsion to implement this and remove all the confusion
around calculating non-standard units of time.
Also fix the non-verbose, sub-second time output printed 4
significant digits but only calculated the time to hundreths of a
second. Again, just use milliseconds for it.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
libxcmd/input.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/libxcmd/input.c b/libxcmd/input.c
index 441bb2fbbf34..4830494eb173 100644
--- a/libxcmd/input.c
+++ b/libxcmd/input.c
@@ -154,9 +154,10 @@ tdiv(double value, struct timeval tv)
return value / ((double)tv.tv_sec + ((double)tv.tv_usec / 1000000.0));
}
-#define HOURS(sec) ((sec) / (60 * 60))
-#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
-#define SECONDS(sec) ((sec) % 60)
+#define HOURS(sec) ((sec) / (60 * 60))
+#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
+#define SECONDS(sec) ((sec) % 60)
+#define MILLISECONDS(usec) ((usec) / 1000)
void
timestr(
@@ -165,27 +166,26 @@ timestr(
size_t size,
int format)
{
- double usec = (double)tv->tv_usec / 1000000.0;
-
if (format & TERSE_FIXED_TIME) {
if (!HOURS(tv->tv_sec)) {
- snprintf(ts, size, "%u:%02u.%02u",
+ snprintf(ts, size, "%u:%02u.%03u",
(unsigned int) MINUTES(tv->tv_sec),
(unsigned int) SECONDS(tv->tv_sec),
- (unsigned int) usec * 100);
+ (unsigned int) MILLISECONDS(tv->tv_usec));
return;
}
format |= VERBOSE_FIXED_TIME; /* fallback if hours needed */
}
if ((format & VERBOSE_FIXED_TIME) || tv->tv_sec) {
- snprintf(ts, size, "%u:%02u:%02u.%02u",
+ snprintf(ts, size, "%u:%02u:%02u.%03u",
(unsigned int) HOURS(tv->tv_sec),
(unsigned int) MINUTES(tv->tv_sec),
(unsigned int) SECONDS(tv->tv_sec),
- (unsigned int) usec * 100);
+ (unsigned int) MILLISECONDS(tv->tv_usec));
} else {
- snprintf(ts, size, "0.%04u sec", (unsigned int) usec * 10000);
+ snprintf(ts, size, "0.%03u sec",
+ (unsigned int) MILLISECONDS(tv->tv_usec));
}
}
--
2.16.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] xfs_io: fix operation time reporting
2018-03-27 7:26 Dave Chinner
@ 2018-03-27 18:28 ` Eric Sandeen
0 siblings, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2018-03-27 18:28 UTC (permalink / raw)
To: Dave Chinner, linux-xfs
On 3/27/18 2:26 AM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Currently the 100th/sec units always report zero, such as:
>
> 32 MiB, 8192 ops; 0:00:21.00 (1.476 MiB/sec and 377.9260 ops/sec)
> ^^
> This is incorrect.
>
> Fix it by reporting milliseconds to be consistent with other
> time recording utilities. This means we can just use simple
> integer divsion to implement this and remove all the confusion
> around calculating non-standard units of time.
>
> Also fix the non-verbose, sub-second time output printed 4
> significant digits but only calculated the time to hundreths of a
> second. Again, just use milliseconds for it.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Thanks for the update.
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
> libxcmd/input.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/libxcmd/input.c b/libxcmd/input.c
> index 441bb2fbbf34..4830494eb173 100644
> --- a/libxcmd/input.c
> +++ b/libxcmd/input.c
> @@ -154,9 +154,10 @@ tdiv(double value, struct timeval tv)
> return value / ((double)tv.tv_sec + ((double)tv.tv_usec / 1000000.0));
> }
>
> -#define HOURS(sec) ((sec) / (60 * 60))
> -#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
> -#define SECONDS(sec) ((sec) % 60)
> +#define HOURS(sec) ((sec) / (60 * 60))
> +#define MINUTES(sec) (((sec) % (60 * 60)) / 60)
> +#define SECONDS(sec) ((sec) % 60)
> +#define MILLISECONDS(usec) ((usec) / 1000)
>
> void
> timestr(
> @@ -165,27 +166,26 @@ timestr(
> size_t size,
> int format)
> {
> - double usec = (double)tv->tv_usec / 1000000.0;
> -
> if (format & TERSE_FIXED_TIME) {
> if (!HOURS(tv->tv_sec)) {
> - snprintf(ts, size, "%u:%02u.%02u",
> + snprintf(ts, size, "%u:%02u.%03u",
> (unsigned int) MINUTES(tv->tv_sec),
> (unsigned int) SECONDS(tv->tv_sec),
> - (unsigned int) usec * 100);
> + (unsigned int) MILLISECONDS(tv->tv_usec));
> return;
> }
> format |= VERBOSE_FIXED_TIME; /* fallback if hours needed */
> }
>
> if ((format & VERBOSE_FIXED_TIME) || tv->tv_sec) {
> - snprintf(ts, size, "%u:%02u:%02u.%02u",
> + snprintf(ts, size, "%u:%02u:%02u.%03u",
> (unsigned int) HOURS(tv->tv_sec),
> (unsigned int) MINUTES(tv->tv_sec),
> (unsigned int) SECONDS(tv->tv_sec),
> - (unsigned int) usec * 100);
> + (unsigned int) MILLISECONDS(tv->tv_usec));
> } else {
> - snprintf(ts, size, "0.%04u sec", (unsigned int) usec * 10000);
> + snprintf(ts, size, "0.%03u sec",
> + (unsigned int) MILLISECONDS(tv->tv_usec));
> }
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-03-27 18:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-01 3:56 [PATCH] xfs_io: fix operation time reporting Dave Chinner
2018-03-02 4:00 ` Eric Sandeen
2018-03-02 21:49 ` Dave Chinner
2018-03-02 21:52 ` Eric Sandeen
-- strict thread matches above, loose matches on Subject: below --
2018-03-27 7:26 Dave Chinner
2018-03-27 18:28 ` Eric Sandeen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox