Re: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for changes in frequency.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Monday, October 24, 2022 5:43 AM
> To: Keller, Jacob E
> Cc: linuxptp-devel@lists.sourceforge.net
> Subject: Re: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for
> changes in frequency.
>
> On Thu, Oct 20, 2022 at 04:50:37PM +, Keller, Jacob E wrote:
> > > index f0141be..b5a69cc 100644
> > > --- a/clockcheck.c
> > > +++ b/clockcheck.c
> > > @@ -123,6 +123,16 @@ void clockcheck_set_freq(struct clockcheck *cc, int
> freq)
> > > cc->freq_known = 1;
> > > }
> > >
> > > +int clockcheck_freq(struct clockcheck *cc, int freq)
> > > +{
> > > + /* Allow difference of 1 ppb due to conversion to/from double */
> > > + if (cc->freq_known && abs(cc->current_freq - freq) > 1) {
> > > + pr_warning("clockcheck: clock frequency changed
> > > unexpectedly!");
> >
> >
> > The modified documentation would make me think this should allow up to the
> sanity_freq_limit as a difference? Perhaps the documentation should be
> improved somewhat to clarify the difference?
>
> If I expand the description a bit, is it better?
>
> .B sanity_freq_limit
> The maximum allowed frequency offset between uncorrected clock and the
> system
> monotonic clock in parts per billion (ppb). This is used as a sanity check of
> the synchronized clock. When a larger offset is measured, a warning message
> will be printed and the servo will be reset. If the frequency correction set
> by
> ptp4l changes unexpectedly between updates of the clock (e.g. due to another
> process trying to control the clock), a warning message will be printed. When
> set to 0, the sanity check is disabled. The default is 2 (20%).
>
>
> If not, what would you suggest?
>
I like this better!
Thanks,
Jake
> Thanks,
>
> --
> Miroslav Lichvar
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for changes in frequency.
On Thu, Oct 20, 2022 at 04:50:37PM +, Keller, Jacob E wrote:
> > index f0141be..b5a69cc 100644
> > --- a/clockcheck.c
> > +++ b/clockcheck.c
> > @@ -123,6 +123,16 @@ void clockcheck_set_freq(struct clockcheck *cc, int
> > freq)
> > cc->freq_known = 1;
> > }
> >
> > +int clockcheck_freq(struct clockcheck *cc, int freq)
> > +{
> > + /* Allow difference of 1 ppb due to conversion to/from double */
> > + if (cc->freq_known && abs(cc->current_freq - freq) > 1) {
> > + pr_warning("clockcheck: clock frequency changed
> > unexpectedly!");
>
>
> The modified documentation would make me think this should allow up to the
> sanity_freq_limit as a difference? Perhaps the documentation should be
> improved somewhat to clarify the difference?
If I expand the description a bit, is it better?
.B sanity_freq_limit
The maximum allowed frequency offset between uncorrected clock and the system
monotonic clock in parts per billion (ppb). This is used as a sanity check of
the synchronized clock. When a larger offset is measured, a warning message
will be printed and the servo will be reset. If the frequency correction set by
ptp4l changes unexpectedly between updates of the clock (e.g. due to another
process trying to control the clock), a warning message will be printed. When
set to 0, the sanity check is disabled. The default is 2 (20%).
If not, what would you suggest?
Thanks,
--
Miroslav Lichvar
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
Re: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for changes in frequency.
> -Original Message-
> From: Miroslav Lichvar
> Sent: Thursday, October 20, 2022 7:13 AM
> To: linuxptp-devel@lists.sourceforge.net
> Subject: [Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for
> changes
> in frequency.
>
> Before setting the new frequency offset on a clock update, compare the
> current frequency returned by the kernel with the value saved from the
> previous update. Print a warning message if the difference is larger
> than 1 ppb, allowing for rounding errors in conversion to and from
> double. The kernel caches the value set by clock_adjtime() in shifted
> ppm, it doesn't request it from the driver, which can have a lower
> resulution.
>
> This should detect misconfigurations where multiple processes are trying
> to control the clock (e.g. another ptp4l/phc2sys instance or an NTP
> client), even when they don't step the clock.
>
> Signed-off-by: Miroslav Lichvar
> ---
> clock.c | 3 +++
> clockcheck.c | 10 ++
> clockcheck.h | 8
> phc2sys.c| 3 +++
> ptp4l.8 | 4 +++-
> 5 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/clock.c b/clock.c
> index 46ac9c2..8177e77 100644
> --- a/clock.c
> +++ b/clock.c
> @@ -1776,6 +1776,9 @@ static void clock_step_window(struct clock *c)
>
> static void clock_synchronize_locked(struct clock *c, double adj)
> {
> + if (c->sanity_check) {
> + clockcheck_freq(c->sanity_check, clockadj_get_freq(c->clkid));
> + }
> clockadj_set_freq(c->clkid, -adj);
> if (c->clkid == CLOCK_REALTIME) {
> sysclk_set_sync();
> diff --git a/clockcheck.c b/clockcheck.c
> index f0141be..b5a69cc 100644
> --- a/clockcheck.c
> +++ b/clockcheck.c
> @@ -123,6 +123,16 @@ void clockcheck_set_freq(struct clockcheck *cc, int freq)
> cc->freq_known = 1;
> }
>
> +int clockcheck_freq(struct clockcheck *cc, int freq)
> +{
> + /* Allow difference of 1 ppb due to conversion to/from double */
> + if (cc->freq_known && abs(cc->current_freq - freq) > 1) {
> + pr_warning("clockcheck: clock frequency changed
> unexpectedly!");
The modified documentation would make me think this should allow up to the
sanity_freq_limit as a difference? Perhaps the documentation should be improved
somewhat to clarify the difference?
Thanks,
Jake
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
[Linuxptp-devel] [PATCH v2 4/4] Extend clockcheck to check for changes in frequency.
Before setting the new frequency offset on a clock update, compare the
current frequency returned by the kernel with the value saved from the
previous update. Print a warning message if the difference is larger
than 1 ppb, allowing for rounding errors in conversion to and from
double. The kernel caches the value set by clock_adjtime() in shifted
ppm, it doesn't request it from the driver, which can have a lower
resulution.
This should detect misconfigurations where multiple processes are trying
to control the clock (e.g. another ptp4l/phc2sys instance or an NTP
client), even when they don't step the clock.
Signed-off-by: Miroslav Lichvar
---
clock.c | 3 +++
clockcheck.c | 10 ++
clockcheck.h | 8
phc2sys.c| 3 +++
ptp4l.8 | 4 +++-
5 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/clock.c b/clock.c
index 46ac9c2..8177e77 100644
--- a/clock.c
+++ b/clock.c
@@ -1776,6 +1776,9 @@ static void clock_step_window(struct clock *c)
static void clock_synchronize_locked(struct clock *c, double adj)
{
+ if (c->sanity_check) {
+ clockcheck_freq(c->sanity_check, clockadj_get_freq(c->clkid));
+ }
clockadj_set_freq(c->clkid, -adj);
if (c->clkid == CLOCK_REALTIME) {
sysclk_set_sync();
diff --git a/clockcheck.c b/clockcheck.c
index f0141be..b5a69cc 100644
--- a/clockcheck.c
+++ b/clockcheck.c
@@ -123,6 +123,16 @@ void clockcheck_set_freq(struct clockcheck *cc, int freq)
cc->freq_known = 1;
}
+int clockcheck_freq(struct clockcheck *cc, int freq)
+{
+ /* Allow difference of 1 ppb due to conversion to/from double */
+ if (cc->freq_known && abs(cc->current_freq - freq) > 1) {
+ pr_warning("clockcheck: clock frequency changed unexpectedly!");
+ return 1;
+ }
+ return 0;
+}
+
void clockcheck_step(struct clockcheck *cc, int64_t step)
{
if (cc->last_ts)
diff --git a/clockcheck.h b/clockcheck.h
index 1ff86eb..4b09b98 100644
--- a/clockcheck.h
+++ b/clockcheck.h
@@ -54,6 +54,14 @@ int clockcheck_sample(struct clockcheck *cc, uint64_t ts);
*/
void clockcheck_set_freq(struct clockcheck *cc, int freq);
+/**
+ * Check whether the frequency correction did not change unexpectedly.
+ * @param cc Pointer to a clock check obtained via @ref clockcheck_create().
+ * @param freq Current reading of the frequency correction in ppb.
+ * @return Zero if the frequency did not change, non-zero otherwise.
+ */
+int clockcheck_freq(struct clockcheck *cc, int freq);
+
/**
* Inform clock check that the clock was stepped.
* @param cc Pointer to a clock check obtained via @ref clockcheck_create().
diff --git a/phc2sys.c b/phc2sys.c
index ebc43e5..88ed00c 100644
--- a/phc2sys.c
+++ b/phc2sys.c
@@ -561,6 +561,9 @@ static void update_clock(struct phc2sys_private *priv,
struct clock *clock,
/* Fall through. */
case SERVO_LOCKED:
case SERVO_LOCKED_STABLE:
+ if (clock->sanity_check)
+ clockcheck_freq(clock->sanity_check,
+ clockadj_get_freq(clock->clkid));
clockadj_set_freq(clock->clkid, -ppb);
if (clock->clkid == CLOCK_REALTIME)
sysclk_set_sync();
diff --git a/ptp4l.8 b/ptp4l.8
index 1268802..220d594 100644
--- a/ptp4l.8
+++ b/ptp4l.8
@@ -619,7 +619,9 @@ This option used to be called
The maximum allowed frequency offset between uncorrected clock and the system
monotonic clock in parts per billion (ppb). This is used as a sanity check of
the synchronized clock. When a larger offset is measured, a warning message
-will be printed and the servo will be reset. When set to 0, the sanity check is
+will be printed and the servo will be reset. If the frequency correction set by
+ptp4l changes unexpectedly (e.g. due to another process trying to control the
+clock), a warning message will be printed. When set to 0, the sanity check is
disabled. The default is 2 (20%).
.TP
.B initial_delay
--
2.37.3
___
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
