| Message ID | acb789d2e1df1c30d6dfcb9452fef1a398c3b113.1516867148.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | kdb: Change timespec to use timespec64 | expand |
On Thu, Jan 25, 2018 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > @@ -2554,7 +2554,7 @@ static int kdb_summary(int argc, const char **argv) > kdb_printf("domainname %s\n", init_uts_ns.name.domainname); > kdb_printf("ccversion %s\n", __stringify(CCVERSION)); > > - now = __current_kernel_time(); > + now = current_kernel_time64(); > kdb_gmtime(&now, &tm); > kdb_printf("date %04d-%02d-%02d %02d:%02d:%02d " > "tz_minuteswest %d\n", Thanks for picking this one up again, we should find a permanent solution here. Unfortunately you patch is incorrect, as we cannot safely call current_kernel_time64() from NMI context. The __ prefix on __current_kernel_time() indicates that this is a special call that intentionally doesn't read the hardware time to avoid taking locks that might already be held in the context from which we entered the debugger. See https://patchwork.kernel.org/patch/10002097/ for my earlier patch. > @@ -2521,8 +2521,8 @@ static void kdb_gmtime(struct timespec *tv, struct kdb_tm *tm) > */ > static void kdb_sysinfo(struct sysinfo *val) > { > - struct timespec uptime; > - ktime_get_ts(&uptime); > + struct timespec64 uptime; > + ktime_get_ts64(&uptime); > memset(val, 0, sizeof(*val)); > val->uptime = uptime.tv_sec; > val->loads[0] = avenrun[0]; This function appears to have the same problem, except that it is a preexisting issue in this case. I had not noticed this earlier, but we must fix it in a similar manner to the other one. Arnd
On 25 January 2018 at 16:55, Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Jan 25, 2018 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> @@ -2554,7 +2554,7 @@ static int kdb_summary(int argc, const char **argv) >> kdb_printf("domainname %s\n", init_uts_ns.name.domainname); >> kdb_printf("ccversion %s\n", __stringify(CCVERSION)); >> >> - now = __current_kernel_time(); >> + now = current_kernel_time64(); >> kdb_gmtime(&now, &tm); >> kdb_printf("date %04d-%02d-%02d %02d:%02d:%02d " >> "tz_minuteswest %d\n", > > Thanks for picking this one up again, we should find a permanent solution here. > Unfortunately you patch is incorrect, as we cannot safely call > current_kernel_time64() > from NMI context. Ah, thanks for pointing out the issue, since I do not know what context the function will be called in kdb. > > The __ prefix on __current_kernel_time() indicates that this is a special call > that intentionally doesn't read the hardware time to avoid taking locks that > might already be held in the context from which we entered the debugger. > > See https://patchwork.kernel.org/patch/10002097/ for my earlier patch. This patch had not been merged into mainline? > >> @@ -2521,8 +2521,8 @@ static void kdb_gmtime(struct timespec *tv, struct kdb_tm *tm) >> */ >> static void kdb_sysinfo(struct sysinfo *val) >> { >> - struct timespec uptime; >> - ktime_get_ts(&uptime); >> + struct timespec64 uptime; >> + ktime_get_ts64(&uptime); >> memset(val, 0, sizeof(*val)); >> val->uptime = uptime.tv_sec; >> val->loads[0] = avenrun[0]; > > This function appears to have the same problem, except that it is a preexisting > issue in this case. I had not noticed this earlier, but we must fix it > in a similar > manner to the other one. OK. -- Baolin.wang Best Regards
On Thu, Jan 25, 2018 at 05:18:54PM +0800, Baolin Wang wrote: > On 25 January 2018 at 16:55, Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Jan 25, 2018 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > >> @@ -2554,7 +2554,7 @@ static int kdb_summary(int argc, const char **argv) > >> kdb_printf("domainname %s\n", init_uts_ns.name.domainname); > >> kdb_printf("ccversion %s\n", __stringify(CCVERSION)); > >> > >> - now = __current_kernel_time(); > >> + now = current_kernel_time64(); > >> kdb_gmtime(&now, &tm); > >> kdb_printf("date %04d-%02d-%02d %02d:%02d:%02d " > >> "tz_minuteswest %d\n", > > > > Thanks for picking this one up again, we should find a permanent solution here. > > Unfortunately you patch is incorrect, as we cannot safely call > > current_kernel_time64() > > from NMI context. > > Ah, thanks for pointing out the issue, since I do not know what > context the function will be called in kdb. > > > > > The __ prefix on __current_kernel_time() indicates that this is a special call > > that intentionally doesn't read the hardware time to avoid taking locks that > > might already be held in the context from which we entered the debugger. > > > > See https://patchwork.kernel.org/patch/10002097/ for my earlier patch. > > This patch had not been merged into mainline? Not yet (and I'm afraid it's not in kgdb-next either) but the ack from Jason is from this kernel cycle so we'll see what can be done! Daniel.
On 01/25/2018 05:38 AM, Daniel Thompson wrote: > On Thu, Jan 25, 2018 at 05:18:54PM +0800, Baolin Wang wrote: >> On 25 January 2018 at 16:55, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Thu, Jan 25, 2018 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>>> @@ -2554,7 +2554,7 @@ static int kdb_summary(int argc, const char **argv) >>>> kdb_printf("domainname %s\n", init_uts_ns.name.domainname); >>>> kdb_printf("ccversion %s\n", __stringify(CCVERSION)); >>>> >>>> - now = __current_kernel_time(); >>>> + now = current_kernel_time64(); >>>> kdb_gmtime(&now, &tm); >>>> kdb_printf("date %04d-%02d-%02d %02d:%02d:%02d " >>>> "tz_minuteswest %d\n", >>> >>> Thanks for picking this one up again, we should find a permanent solution here. >>> Unfortunately you patch is incorrect, as we cannot safely call >>> current_kernel_time64() >>> from NMI context. >> >> Ah, thanks for pointing out the issue, since I do not know what >> context the function will be called in kdb. >> >>> >>> The __ prefix on __current_kernel_time() indicates that this is a special call >>> that intentionally doesn't read the hardware time to avoid taking locks that >>> might already be held in the context from which we entered the debugger. >>> >>> See https://patchwork.kernel.org/patch/10002097/ for my earlier patch. >> >> This patch had not been merged into mainline? > > Not yet (and I'm afraid it's not in kgdb-next either) but the ack from Jason is from > this kernel cycle so we'll see what can be done! > > I thought for what ever reason this was going through the time keeper subtree. I added it immediately to kgdb-next so it will be evaluated in the linux-next tree in the next day or so, and we can get this merged in the merge window. Thanks, Jason.
On Thu, Jan 25, 2018 at 3:49 PM, Jason Wessel <jason.wessel@windriver.com> wrote: > On 01/25/2018 05:38 AM, Daniel Thompson wrote: >> >> On Thu, Jan 25, 2018 at 05:18:54PM +0800, Baolin Wang wrote: >>> >>> On 25 January 2018 at 16:55, Arnd Bergmann <arnd@arndb.de> wrote: >>>> >>>> On Thu, Jan 25, 2018 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> >>>> wrote: >>>>> >>>>> @@ -2554,7 +2554,7 @@ static int kdb_summary(int argc, const char >>>>> **argv) >>>>> kdb_printf("domainname %s\n", init_uts_ns.name.domainname); >>>>> kdb_printf("ccversion %s\n", __stringify(CCVERSION)); >>>>> >>>>> - now = __current_kernel_time(); >>>>> + now = current_kernel_time64(); >>>>> kdb_gmtime(&now, &tm); >>>>> kdb_printf("date %04d-%02d-%02d %02d:%02d:%02d " >>>>> "tz_minuteswest %d\n", >>>> >>>> >>>> Thanks for picking this one up again, we should find a permanent >>>> solution here. >>>> Unfortunately you patch is incorrect, as we cannot safely call >>>> current_kernel_time64() >>>> from NMI context. >>> >>> >>> Ah, thanks for pointing out the issue, since I do not know what >>> context the function will be called in kdb. >>> >>>> >>>> The __ prefix on __current_kernel_time() indicates that this is a >>>> special call >>>> that intentionally doesn't read the hardware time to avoid taking locks >>>> that >>>> might already be held in the context from which we entered the debugger. >>>> >>>> See https://patchwork.kernel.org/patch/10002097/ for my earlier patch. >>> >>> >>> This patch had not been merged into mainline? >> >> >> Not yet (and I'm afraid it's not in kgdb-next either) but the ack from >> Jason is from >> this kernel cycle so we'll see what can be done! >> >> > > I thought for what ever reason this was going through the time keeper > subtree. I added it immediately to kgdb-next so it will be evaluated in > the linux-next tree in the next day or so, and we can get this merged in the > merge window. Ok, thanks a lot! We should still come up with a patch for kdb_sysinfo(), which doesn't have a problem with time overflow (monotonic time doesn't overflow) but has an issue with locking and uses 'struct timespec'. Baolin, could you respin your patch on top of Jason's tree and replace ktime_get_ts64() with something based on ktime_get_fast_ns? Arnd
On 25 January 2018 at 23:12, Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Jan 25, 2018 at 3:49 PM, Jason Wessel > <jason.wessel@windriver.com> wrote: >> On 01/25/2018 05:38 AM, Daniel Thompson wrote: >>> >>> On Thu, Jan 25, 2018 at 05:18:54PM +0800, Baolin Wang wrote: >>>> >>>> On 25 January 2018 at 16:55, Arnd Bergmann <arnd@arndb.de> wrote: >>>>> >>>>> On Thu, Jan 25, 2018 at 9:05 AM, Baolin Wang <baolin.wang@linaro.org> >>>>> wrote: >>>>>> >>>>>> @@ -2554,7 +2554,7 @@ static int kdb_summary(int argc, const char >>>>>> **argv) >>>>>> kdb_printf("domainname %s\n", init_uts_ns.name.domainname); >>>>>> kdb_printf("ccversion %s\n", __stringify(CCVERSION)); >>>>>> >>>>>> - now = __current_kernel_time(); >>>>>> + now = current_kernel_time64(); >>>>>> kdb_gmtime(&now, &tm); >>>>>> kdb_printf("date %04d-%02d-%02d %02d:%02d:%02d " >>>>>> "tz_minuteswest %d\n", >>>>> >>>>> >>>>> Thanks for picking this one up again, we should find a permanent >>>>> solution here. >>>>> Unfortunately you patch is incorrect, as we cannot safely call >>>>> current_kernel_time64() >>>>> from NMI context. >>>> >>>> >>>> Ah, thanks for pointing out the issue, since I do not know what >>>> context the function will be called in kdb. >>>> >>>>> >>>>> The __ prefix on __current_kernel_time() indicates that this is a >>>>> special call >>>>> that intentionally doesn't read the hardware time to avoid taking locks >>>>> that >>>>> might already be held in the context from which we entered the debugger. >>>>> >>>>> See https://patchwork.kernel.org/patch/10002097/ for my earlier patch. >>>> >>>> >>>> This patch had not been merged into mainline? >>> >>> >>> Not yet (and I'm afraid it's not in kgdb-next either) but the ack from >>> Jason is from >>> this kernel cycle so we'll see what can be done! >>> >>> >> >> I thought for what ever reason this was going through the time keeper >> subtree. I added it immediately to kgdb-next so it will be evaluated in >> the linux-next tree in the next day or so, and we can get this merged in the >> merge window. > > Ok, thanks a lot! > > We should still come up with a patch for kdb_sysinfo(), which doesn't > have a problem with time overflow (monotonic time doesn't overflow) > but has an issue with locking and uses 'struct timespec'. > > Baolin, could you respin your patch on top of Jason's tree and > replace ktime_get_ts64() with something based on ktime_get_fast_ns? Sure, I will do that today. -- Baolin.wang Best Regards
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c index c8146d5..c20c61e 100644 --- a/kernel/debug/kdb/kdb_main.c +++ b/kernel/debug/kdb/kdb_main.c @@ -2488,7 +2488,7 @@ struct kdb_tm { int tm_year; /* year */ }; -static void kdb_gmtime(struct timespec *tv, struct kdb_tm *tm) +static void kdb_gmtime(struct timespec64 *tv, struct kdb_tm *tm) { /* This will work from 1970-2099, 2100 is not a leap year */ static int mon_day[] = { 31, 29, 31, 30, 31, 30, 31, @@ -2521,8 +2521,8 @@ static void kdb_gmtime(struct timespec *tv, struct kdb_tm *tm) */ static void kdb_sysinfo(struct sysinfo *val) { - struct timespec uptime; - ktime_get_ts(&uptime); + struct timespec64 uptime; + ktime_get_ts64(&uptime); memset(val, 0, sizeof(*val)); val->uptime = uptime.tv_sec; val->loads[0] = avenrun[0]; @@ -2539,7 +2539,7 @@ static void kdb_sysinfo(struct sysinfo *val) */ static int kdb_summary(int argc, const char **argv) { - struct timespec now; + struct timespec64 now; struct kdb_tm tm; struct sysinfo val; @@ -2554,7 +2554,7 @@ static int kdb_summary(int argc, const char **argv) kdb_printf("domainname %s\n", init_uts_ns.name.domainname); kdb_printf("ccversion %s\n", __stringify(CCVERSION)); - now = __current_kernel_time(); + now = current_kernel_time64(); kdb_gmtime(&now, &tm); kdb_printf("date %04d-%02d-%02d %02d:%02d:%02d " "tz_minuteswest %d\n",
Since struct timespec is not y2038 safe on 32 bit machines, thus replace uses of struct timespec with struct timespec64 in the kernel. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- kernel/debug/kdb/kdb_main.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) -- 1.7.9.5