| Message ID | b3c1e952cfff95e0289772d6af5ec2ebd6cc3141.1516935110.git.baolin.wang@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | kdb: use ktime_get_seconds() instead of ktime_get_ts() | expand |
On 01/25/2018 09:03 PM, Baolin Wang wrote: > The kdb code will print the monotonic time by ktime_get_ts(), but > the ktime_get_ts() will be protected by a sequence lock, that will > introduce one deadlock risk if the lock was already held in the > context from which we entered the debugger. > > Since kdb is only interested in the second field, we can use the > ktime_get_seconds() to get the monotonic time without a lock, > moreover we can remove the 'struct timespec', which is not y2038 > safe. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> Acked-by: Jason Wessel <jason.wessel@windriver.com> Thanks. Added to the kgdb-next branch for the next merge cycle. Jason. > --- > kernel/debug/kdb/kdb_main.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > index 69e70f4..f0fc6f7 100644 > --- a/kernel/debug/kdb/kdb_main.c > +++ b/kernel/debug/kdb/kdb_main.c > @@ -2486,10 +2486,8 @@ static int kdb_kill(int argc, const char **argv) > */ > static void kdb_sysinfo(struct sysinfo *val) > { > - struct timespec uptime; > - ktime_get_ts(&uptime); > memset(val, 0, sizeof(*val)); > - val->uptime = uptime.tv_sec; > + val->uptime = ktime_get_seconds(); > val->loads[0] = avenrun[0]; > val->loads[1] = avenrun[1]; > val->loads[2] = avenrun[2];
On Fri, Jan 26, 2018 at 4:03 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > The kdb code will print the monotonic time by ktime_get_ts(), but > the ktime_get_ts() will be protected by a sequence lock, that will > introduce one deadlock risk if the lock was already held in the > context from which we entered the debugger. > > Since kdb is only interested in the second field, we can use the > ktime_get_seconds() to get the monotonic time without a lock, > moreover we can remove the 'struct timespec', which is not y2038 > safe. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > kernel/debug/kdb/kdb_main.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > index 69e70f4..f0fc6f7 100644 > --- a/kernel/debug/kdb/kdb_main.c > +++ b/kernel/debug/kdb/kdb_main.c > @@ -2486,10 +2486,8 @@ static int kdb_kill(int argc, const char **argv) > */ > static void kdb_sysinfo(struct sysinfo *val) > { > - struct timespec uptime; > - ktime_get_ts(&uptime); > memset(val, 0, sizeof(*val)); > - val->uptime = uptime.tv_sec; > + val->uptime = ktime_get_seconds(); > val->loads[0] = avenrun[0]; > val->loads[1] = avenrun[1]; > val->loads[2] = avenrun[2]; Using ktime_get_seconds() avoids locking problems, but I wonder what would happen if we trigger the 'WARN_ON(timekeeping_suspended)' from kdb. Is that a problem? If it is, we have to use ktime_get_mono_fast_ns() and div_u64() instead. Arnd
On Fri, Jan 26, 2018 at 10:21:58AM +0100, Arnd Bergmann wrote: > On Fri, Jan 26, 2018 at 4:03 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > > The kdb code will print the monotonic time by ktime_get_ts(), but > > the ktime_get_ts() will be protected by a sequence lock, that will > > introduce one deadlock risk if the lock was already held in the > > context from which we entered the debugger. > > > > Since kdb is only interested in the second field, we can use the > > ktime_get_seconds() to get the monotonic time without a lock, > > moreover we can remove the 'struct timespec', which is not y2038 > > safe. > > > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > --- > > kernel/debug/kdb/kdb_main.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c > > index 69e70f4..f0fc6f7 100644 > > --- a/kernel/debug/kdb/kdb_main.c > > +++ b/kernel/debug/kdb/kdb_main.c > > @@ -2486,10 +2486,8 @@ static int kdb_kill(int argc, const char **argv) > > */ > > static void kdb_sysinfo(struct sysinfo *val) > > { > > - struct timespec uptime; > > - ktime_get_ts(&uptime); > > memset(val, 0, sizeof(*val)); > > - val->uptime = uptime.tv_sec; > > + val->uptime = ktime_get_seconds(); > > val->loads[0] = avenrun[0]; > > val->loads[1] = avenrun[1]; > > val->loads[2] = avenrun[2]; > > Using ktime_get_seconds() avoids locking problems, but I wonder what > would happen if we trigger the 'WARN_ON(timekeeping_suspended)' > from kdb. Is that a problem? If it is, we have to use ktime_get_mono_fast_ns() > and div_u64() instead. Normally a WARN_ON() doesn't triggered a kgdb_breakpoint() so (apart from bugs) we can start executing the warning. Unfortunately kdb_trap_printk isn't set when we call ktime_get_seconds() so printing the warning isn't safe. If we had no choice of time function we could work around by enabling printk() trapping for the call but since ktime_get_mono_fast_ns() already exists its probably best just to use that. Daniel.
On 26 January 2018 at 22:00, Daniel Thompson <daniel.thompson@linaro.org> wrote: > On Fri, Jan 26, 2018 at 10:21:58AM +0100, Arnd Bergmann wrote: >> On Fri, Jan 26, 2018 at 4:03 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> > The kdb code will print the monotonic time by ktime_get_ts(), but >> > the ktime_get_ts() will be protected by a sequence lock, that will >> > introduce one deadlock risk if the lock was already held in the >> > context from which we entered the debugger. >> > >> > Since kdb is only interested in the second field, we can use the >> > ktime_get_seconds() to get the monotonic time without a lock, >> > moreover we can remove the 'struct timespec', which is not y2038 >> > safe. >> > >> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> > --- >> > kernel/debug/kdb/kdb_main.c | 4 +--- >> > 1 file changed, 1 insertion(+), 3 deletions(-) >> > >> > diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c >> > index 69e70f4..f0fc6f7 100644 >> > --- a/kernel/debug/kdb/kdb_main.c >> > +++ b/kernel/debug/kdb/kdb_main.c >> > @@ -2486,10 +2486,8 @@ static int kdb_kill(int argc, const char **argv) >> > */ >> > static void kdb_sysinfo(struct sysinfo *val) >> > { >> > - struct timespec uptime; >> > - ktime_get_ts(&uptime); >> > memset(val, 0, sizeof(*val)); >> > - val->uptime = uptime.tv_sec; >> > + val->uptime = ktime_get_seconds(); >> > val->loads[0] = avenrun[0]; >> > val->loads[1] = avenrun[1]; >> > val->loads[2] = avenrun[2]; >> >> Using ktime_get_seconds() avoids locking problems, but I wonder what >> would happen if we trigger the 'WARN_ON(timekeeping_suspended)' >> from kdb. Is that a problem? If it is, we have to use ktime_get_mono_fast_ns() >> and div_u64() instead. > > Normally a WARN_ON() doesn't triggered a kgdb_breakpoint() so (apart > from bugs) we can start executing the warning. Unfortunately > kdb_trap_printk isn't set when we call ktime_get_seconds() so printing > the warning isn't safe. > > If we had no choice of time function we could work around by > enabling printk() trapping for the call but since ktime_get_mono_fast_ns() > already exists its probably best just to use that. > If timekeeping_suspended is set, which means the system had been in suspend state. So now we still need debugger the system? But cores were already powered down. The ktime_get_mono_fast_ns() will access the the clocksource driver, if the timekeeping is suspended following system suspend and the clocksource is not SUSPEND_NONSTOP, we may meet some unexpected issue to access the timer's register without clock. So I am not sure if ktime_get_mono_fast_ns() can work well for this case. -- Baolin.wang Best Regards
On Fri, Jan 26, 2018 at 3:20 PM, Baolin Wang <baolin.wang@linaro.org> wrote: > On 26 January 2018 at 22:00, Daniel Thompson <daniel.thompson@linaro.org> wrote: >> On Fri, Jan 26, 2018 at 10:21:58AM +0100, Arnd Bergmann wrote: >>> On Fri, Jan 26, 2018 at 4:03 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>> >>> Using ktime_get_seconds() avoids locking problems, but I wonder what >>> would happen if we trigger the 'WARN_ON(timekeeping_suspended)' >>> from kdb. Is that a problem? If it is, we have to use ktime_get_mono_fast_ns() >>> and div_u64() instead. >> >> Normally a WARN_ON() doesn't triggered a kgdb_breakpoint() so (apart >> from bugs) we can start executing the warning. Unfortunately >> kdb_trap_printk isn't set when we call ktime_get_seconds() so printing >> the warning isn't safe. >> >> If we had no choice of time function we could work around by >> enabling printk() trapping for the call but since ktime_get_mono_fast_ns() >> already exists its probably best just to use that. >> > > If timekeeping_suspended is set, which means the system had been in > suspend state. So now we still need debugger the system? But cores > were already powered down. I'm not using kdb myself, but I would assume that trapping into the debugger during a suspend/resume bug is a very important scenario. > The ktime_get_mono_fast_ns() will access the the clocksource driver, > if the timekeeping is suspended following system suspend and the > clocksource is not SUSPEND_NONSTOP, we may meet some unexpected issue > to access the timer's register without clock. So I am not sure if > ktime_get_mono_fast_ns() can work well for this case. I misread the code the same way before, but as Thomas Gleixner pointed out, ktime_get_mono_fast_ns() will not call the clocksource driver when timekeeping is suspended. See halt_fast_timekeeper(). Arnd
On 27 January 2018 at 00:01, Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Jan 26, 2018 at 3:20 PM, Baolin Wang <baolin.wang@linaro.org> wrote: >> On 26 January 2018 at 22:00, Daniel Thompson <daniel.thompson@linaro.org> wrote: >>> On Fri, Jan 26, 2018 at 10:21:58AM +0100, Arnd Bergmann wrote: >>>> On Fri, Jan 26, 2018 at 4:03 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>>> >>>> Using ktime_get_seconds() avoids locking problems, but I wonder what >>>> would happen if we trigger the 'WARN_ON(timekeeping_suspended)' >>>> from kdb. Is that a problem? If it is, we have to use ktime_get_mono_fast_ns() >>>> and div_u64() instead. >>> >>> Normally a WARN_ON() doesn't triggered a kgdb_breakpoint() so (apart >>> from bugs) we can start executing the warning. Unfortunately >>> kdb_trap_printk isn't set when we call ktime_get_seconds() so printing >>> the warning isn't safe. >>> >>> If we had no choice of time function we could work around by >>> enabling printk() trapping for the call but since ktime_get_mono_fast_ns() >>> already exists its probably best just to use that. >>> >> >> If timekeeping_suspended is set, which means the system had been in >> suspend state. So now we still need debugger the system? But cores >> were already powered down. > > I'm not using kdb myself, but I would assume that trapping into the debugger > during a suspend/resume bug is a very important scenario. > >> The ktime_get_mono_fast_ns() will access the the clocksource driver, >> if the timekeeping is suspended following system suspend and the >> clocksource is not SUSPEND_NONSTOP, we may meet some unexpected issue >> to access the timer's register without clock. So I am not sure if >> ktime_get_mono_fast_ns() can work well for this case. > > I misread the code the same way before, but as Thomas Gleixner > pointed out, ktime_get_mono_fast_ns() will not call the clocksource > driver when timekeeping is suspended. See halt_fast_timekeeper(). Ah, I missed halt_fast_timekeeper() too, thanks for pointing it out. Now I think ktime_get_mono_fast_ns() can work for this case. Jason, could you drop the previous patch? I will respin v2 to use ktime_get_mono_fast_ns() as Arnd suggested. Thanks. -- Baolin.wang Best Regards
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index 69e70f4..f0fc6f7 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -2486,10 +2486,8 @@ static int kdb_kill(int argc, const char **argv) */ static void kdb_sysinfo(struct sysinfo *val) { - struct timespec uptime; - ktime_get_ts(&uptime); memset(val, 0, sizeof(*val)); - val->uptime = uptime.tv_sec; + val->uptime = ktime_get_seconds(); val->loads[0] = avenrun[0]; val->loads[1] = avenrun[1]; val->loads[2] = avenrun[2];
The kdb code will print the monotonic time by ktime_get_ts(), but the ktime_get_ts() will be protected by a sequence lock, that will introduce one deadlock risk if the lock was already held in the context from which we entered the debugger. Since kdb is only interested in the second field, we can use the ktime_get_seconds() to get the monotonic time without a lock, moreover we can remove the 'struct timespec', which is not y2038 safe. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- kernel/debug/kdb/kdb_main.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) -- 1.7.9.5