[PATCH] hwclock: flush stdout in hwclock -c

[PATCH] hwclock: flush stdout in hwclock -c

[Alexey Galakhov]

This allows using hwclock -c in automated test scripts that parse
its output line-by-line. The standard output is flushed before
each 10 second delay.

Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>
---
 sys-utils/hwclock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index e1e5816..4cdb2ea 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -1532,6 +1532,7 @@ static int compare_clock (const bool utc, const bool local_opt)
 			printf("hw-time      system-time         freq-offset-ppm   tick\n");
 			printf("%10.0f   %10.6f\n", (double) time1_hw, time1_sys);
 		}
+		fflush(stdout);
 		sleep(10);
 	}
 
-- 
2.3.5

Re: [PATCH] hwclock: flush stdout in hwclock -c

[J William Piggott]

Hello Alexey,

 I've been advocating for the removal of the compare function. One of my
 justifications for doing so is that it has been broken from day one and
 therefore nobody can be using for anything.

 So could you tell me how you are using it, and what useful purpose it is
 serving for you?

Thanks
 William

On 04/16/2015 11:26 AM, Alexey Galakhov wrote:
> This allows using hwclock -c in automated test scripts that parse
> its output line-by-line. The standard output is flushed before
> each 10 second delay.
> 
> Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>
> ---
>  sys-utils/hwclock.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
> index e1e5816..4cdb2ea 100644
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -1532,6 +1532,7 @@ static int compare_clock (const bool utc, const bool local_opt)
>  			printf("hw-time      system-time         freq-offset-ppm   tick\n");
>  			printf("%10.0f   %10.6f\n", (double) time1_hw, time1_sys);
>  		}
> +		fflush(stdout);
>  		sleep(10);
>  	}
>  
> 

Re: [PATCH] hwclock: flush stdout in hwclock -c

[Alexey Galakhov]

Hello William,

Am Sun, 19 Apr 2015 20:34:19 -0400
schrieb J William Piggott <elseifthen@gmx.com>:

>  I've been advocating for the removal of the compare function. One of
> my justifications for doing so is that it has been broken from day
> one and therefore nobody can be using for anything.
> 
>  So could you tell me how you are using it, and what useful purpose
> it is serving for you?

We use it in an automated test of the Linux system time on an embedded
system. Recently we fixed a kernel bug introduced by one of the 3rd
party patches that caused a huge (0.01%-0.1%) drift of the
CLOCK_REALTIME due to incorrect PLL initialization. The corresponding
test based on hwclock's compare is now added to our automated test
system. The test runs hwclock -c forever and fails if any drift
anomality is discovered.

Looks like hwclock -c works correctly at least in our case. I think it
could be rewritten using clock_gettime() instead of gettimeofday().

Regards,
Alexey

Re: [PATCH] hwclock: flush stdout in hwclock -c

[J William Piggott]

On 04/20/2015 03:28 AM, Alexey Galakhov wrote:
> Hello William,
> 
> Am Sun, 19 Apr 2015 20:34:19 -0400
> schrieb J William Piggott <elseifthen@gmx.com>:
> 
>>  I've been advocating for the removal of the compare function. One of
>> my justifications for doing so is that it has been broken from day
>> one and therefore nobody can be using for anything.
>>
>>  So could you tell me how you are using it, and what useful purpose
>> it is serving for you?
> 
> We use it in an automated test of the Linux system time on an embedded
> system. Recently we fixed a kernel bug introduced by one of the 3rd
> party patches that caused a huge (0.01%-0.1%) drift of the
> CLOCK_REALTIME due to incorrect PLL initialization. The corresponding
> test based on hwclock's compare is now added to our automated test
> system. The test runs hwclock -c forever and fails if any drift
> anomality is discovered.
>

So you are using the Hardware Clock as a reference to detect anomalies
in the System Clock? You must have an exceptional Hardware Clock. It
sounds as though you are using NTP, why not compare NTP time directly to
your System time?

> Looks like hwclock -c works correctly at least in our case.

I assume then, that you are only using columns one and two, because
columns 3 and 4 are invalid output. If yes, then you are not using
hwclock -c for its actual 'compare' functionality, but only as a source
for timestamps. Your test could capture timestamps without hwclock's
compare function, yes?


> I think it
> could be rewritten using clock_gettime() instead of gettimeofday().
> 
> Regards,
> Alexey
> 

Re: [PATCH] hwclock: flush stdout in hwclock -c

[Alexey Galakhov]

Am Tue, 21 Apr 2015 11:19:06 -0400
schrieb J William Piggott <elseifthen@gmx.com>:

> So you are using the Hardware Clock as a reference to detect anomalies
> in the System Clock? You must have an exceptional Hardware Clock. It
> sounds as though you are using NTP, why not compare NTP time directly
> to your System time?

We just compare all available clocks (actually two) to find anomalies
in both. The anomalies we're looking for are really huge and even
visible with the naked eye (10000 ppm or more), so even a crude
Hardware Clock is more than enough.

The problem of using NTP is, we're running that on a bunch of
experimental devices. Some of them have no working network connection
as the hardware is not quite ready.

> I assume then, that you are only using columns one and two, because
> columns 3 and 4 are invalid output.

IN the beginning I used column 3. By examining the source code I
figured out that it is suitable for such kind of tests, at least
if /etc/adjtime is empty.

> Your test could capture timestamps without hwclock's compare
> function, yes?

Right now I reimplemented the whole (simplified) hwclock -c
functionality in my own test. In fact, I recalculate column 3 on my own.

What I really want to see is a "correct" column 3. In my test I'm doing
direct comparison. Pseudocode:

while(1) {
    do { // busy-wait
        t1 = clock_gettime();
        t2 = ioctl(RTC);
    } while t2 does not change;
    
    drift = (t1 - t2) / total time;

    sleep(until t1 + 990ms);
}

My test fails if a relative drift is more than expected with such a
hardware. This is useful to quickly detect stupid bugs like wrong PLL
configuration on new hardware ("time runs 10% faster" and so on). The
reason why we do so is, nobody really cares about the system clock
while installing the kernel and base system. Thus the problem is
detected only when the complete user level is up and running, and
nobody wants to touch the bootloader and the kernel at this time. So we
created a bunch of "early warning tests" to detect non-obvious kernel
and hardware issues.

Regards,
Alexey

Re: [PATCH] hwclock: flush stdout in hwclock -c

[J William Piggott]

On 04/21/2015 11:50 AM, Alexey Galakhov wrote:
> Am Tue, 21 Apr 2015 11:19:06 -0400
> schrieb J William Piggott <elseifthen@gmx.com>:
> 
>> I assume then, that you are only using columns one and two, because
>> columns 3 and 4 are invalid output.
> 
> IN the beginning I used column 3. By examining the source code I
> figured out that it is suitable for such kind of tests, at least
> if /etc/adjtime is empty.
> 
>> Your test could capture timestamps without hwclock's compare
>> function, yes?
> 
> Right now I reimplemented the whole (simplified) hwclock -c
> functionality in my own test. In fact, I recalculate column 3 on my own.
> 
> What I really want to see is a "correct" column 3. In my test I'm doing
> direct comparison.

So you are confirming my position that hwclock's compare function is
broken and that you are not using it for any useful purpose.


> Pseudocode:
> 
> while(1) {
>     do { // busy-wait
>         t1 = clock_gettime();
>         t2 = ioctl(RTC);
>     } while t2 does not change;
>     
>     drift = (t1 - t2) / total time;
> 
>     sleep(until t1 + 990ms);
> }
> 
> My test fails if a relative drift is more than expected with such a
> hardware. This is useful to quickly detect stupid bugs like wrong PLL
> configuration on new hardware ("time runs 10% faster" and so on). The
> reason why we do so is, nobody really cares about the system clock
> while installing the kernel and base system. Thus the problem is
> detected only when the complete user level is up and running, and
> nobody wants to touch the bootloader and the kernel at this time. So we
> created a bunch of "early warning tests" to detect non-obvious kernel
> and hardware issues.

Thank you for the detailed explanation Alexey. I can see how comparing
the System and Hardware clocks is useful for your situation and worthy
of a specialize test as you have written. In my opinion, comparing two
drifting clocks is not useful to end users, system administrators, or
distribution maintainers and I am hopeful that Karel will agree to its
removal from hwclock.

> 
> Regards,
> Alexey
> --
> To unsubscribe from this list: send the line "unsubscribe util-linux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] hwclock: flush stdout in hwclock -c

[Karel Zak]

On Thu, Apr 16, 2015 at 05:26:27PM +0200, Alexey Galakhov wrote:
> This allows using hwclock -c in automated test scripts that parse
> its output line-by-line. The standard output is flushed before
> each 10 second delay.
> 
> Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>
> ---
>  sys-utils/hwclock.c | 1 +
>  1 file changed, 1 insertion(+)

 Applied, thanks. We will see shat happen in future...

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH] hwclock: flush stdout in hwclock -c

[J William Piggott]

I do not understand what this patch accomplishes. stdout is line
buffered. All of hwclock -c output ends with \n, which flushes
the buffer. Then we flush it again with fflush? What am I missing?

On 04/27/2015 04:27 AM, Karel Zak wrote:
> On Thu, Apr 16, 2015 at 05:26:27PM +0200, Alexey Galakhov wrote:
>> This allows using hwclock -c in automated test scripts that parse
>> its output line-by-line. The standard output is flushed before
>> each 10 second delay.
>>
>> Signed-off-by: Alexey Galakhov <agalakhov@gmail.com>
>> ---
>>  sys-utils/hwclock.c | 1 +
>>  1 file changed, 1 insertion(+)
> 
>  Applied, thanks. We will see shat happen in future...
> 
>     Karel
> 

Re: [PATCH] hwclock: flush stdout in hwclock -c

[Alexey Galakhov]

On Mon, 27 Apr 2015 17:27:07 -0400
J William Piggott <elseifthen@gmx.com> wrote:

> I do not understand what this patch accomplishes. stdout is line
> buffered. All of hwclock -c output ends with \n, which flushes
> the buffer. Then we flush it again with fflush? What am I missing?

stdout is line buffered only if it is not redirected. Using the pipe
symbol in shell to redirect the output i.e. to awk results in not
flushing the buffer until it's full.

A quick test:
$ hwclock -c | cat


Regards,
Alexey

Re: [PATCH] hwclock: flush stdout in hwclock -c

[Bernhard Voelker]

On 04/27/2015 11:42 PM, Alexey Galakhov wrote:
> $ hwclock -c | cat

You can change the behavior of the buffer:  ;-)

  $ stdbuf -oL  hwclock -c | cat

OTOH, I don't see a problem with just one another fflush()
every few seconds.

Have a nice day,
Berny

Re: [PATCH] hwclock: flush stdout in hwclock -c

[J William Piggott]

On 04/27/2015 05:42 PM, Alexey Galakhov wrote:
> On Mon, 27 Apr 2015 17:27:07 -0400
> J William Piggott <elseifthen@gmx.com> wrote:
> 
>> I do not understand what this patch accomplishes. stdout is line
>> buffered. All of hwclock -c output ends with \n, which flushes
>> the buffer. Then we flush it again with fflush? What am I missing?
> 
> stdout is line buffered only if it is not redirected.

Thanks for the explanation, I wasn't aware of that.

> Using the pipe
> symbol in shell to redirect the output i.e. to awk results in not
> flushing the buffer until it's full.
> 
> A quick test:
> $ hwclock -c | cat
> 
> 
> Regards,
> Alexey
>