| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1265234930.3255.54.camel@work-vm>
Date: Wed, 03 Feb 2010 14:08:50 -0800
From: john stultz <johnstul@...ibm.com>
To: Alexander Gordeev <lasaine@....cs.msu.su>
Cc: linux-kernel@...r.kernel.org, linuxpps@...enneenne.com,
"Nikita V. Youshchenko" <yoush@...msu.su>, stas@....cs.msu.su,
Rodolfo Giometti <giometti@...eenne.com>,
Rodolfo Giometti <giometti@...ux.it>,
Andrew Morton <akpm@...ux-foundation.org>,
Alan Cox <alan@...rguk.ukuu.org.uk>,
Ingo Molnar <mingo@...e.hu>,
Thomas Gleixner <tglx@...utronix.de>,
Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH 1/5] ntp: add hardpps implementation
On Wed, 2010-02-03 at 23:56 +0300, Alexander Gordeev wrote:
> This commit adds hardpps() implementation based upon the original one
> from the NTPv4 reference kernel code. However, it is highly optimized
> towards very fast syncronization and maximum stickness to PPS signal.
> The typical error is less then a microsecond.
> To make it sync faster I had to throw away exponential phase filter so
> that the full phase offset is corrected immediately. Then I also had to
> throw away median phase filter because it gives a bigger error itself
> if used without exponential filter.
> Maybe we will find an appropriate filtering scheme in the future but
> it's not necessary if the signal quality is ok.
>
> Signed-off-by: Alexander Gordeev <lasaine@....cs.msu.su>
Very cool! Bunch of style comments below.
One other thing: From the comments in the code, it looks like this may
have come straight from the reference implementation. You might want to
provide some extra documentation or comment providing proper credit, and
clarifying that the code is in fact GPLv2 compatible.
Also please review Section 12 of Documentation/SubmittingPatches
thanks
-john
[snip]
> +long pps_tf[3]; /* phase median filter */
> +s64 pps_freq; /* scaled frequency offset (ns/s) */
> +s64 pps_fcount; /* frequency accumulator */
> +long pps_jitter; /* nominal jitter (ns) */
> +long pps_stabil; /* nominal stability (scaled ns/s) */
> +int pps_valid; /* signal watchdog counter */
> +int pps_shift; /* nominal interval duration (s) (shift) */
> +int pps_shiftmax; /* max interval duration (s) (shift) */
> +int pps_intcnt; /* interval counter */
> +
> +/*
> + * PPS signal quality monitors
> + */
> +long pps_calcnt; /* calibration intervals */
> +long pps_jitcnt; /* jitter limit exceeded */
> +long pps_stbcnt; /* stability limit exceeded */
> +long pps_errcnt; /* calibration errors */
> +
Shouldn't all of the above values be made static?
[snip]
> /*
> @@ -233,8 +277,6 @@ static enum hrtimer_restart ntp_leap_second(struct hrtimer *timer)
> */
> void second_overflow(void)
> {
> - s64 delta;
> -
> /* Bump the maxerror field */
> time_maxerror += MAXFREQ / NSEC_PER_USEC;
> if (time_maxerror > NTP_PHASE_LIMIT) {
> @@ -248,9 +290,27 @@ void second_overflow(void)
> */
> tick_length = tick_length_base;
>
> - delta = shift_right(time_offset, SHIFT_PLL + time_constant);
> - time_offset -= delta;
> - tick_length += delta;
> +#ifdef CONFIG_NTP_PPS
> + if (time_status & STA_PPSTIME && time_status & STA_PPSSIGNAL) {
> + tick_length += time_offset;
> + time_offset = 0;
> + } else
> +#endif /* CONFIG_NTP_PPS */
> + {
> + s64 delta;
> + delta =
> + shift_right(time_offset, SHIFT_PLL + time_constant);
> + time_offset -= delta;
> + tick_length += delta;
> + }
Ugh. Surely there's a better way to do this then using the ifdefs in
code?
Maybe something like:
#ifdef CONFIG_NTP_PPS
/* Comment about how pps consumes the whole offset */
static inline s64 ntp_offset_chunk(s64 offset)
{
if (time_status & STA_PPSTIME && time_status & STA_PPSSIGNAL)
return offset
else
return shift_right(offset, SHIFT_PLL + time_constant);
}
#else
static inline s64 ntp_offset_chunk(s64 offset)
{
return shift_right(offset, SHIFT_PLL + time_constant);
}
#endif
Then in second_overflow():
delta = ntp_offset_chunk(time_offset);
time_offset -= delta;
tick_length += delta;
Feel free to use a better name then ntp_offset_chunk(), but this keeps
the logic from being obfuscated by all the #ifdefs
> +#ifdef CONFIG_NTP_PPS
> + if (pps_valid > 0)
> + pps_valid--;
> + else
> + time_status &= ~(STA_PPSSIGNAL | STA_PPSJITTER |
> + STA_PPSWANDER | STA_PPSERROR);
> +#endif /* CONFIG_NTP_PPS */
Similarly, can some sort of pps_dec_valid() inline function be used
here?
[snip]
> +#ifdef CONFIG_NTP_PPS
> +
> +/* normalize the timestamp so that nsec is in the
> + ( -NSEC_PER_SEC / 2, NSEC_PER_SEC / 2 ] interval */
> +static inline struct timespec pps_normalize_ts(struct timespec ts)
> +{
> + if (ts.tv_nsec > (NSEC_PER_SEC >> 1)) {
> + ts.tv_nsec -= NSEC_PER_SEC;
> + ts.tv_sec++;
> + }
> +
> + return ts;
> +}
Hrmm.. So by converting a timespec into a second + [-NSEC_PER_SEC/2,
NSEC_PER_SEC/2] value, you aren't really using a timespec anymore,
right? I mean, the timepsec_valid() would fail in many cases, for
instance.
I realize its pretty close, and you *can* use a timespec this way, but
to me it seems reasonable to introduce a new structure (pps_normtime?)
so that its clear you shouldn't exchange timespec values and
pps_normtime values directly?
This is sort of a nit, and maybe others disagree, so not the most
critical issue. But it seems like you're abusing the structure a bit.
[snip]
> +/* decrease frequency calibration interval length */
> +static inline void pps_dec_freq_interval(void)
> +{
> + if (--pps_intcnt <= -4) {
> + pps_intcnt = -4;
Is -4 a magic value?
[snip]
> + } else { /* good sample */
> + if (++pps_intcnt >= 4) {
> + pps_intcnt = 4;
Again, the magic 4?
[snip]
> + pps_stabil += (div_s64(((s64)delta_mod) <<
> + (NTP_SCALE_SHIFT - SHIFT_USEC),
> + NSEC_PER_USEC) - pps_stabil) >> PPS_FAVG;
Hmm. Two 64bit divides in this path? This code would run each second,
right? It would probably be good to see if this could be optimized a
little bit more, but I guess its similar to ntp_update_frequency() which
is called on adjtimex() calls (every 16 seconds at most with networked
ntp).
> +void hardpps(const struct timespec *p_ts, s64 nsec)
> +{
> + struct timespec ts_norm, freq_norm;
> +
> + ts_norm = pps_normalize_ts(*p_ts);
> + freq_norm = pps_normalize_ts(ns_to_timespec(nsec));
> +
[snip]
> +
> + write_seqlock_irq(&xtime_lock);
This is called possibly in irq context, right? So you probably want to
use write_seqlock_irqsave(), no?
thanks
-john
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists