Re: [RFC/PATCH RT] Revert "rt,ntp: Move call to schedule_delayed_work() to helper thread"

[Steven Rostedt]

On Fri, 2 Mar 2018 12:26:03 +0100 (CET)
Thomas Gleixner  wrote:

> On Fri, 2 Mar 2018, Sebastian Andrzej Siewior wrote:
> 
> > I've been looking at this in v3.10-RT where it got in. The patch
> > description says
> > 
> > |The ntp code for notify_cmos_timer() is called from a hard interrupt
> > |context.
> > 
> > I see only one caller of ntp_notify_cmos_timer() and that is
> > do_adjtimex() after "raw_spin_unlock_irqrestore()".
> > I see a few callers of do_adjtimex() which is SYS_adjtimex() (+compat)
> > and posix_clock_realtime_adj() which in turn is called by
> > SYS_clock_adjtime().
> > 
> > I can't find the hard interrupt context. May I revert this patch?  
> 
> That really looks bogus. ntp_notify_cmos_timer() has never been invoked
> from hard interrupt context or from a atomic region. No idea how that patch
> ended up in RT...
>

I'm looking into why I did that. Unless I ended up converting the wrong
function. The patch is from 2013, and I'm sure it was due to some bug
that was triggered with the Red Hat RT kernel. Unless the RH version had
a call somewhere to it.

I'll dig a little further because I'm curious to why I added that
patch, but in the mean time, by all means, revert it.

Thanks!

-- Steve

Re: [RFC/PATCH RT] Revert "rt,ntp: Move call to schedule_delayed_work() to helper thread"

[Steven Rostedt]

On Fri, 2 Mar 2018 12:26:03 +0100 (CET)
Thomas Gleixner  wrote:

> On Fri, 2 Mar 2018, Sebastian Andrzej Siewior wrote:
> 
> > I've been looking at this in v3.10-RT where it got in. The patch
> > description says
> > 
> > |The ntp code for notify_cmos_timer() is called from a hard interrupt
> > |context.
> > 
> > I see only one caller of ntp_notify_cmos_timer() and that is
> > do_adjtimex() after "raw_spin_unlock_irqrestore()".
> > I see a few callers of do_adjtimex() which is SYS_adjtimex() (+compat)
> > and posix_clock_realtime_adj() which in turn is called by
> > SYS_clock_adjtime().
> > 
> > I can't find the hard interrupt context. May I revert this patch?  
> 
> That really looks bogus. ntp_notify_cmos_timer() has never been invoked
> from hard interrupt context or from a atomic region. No idea how that patch
> ended up in RT...
>

I'm looking into why I did that. Unless I ended up converting the wrong
function. The patch is from 2013, and I'm sure it was due to some bug
that was triggered with the Red Hat RT kernel. Unless the RH version had
a call somewhere to it.

I'll dig a little further because I'm curious to why I added that
patch, but in the mean time, by all means, revert it.

Thanks!

-- Steve

Re: [RFC/PATCH RT] Revert "rt,ntp: Move call to schedule_delayed_work() to helper thread"

[Thomas Gleixner]

On Fri, 2 Mar 2018, Sebastian Andrzej Siewior wrote:

> I've been looking at this in v3.10-RT where it got in. The patch
> description says
> 
> |The ntp code for notify_cmos_timer() is called from a hard interrupt
> |context.
> 
> I see only one caller of ntp_notify_cmos_timer() and that is
> do_adjtimex() after "raw_spin_unlock_irqrestore()".
> I see a few callers of do_adjtimex() which is SYS_adjtimex() (+compat)
> and posix_clock_realtime_adj() which in turn is called by
> SYS_clock_adjtime().
> 
> I can't find the hard interrupt context. May I revert this patch?

That really looks bogus. ntp_notify_cmos_timer() has never been invoked
from hard interrupt context or from a atomic region. No idea how that patch
ended up in RT...

Thanks,

tglx

Re: [RFC/PATCH RT] Revert "rt,ntp: Move call to schedule_delayed_work() to helper thread"

[Thomas Gleixner]

On Fri, 2 Mar 2018, Sebastian Andrzej Siewior wrote:

> I've been looking at this in v3.10-RT where it got in. The patch
> description says
> 
> |The ntp code for notify_cmos_timer() is called from a hard interrupt
> |context.
> 
> I see only one caller of ntp_notify_cmos_timer() and that is
> do_adjtimex() after "raw_spin_unlock_irqrestore()".
> I see a few callers of do_adjtimex() which is SYS_adjtimex() (+compat)
> and posix_clock_realtime_adj() which in turn is called by
> SYS_clock_adjtime().
> 
> I can't find the hard interrupt context. May I revert this patch?

That really looks bogus. ntp_notify_cmos_timer() has never been invoked
from hard interrupt context or from a atomic region. No idea how that patch
ended up in RT...

Thanks,

tglx

[RFC/PATCH RT] Revert "rt,ntp: Move call to schedule_delayed_work() to helper thread"

[Sebastian Andrzej Siewior]

I've been looking at this in v3.10-RT where it got in. The patch
description says

|The ntp code for notify_cmos_timer() is called from a hard interrupt
|context.

I see only one caller of ntp_notify_cmos_timer() and that is
do_adjtimex() after "raw_spin_unlock_irqrestore()".
I see a few callers of do_adjtimex() which is SYS_adjtimex() (+compat)
and posix_clock_realtime_adj() which in turn is called by
SYS_clock_adjtime().

I can't find the hard interrupt context. May I revert this patch?

Signed-off-by: Sebastian Andrzej Siewior 
---
 kernel/time/ntp.c | 26 --
 1 file changed, 26 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 2c226b90c231..99e03bec68e4 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -18,7 +18,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include "ntp_internal.h"
 #include "timekeeping_internal.h"
@@ -570,35 +569,10 @@ static void sync_cmos_clock(struct work_struct *work)
   _cmos_work, timespec64_to_jiffies());
 }
 
-#ifdef CONFIG_PREEMPT_RT_FULL
-
-static void run_clock_set_delay(struct swork_event *event)
-{
-   queue_delayed_work(system_power_efficient_wq, _cmos_work, 0);
-}
-
-static struct swork_event ntp_cmos_swork;
-
-void ntp_notify_cmos_timer(void)
-{
-   swork_queue(_cmos_swork);
-}
-
-static __init int create_cmos_delay_thread(void)
-{
-   WARN_ON(swork_get());
-   INIT_SWORK(_cmos_swork, run_clock_set_delay);
-   return 0;
-}
-early_initcall(create_cmos_delay_thread);
-
-#else
-
 void ntp_notify_cmos_timer(void)
 {
queue_delayed_work(system_power_efficient_wq, _cmos_work, 0);
 }
-#endif /* CONFIG_PREEMPT_RT_FULL */
 
 #else
 void ntp_notify_cmos_timer(void) { }
-- 
2.16.2


Sebastian

[RFC/PATCH RT] Revert "rt,ntp: Move call to schedule_delayed_work() to helper thread"

[Sebastian Andrzej Siewior]

I've been looking at this in v3.10-RT where it got in. The patch
description says

|The ntp code for notify_cmos_timer() is called from a hard interrupt
|context.

I see only one caller of ntp_notify_cmos_timer() and that is
do_adjtimex() after "raw_spin_unlock_irqrestore()".
I see a few callers of do_adjtimex() which is SYS_adjtimex() (+compat)
and posix_clock_realtime_adj() which in turn is called by
SYS_clock_adjtime().

I can't find the hard interrupt context. May I revert this patch?

Signed-off-by: Sebastian Andrzej Siewior 
---
 kernel/time/ntp.c | 26 --
 1 file changed, 26 deletions(-)

diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 2c226b90c231..99e03bec68e4 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -18,7 +18,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include "ntp_internal.h"
 #include "timekeeping_internal.h"
@@ -570,35 +569,10 @@ static void sync_cmos_clock(struct work_struct *work)
   _cmos_work, timespec64_to_jiffies());
 }
 
-#ifdef CONFIG_PREEMPT_RT_FULL
-
-static void run_clock_set_delay(struct swork_event *event)
-{
-   queue_delayed_work(system_power_efficient_wq, _cmos_work, 0);
-}
-
-static struct swork_event ntp_cmos_swork;
-
-void ntp_notify_cmos_timer(void)
-{
-   swork_queue(_cmos_swork);
-}
-
-static __init int create_cmos_delay_thread(void)
-{
-   WARN_ON(swork_get());
-   INIT_SWORK(_cmos_swork, run_clock_set_delay);
-   return 0;
-}
-early_initcall(create_cmos_delay_thread);
-
-#else
-
 void ntp_notify_cmos_timer(void)
 {
queue_delayed_work(system_power_efficient_wq, _cmos_work, 0);
 }
-#endif /* CONFIG_PREEMPT_RT_FULL */
 
 #else
 void ntp_notify_cmos_timer(void) { }
-- 
2.16.2


Sebastian