[PATCH] kdb: use ktime_get_seconds() instead of ktime_get_ts()

[PATCH] kdb: use ktime_get_seconds() instead of ktime_get_ts()

[Baolin Wang]

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];
-- 
1.7.9.5

Re: [PATCH] kdb: use ktime_get_seconds() instead of ktime_get_ts()

[Jason Wessel]

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];

Re: [PATCH] kdb: use ktime_get_seconds() instead of ktime_get_ts()

[Arnd Bergmann]

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

Re: [PATCH] kdb: use ktime_get_seconds() instead of ktime_get_ts()

[Daniel Thompson]

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.

Re: [PATCH] kdb: use ktime_get_seconds() instead of ktime_get_ts()

[Baolin Wang]

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

Re: [PATCH] kdb: use ktime_get_seconds() instead of ktime_get_ts()

[Arnd Bergmann]

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

Re: [PATCH] kdb: use ktime_get_seconds() instead of ktime_get_ts()

[Baolin Wang]

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