Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720493 > unrolled thread

Re: [PATCH v5 1/2] sched/clock: interface to allow timestamps early in boot

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 1/2] sched/clock: interface to allow timestamps early  in boot Thomas Gleixner <tglx@linutronix.de> - 2017-08-26 01:00 +0200
    Re: [PATCH v5 1/2] sched/clock: interface to allow timestamps early  in boot Pasha Tatashin <pasha.tatashin@oracle.com> - 2017-08-28 16:20 +0200
      Re: [PATCH v5 1/2] sched/clock: interface to allow timestamps early  in boot Thomas Gleixner <tglx@linutronix.de> - 2017-08-28 17:20 +0200
        Re: [PATCH v5 1/2] sched/clock: interface to allow timestamps early  in boot Pasha Tatashin <pasha.tatashin@oracle.com> - 2017-08-28 19:50 +0200
      Re: [PATCH v5 1/2] sched/clock: interface to allow timestamps early  in boot Thomas Gleixner <tglx@linutronix.de> - 2017-08-28 17:20 +0200
        Re: [PATCH v5 1/2] sched/clock: interface to allow timestamps early  in boot Pasha Tatashin <pasha.tatashin@oracle.com> - 2017-08-28 20:10 +0200

#1720493 — Re: [PATCH v5 1/2] sched/clock: interface to allow timestamps early in boot

On Wed, 23 Aug 2017, Pavel Tatashin wrote:
> 
> - Use weak sched_clock_early() interface to determine time from boot in
>   arch specific read_boot_clock64()

weak sched_clock_early() is not an interface. The weak implementation is
merily a place holder which can be overridden by a real implementation.

Aside of that this change is completely unrelated to the sched clock core
changes and wants to be split out into a separate patch.

> +/*
> + * Called once during to boot to initialize boot time.
> + */
> +void read_boot_clock64(struct timespec64 *ts)

And because its called only once, it does not need to be marked __init()
and must be kept around forever, right?

> +{
> +	u64 ns_boot = sched_clock_early(); /* nsec from boot */

Please do not use tail comments. They are a horrible habit.

Instead of adding this crap you'd have better spent time in adding proper
comments explaining the reasoning behind this function,

> +	struct timespec64 ts_now;
> +	bool valid_clock;
> +	u64 ns_now;
> +
> +	/* Time from epoch */
> +	read_persistent_clock64(&ts_now);
> +	ns_now = timespec64_to_ns(&ts_now);
> +	valid_clock = ns_boot && timespec64_valid_strict(&ts_now) &&
> +			(ns_now > ns_boot);
> +
> +	if (!valid_clock)
> +		*ts = (struct timespec64){0, 0};
> +	else
> +		*ts = ns_to_timespec64(ns_now - ns_boot);
> +}

This is really broken. Look at the time keeping init code. It does:

     read_persistent_clock64(&now);
     ...
     read_boot_clock64(&boot);
     ...
     tk_set_xtime(tk, &now);
     ...
     set_normalized_timespec64(&tmp, -boot.tv_sec, -boot.tv_nsec);
     tk_set_wall_to_mono(tk, tmp);

Lets assume that the initial read_persistent_clock64() happens right before
the second. For simplicity lets assume we get 1000 seconds since the epoch.

Now read_boot_clock() reads sched_clock_early() which returns 1 second.

The second read_persistent_clock64() returns 1001 seconds since the epoch
because the RTC advanced by now. So the resulting time stamp is going to be
1000s since the epoch.

In case the RTC still returns 100 since the epoch, the resulting time stamp
is 999s since the epoch.

A full second difference. That's time stamp lottery but nothing which we
want to base any boot time analysis on.

You have to come up with something more useful than that.

Thanks,

	tglx

[toc] | [next] | [standalone]

#1721653

Hi Thomas,

Thank you for your comments. My replies below.

>> +/*
>> + * Called once during to boot to initialize boot time.
>> + */
>> +void read_boot_clock64(struct timespec64 *ts)
> 
> And because its called only once, it does not need to be marked __init()
> and must be kept around forever, right?

This is because every other architecture implements read_boot_clock64() 
without __init: arm, s390. Beside, the original weak stub does not have 
__init macro. So, I can certainly try to add it for x86, but I am not 
sure what is the behavior once __init section is gone, but weak 
implementation stays.

> 
>> +{
>> +	u64 ns_boot = sched_clock_early(); /* nsec from boot */
> 
> Please do not use tail comments. They are a horrible habit.
> 
> Instead of adding this crap you'd have better spent time in adding proper
> comments explaining the reasoning behind this function,

OK, I will add introduction comment, and remove the tail comment.

> This is really broken. Look at the time keeping init code. It does:
> 
>       read_persistent_clock64(&now);
>       ...
>       read_boot_clock64(&boot);
>       ...
>       tk_set_xtime(tk, &now);
>       ...
>       set_normalized_timespec64(&tmp, -boot.tv_sec, -boot.tv_nsec);
>       tk_set_wall_to_mono(tk, tmp);
> 
> Lets assume that the initial read_persistent_clock64() happens right before
> the second. For simplicity lets assume we get 1000 seconds since the epoch.
> 
> Now read_boot_clock() reads sched_clock_early() which returns 1 second.
> 
> The second read_persistent_clock64() returns 1001 seconds since the epoch
> because the RTC advanced by now. So the resulting time stamp is going to be
> 1000s since the epoch.
> 
> In case the RTC still returns 100 since the epoch, the resulting time stamp
> is 999s since the epoch.
> 
> A full second difference. That's time stamp lottery but nothing which we
> want to base any boot time analysis on.
> 
> You have to come up with something more useful than that.
> 

This makes sense. Changing order in timekeeping_init(void) should take 
care of this:

Change to:

void __init timekeeping_init(void)
{
	/*
	 * We must determine boot timestamp before getting current  	
	 * persistent clock value, because implementation of
	 * read_boot_clock64() might also call the persistent
	 * clock, and a leap second may occur.
	 */

	read_boot_clock64(&boot);
	...
	read_persistent_clock64(&now);
	...
}

[toc] | [prev] | [next] | [standalone]

#1721737

On Mon, 28 Aug 2017, Pasha Tatashin wrote:
> > > +/*
> > > + * Called once during to boot to initialize boot time.
> > > + */
> > > +void read_boot_clock64(struct timespec64 *ts)
> > 
> > And because its called only once, it does not need to be marked __init()
> > and must be kept around forever, right?
> 
> This is because every other architecture implements read_boot_clock64()
> without __init: arm, s390. Beside, the original weak stub does not have __init
> macro. So, I can certainly try to add it for x86, but I am not sure what is
> the behavior once __init section is gone, but weak implementation stays.

And what about fixing that everywhere?

Just because something is wrong, it does not mean that it needs to be
proliferated.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1721850

>>> And because its called only once, it does not need to be marked __init()
>>> and must be kept around forever, right?
>>
>> This is because every other architecture implements read_boot_clock64()
>> without __init: arm, s390. Beside, the original weak stub does not have __init
>> macro. So, I can certainly try to add it for x86, but I am not sure what is
>> the behavior once __init section is gone, but weak implementation stays.
> 
> And what about fixing that everywhere?
> 

Sure, I will update it everywhere.

[toc] | [prev] | [next] | [standalone]

#1721738

On Mon, 28 Aug 2017, Pasha Tatashin wrote:
> This makes sense. Changing order in timekeeping_init(void) should take care of
> this:
> 
> Change to:
> 
> void __init timekeeping_init(void)
> {
> 	/*
> 	 * We must determine boot timestamp before getting current  	
> 	 * persistent clock value, because implementation of
> 	 * read_boot_clock64() might also call the persistent
> 	 * clock, and a leap second may occur.
> 	 */
> 
> 	read_boot_clock64(&boot);
> 	...
> 	read_persistent_clock64(&now);

No. That's the same crap just the other way round.

s390 can do that, because the boot timestamp is correlated with the
persistent clock. Your's not so much.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1721863

>> void __init timekeeping_init(void)
>> {
>> 	/*
>> 	 * We must determine boot timestamp before getting current  	
>> 	 * persistent clock value, because implementation of
>> 	 * read_boot_clock64() might also call the persistent
>> 	 * clock, and a leap second may occur.
>> 	 */
>>
>> 	read_boot_clock64(&boot);
>> 	...
>> 	read_persistent_clock64(&now);
> 
> No. That's the same crap just the other way round.
> 
> s390 can do that, because the boot timestamp is correlated with the
> persistent clock. Your's not so much.
> 

OK, how about reading the persistent clock only once, and send it's 
value to use for calculation of boot stamp to read_boot_clock64() via a 
new argument:

read_boot_clock64(&now, &boot);

Does this sound alright or is there a better way?

I would need to update read_boot_clock64() everywhere it is declared to 
add the __init macro, so this extra argument is not going to increase 
number of line changes.

Thank you,
Pasha

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web