[PATCH 00/11] Convert the posix_clock_operations and k_clock structure to ready for 2038
This patch series changes the 32-bit time type (timespec/itimerspec) to the 64-bit one (timespec64/itimerspec64), since 32-bit time types will break in the year 2038.
This patch series introduces new methods with timespec64/itimerspec64 type, and removes the old ones with timespec/itimerspec type for posix_clock_operations and k_clock structure.
Also introduces some new functions with timespec64/itimerspec64 type, like current_kernel_time64(), hrtimer_get_res64(), cputime_to_timespec64() and timespec64_to_cputime().
Baolin Wang (11): linux/time64.h:Introduce the 'struct itimerspec64' for 64bit timekeeping:Introduce the current_kernel_time64() function with timespec64 type time/hrtimer:Introduce hrtimer_get_res64() with timespec64 type for getting the timer resolution posix timers:Introduce the 64bit methods with timespec64 type for k_clock structure time/posix-timers:Convert to the 64bit methods for k_clock callback functions char/mmtimer:Convert to the 64bit methods for k_clock callback function time/alarmtimer:Convert to the new methods for k_clock structure time/posix-clock:Convert to the 64bit methods for k_clock and posix_clock_operations structure cputime:Introduce the cputime_to_timespec64/timespec64_to_cputime function time/posix-cpu-timers:Convert to the 64bit methods for k_clock structure k_clock:Remove the 32bit methods with timespec type
arch/powerpc/include/asm/cputime.h | 6 +- arch/s390/include/asm/cputime.h | 8 +- drivers/char/mmtimer.c | 36 ++++---- drivers/ptp/ptp_clock.c | 26 ++---- include/asm-generic/cputime_jiffies.h | 10 +-- include/linux/cputime.h | 15 ++++ include/linux/hrtimer.h | 12 ++- include/linux/jiffies.h | 3 + include/linux/posix-clock.h | 10 +-- include/linux/posix-timers.h | 18 ++-- include/linux/time64.h | 13 +++ include/linux/timekeeping.h | 14 ++- kernel/time/alarmtimer.c | 43 ++++----- kernel/time/hrtimer.c | 10 +-- kernel/time/posix-clock.c | 20 ++--- kernel/time/posix-cpu-timers.c | 83 +++++++++-------- kernel/time/posix-timers.c | 157 +++++++++++++++++++-------------- kernel/time/time.c | 21 +++++ kernel/time/timekeeping.c | 6 +- kernel/time/timekeeping.h | 2 +- 20 files changed, 302 insertions(+), 211 deletions(-)
This patch introduces the 'struct itimerspec64' for 64bit to replace itimerspec, and also introduces the conversion methods: itimerspec64_to_itimerspec() and itimerspec_to_itimerspec64(), that makes itimerspec to ready for 2038 year.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- include/linux/time64.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/linux/time64.h b/include/linux/time64.h index a383147..3647bdd 100644 --- a/include/linux/time64.h +++ b/include/linux/time64.h @@ -18,6 +18,11 @@ struct timespec64 { }; #endif
+struct itimerspec64 { + struct timespec64 it_interval; /* timer period */ + struct timespec64 it_value; /* timer expiration */ +}; + /* Parameters used to convert the timespec values: */ #define MSEC_PER_SEC 1000L #define USEC_PER_MSEC 1000L @@ -187,4 +192,12 @@ static __always_inline void timespec64_add_ns(struct timespec64 *a, u64 ns)
#endif
+#define itimerspec64_to_itimerspec(its64) \ + ({ (struct itimerspec){ .it_interval = timespec64_to_timespec((its64).it_interval), \ + .it_value = timespec64_to_timespec((its64).it_value) }; }) + +#define itimerspec_to_itimerspec64(its) \ + ({ (struct itimerspec64){ .it_interval = timespec_to_timespec64((its).it_interval), \ + .it_value = timespec_to_timespec64((its).it_value) }; }) + #endif /* _LINUX_TIME64_H */
Hello.
On 4/20/2015 8:57 AM, Baolin Wang wrote:
This patch introduces the 'struct itimerspec64' for 64bit to replace itimerspec, and also introduces the conversion methods: itimerspec64_to_itimerspec() and itimerspec_to_itimerspec64(), that makes itimerspec to ready for 2038 year.
"To" not needed here.
Signed-off-by: Baolin Wang baolin.wang@linaro.org
[...]
WBR, Sergei
On 20 April 2015 at 17:49, Sergei Shtylyov < sergei.shtylyov@cogentembedded.com> wrote:
Hello.
On 4/20/2015 8:57 AM, Baolin Wang wrote:
This patch introduces the 'struct itimerspec64' for 64bit to replace
itimerspec, and also introduces the conversion methods: itimerspec64_to_itimerspec() and itimerspec_to_itimerspec64(), that makes itimerspec to ready for 2038 year.
"To" not needed here.
Signed-off-by: Baolin Wang baolin.wang@linaro.org
[...]
WBR, Sergei
Hi Sergei,
Sorry for the mistake. Thank you for your comments. I'll fix that in next patch.
On Mon, 20 Apr 2015, Baolin Wang wrote:
This patch introduces the 'struct itimerspec64' for 64bit to replace itimerspec, and also introduces the conversion methods: itimerspec64_to_itimerspec() and itimerspec_to_itimerspec64(), that makes itimerspec to ready for 2038 year.
Signed-off-by: Baolin Wang baolin.wang@linaro.org
include/linux/time64.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/linux/time64.h b/include/linux/time64.h index a383147..3647bdd 100644 --- a/include/linux/time64.h +++ b/include/linux/time64.h @@ -18,6 +18,11 @@ struct timespec64 { }; #endif +struct itimerspec64 {
- struct timespec64 it_interval; /* timer period */
- struct timespec64 it_value; /* timer expiration */
+};
/* Parameters used to convert the timespec values: */ #define MSEC_PER_SEC 1000L #define USEC_PER_MSEC 1000L @@ -187,4 +192,12 @@ static __always_inline void timespec64_add_ns(struct timespec64 *a, u64 ns) #endif +#define itimerspec64_to_itimerspec(its64) \
+#define itimerspec_to_itimerspec64(its) \
1.) Make these static inlines please. These macros are not typesafe.
2.) Use pointers to the input value.
Thanks.
tglx
On Mon, 20 Apr 2015, Thomas Gleixner wrote:
On Mon, 20 Apr 2015, Baolin Wang wrote:
This patch introduces the 'struct itimerspec64' for 64bit to replace itimerspec, and also introduces the conversion methods: itimerspec64_to_itimerspec() and itimerspec_to_itimerspec64(), that makes itimerspec to ready for 2038 year.
Signed-off-by: Baolin Wang baolin.wang@linaro.org
include/linux/time64.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/linux/time64.h b/include/linux/time64.h index a383147..3647bdd 100644 --- a/include/linux/time64.h +++ b/include/linux/time64.h @@ -18,6 +18,11 @@ struct timespec64 { }; #endif +struct itimerspec64 {
- struct timespec64 it_interval; /* timer period */
- struct timespec64 it_value; /* timer expiration */
+};
Aside of the macro mess. This really wants to be conditional on 64/32 but in the same way as we have the different implementations for timespec64.
Your patch enforces a useless conversion from and to itimerspec64 even on 64 bit because the compiler cannot map the types.
Sigh. timespec64 is a proper guidance here.
Thanks,
tglx
On 21 April 2015 at 03:14, Thomas Gleixner tglx@linutronix.de wrote:
On Mon, 20 Apr 2015, Baolin Wang wrote:
This patch introduces the 'struct itimerspec64' for 64bit to replace
itimerspec,
and also introduces the conversion methods: itimerspec64_to_itimerspec()
and
itimerspec_to_itimerspec64(), that makes itimerspec to ready for 2038
year.
Signed-off-by: Baolin Wang baolin.wang@linaro.org
include/linux/time64.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/include/linux/time64.h b/include/linux/time64.h index a383147..3647bdd 100644 --- a/include/linux/time64.h +++ b/include/linux/time64.h @@ -18,6 +18,11 @@ struct timespec64 { }; #endif
+struct itimerspec64 {
struct timespec64 it_interval; /* timer period */struct timespec64 it_value; /* timer expiration */+};
/* Parameters used to convert the timespec values: */ #define MSEC_PER_SEC 1000L #define USEC_PER_MSEC 1000L @@ -187,4 +192,12 @@ static __always_inline void
timespec64_add_ns(struct timespec64 *a, u64 ns)
#endif
+#define itimerspec64_to_itimerspec(its64) \
+#define itimerspec_to_itimerspec64(its) \
1.) Make these static inlines please. These macros are not typesafe.
2.) Use pointers to the input value.
Thanks.
tglx
Thanks for your comments, i'll fix in next patch.
This patch adds current_kernel_time64() function with timespec64 type, and makes current_kernel_time() 'static inline' and moves it to timekeeping.h file.
It is convenient for user to get the current kernel time with timespec64 type, and delete the current_kernel_time() function easily in timekeeping.h file. That is ready for 2038 when get the current time.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- include/linux/timekeeping.h | 10 +++++++++- kernel/time/timekeeping.c | 6 +++--- 2 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h index 3eaae47..c6d5ae9 100644 --- a/include/linux/timekeeping.h +++ b/include/linux/timekeeping.h @@ -18,10 +18,18 @@ extern int do_sys_settimeofday(const struct timespec *tv, * Kernel time accessors */ unsigned long get_seconds(void); -struct timespec current_kernel_time(void); +struct timespec64 current_kernel_time64(void); /* does not take xtime_lock */ struct timespec __current_kernel_time(void);
+static inline struct timespec current_kernel_time(void) +{ + struct timespec64 now; + + now = current_kernel_time64(); + return timespec64_to_timespec(now); +} + /* * timespec based interfaces */ diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c index 91db941..8ccc02c 100644 --- a/kernel/time/timekeeping.c +++ b/kernel/time/timekeeping.c @@ -1721,7 +1721,7 @@ struct timespec __current_kernel_time(void) return timespec64_to_timespec(tk_xtime(tk)); }
-struct timespec current_kernel_time(void) +struct timespec64 current_kernel_time64(void) { struct timekeeper *tk = &tk_core.timekeeper; struct timespec64 now; @@ -1733,9 +1733,9 @@ struct timespec current_kernel_time(void) now = tk_xtime(tk); } while (read_seqcount_retry(&tk_core.seq, seq));
- return timespec64_to_timespec(now); + return now; } -EXPORT_SYMBOL(current_kernel_time); +EXPORT_SYMBOL(current_kernel_time64);
struct timespec64 get_monotonic_coarse64(void) {
This patch introduces hrtimer_get_res64() function to get the timer resolution with timespec64 type, and moves the hrtimer_get_res() function into include/linux/hrtimer.h as a 'static inline' helper that just calls hrtimer_get_res64.
It is ready for 2038 year when getting the timer resolution by hrtimer_get_res64() function with timespec64 type, and it is convenient to delete the old hrtimer_get_res() function in hrtimer.h file.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- include/linux/hrtimer.h | 12 +++++++++++- kernel/time/hrtimer.c | 10 +++++----- 2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h index 05f6df1..ee8ed44 100644 --- a/include/linux/hrtimer.h +++ b/include/linux/hrtimer.h @@ -383,7 +383,17 @@ static inline int hrtimer_restart(struct hrtimer *timer)
/* Query timers: */ extern ktime_t hrtimer_get_remaining(const struct hrtimer *timer); -extern int hrtimer_get_res(const clockid_t which_clock, struct timespec *tp); +extern int hrtimer_get_res64(const clockid_t which_clock, + struct timespec64 *tp); + +static inline int hrtimer_get_res(const clockid_t which_clock, + struct timespec *tp) +{ + struct timespec64 *ts64; + + *ts64 = timespec_to_timespec64(*tp); + return hrtimer_get_res64(which_clock, ts64); +}
extern ktime_t hrtimer_get_next_event(void);
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c index bee0c1f..508d936 100644 --- a/kernel/time/hrtimer.c +++ b/kernel/time/hrtimer.c @@ -1175,24 +1175,24 @@ void hrtimer_init(struct hrtimer *timer, clockid_t clock_id, EXPORT_SYMBOL_GPL(hrtimer_init);
/** - * hrtimer_get_res - get the timer resolution for a clock + * hrtimer_get_res64 - get the timer resolution for a clock * @which_clock: which clock to query - * @tp: pointer to timespec variable to store the resolution + * @tp: pointer to timespec64 variable to store the resolution * * Store the resolution of the clock selected by @which_clock in the * variable pointed to by @tp. */ -int hrtimer_get_res(const clockid_t which_clock, struct timespec *tp) +int hrtimer_get_res64(const clockid_t which_clock, struct timespec64 *tp) { struct hrtimer_cpu_base *cpu_base; int base = hrtimer_clockid_to_base(which_clock);
cpu_base = raw_cpu_ptr(&hrtimer_bases); - *tp = ktime_to_timespec(cpu_base->clock_base[base].resolution); + *tp = ktime_to_timespec64(cpu_base->clock_base[base].resolution);
return 0; } -EXPORT_SYMBOL_GPL(hrtimer_get_res); +EXPORT_SYMBOL_GPL(hrtimer_get_res64);
static void __run_hrtimer(struct hrtimer *timer, ktime_t *now) {
On Mon, 20 Apr 2015, Baolin Wang wrote:
This patch introduces hrtimer_get_res64() function to get the timer resolution with timespec64 type, and moves the hrtimer_get_res() function into
FYI, That function is about to go away, but it's not a big deal to sort that out once I applied the hrtimer rework to the tip tree.
Thanks,
tglx
This patch introduces the new methods with timespec64 type for k_clcok structure, converts the timepsec type to timespec64 type in k_clock structure and converts the itimerspec type to itimerspec64 type to ready for 2038 issue.
And also introduces the 64bit methods with timespec64 type for the framework functions.
Next step will migrate all the k_clock users to use the new methods with timespec64 type nd itimerspec64 type, and it contains the files of posix-timers.c, mmtimer.c, alarmtimer.c, posix-clock.c and posix-cpu-timers.c.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- include/linux/posix-timers.h | 9 ++++++ kernel/time/posix-timers.c | 65 ++++++++++++++++++++++++++++++++---------- 2 files changed, 59 insertions(+), 15 deletions(-)
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h index 907f3fd..35786c5 100644 --- a/include/linux/posix-timers.h +++ b/include/linux/posix-timers.h @@ -98,9 +98,13 @@ struct k_itimer {
struct k_clock { int (*clock_getres) (const clockid_t which_clock, struct timespec *tp); + int (*clock_getres64) (const clockid_t which_clock, struct timespec64 *tp); int (*clock_set) (const clockid_t which_clock, const struct timespec *tp); + int (*clock_set64) (const clockid_t which_clock, + const struct timespec64 *tp); int (*clock_get) (const clockid_t which_clock, struct timespec * tp); + int (*clock_get64) (const clockid_t which_clock, struct timespec64 *tp); int (*clock_adj) (const clockid_t which_clock, struct timex *tx); int (*timer_create) (struct k_itimer *timer); int (*nsleep) (const clockid_t which_clock, int flags, @@ -109,10 +113,15 @@ struct k_clock { int (*timer_set) (struct k_itimer * timr, int flags, struct itimerspec * new_setting, struct itimerspec * old_setting); + int (*timer_set64) (struct k_itimer *timr, int flags, + struct itimerspec64 *new_setting, + struct itimerspec64 *old_setting); int (*timer_del) (struct k_itimer * timr); #define TIMER_RETRY 1 void (*timer_get) (struct k_itimer * timr, struct itimerspec * cur_setting); + void (*timer_get64) (struct k_itimer *timr, + struct itimerspec64 *cur_setting); };
extern struct k_clock clock_posix_cpu; diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c index 31ea01f..9070387 100644 --- a/kernel/time/posix-timers.c +++ b/kernel/time/posix-timers.c @@ -522,13 +522,13 @@ void posix_timers_register_clock(const clockid_t clock_id, return; }
- if (!new_clock->clock_get) { - printk(KERN_WARNING "POSIX clock id %d lacks clock_get()\n", + if (!new_clock->clock_get && !new_clock->clock_get64) { + printk(KERN_WARNING "POSIX clock id %d lacks clock_get() and clock_get64()\n", clock_id); return; } - if (!new_clock->clock_getres) { - printk(KERN_WARNING "POSIX clock id %d lacks clock_getres()\n", + if (!new_clock->clock_getres && !new_clock->clock_getres64) { + printk(KERN_WARNING "POSIX clock id %d lacks clock_getres() and clock_getres64()\n", clock_id); return; } @@ -579,7 +579,7 @@ static struct k_clock *clockid_to_kclock(const clockid_t id) return (id & CLOCKFD_MASK) == CLOCKFD ? &clock_posix_dynamic : &clock_posix_cpu;
- if (id >= MAX_CLOCKS || !posix_clocks[id].clock_getres) + if (id >= MAX_CLOCKS || (!posix_clocks[id].clock_getres && !posix_clocks[id].clock_getres64)) return NULL; return &posix_clocks[id]; } @@ -771,6 +771,7 @@ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct itimerspec __user *, setting) { struct itimerspec cur_setting; + struct itimerspec64 cur_setting64; struct k_itimer *timr; struct k_clock *kc; unsigned long flags; @@ -781,10 +782,16 @@ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, return -EINVAL;
kc = clockid_to_kclock(timr->it_clock); - if (WARN_ON_ONCE(!kc || !kc->timer_get)) + if (WARN_ON_ONCE(!kc || (!kc->timer_get && !kc->timer_get64))) { ret = -EINVAL; - else - kc->timer_get(timr, &cur_setting); + } else { + if (kc->timer_get64) { + kc->timer_get64(timr, &cur_setting64); + cur_setting = itimerspec64_to_itimerspec(cur_setting64); + } else { + kc->timer_get(timr, &cur_setting); + } + }
unlock_timer(timr, flags);
@@ -877,6 +884,7 @@ SYSCALL_DEFINE4(timer_settime, timer_t, timer_id, int, flags, { struct k_itimer *timr; struct itimerspec new_spec, old_spec; + struct itimerspec64 new_spec64, old_spec64; int error = 0; unsigned long flag; struct itimerspec *rtn = old_setting ? &old_spec : NULL; @@ -897,10 +905,19 @@ retry: return -EINVAL;
kc = clockid_to_kclock(timr->it_clock); - if (WARN_ON_ONCE(!kc || !kc->timer_set)) + if (WARN_ON_ONCE(!kc || (!kc->timer_set && !kc->timer_set64))) { error = -EINVAL; - else - error = kc->timer_set(timr, flags, &new_spec, rtn); + } else { + if (kc->timer_set64) { + new_spec64 = itimerspec_to_itimerspec64(new_spec); + error = kc->timer_set64(timr, flags, &new_spec64, + &old_spec64); + if (old_setting) + old_spec = itimerspec64_to_itimerspec(old_spec64); + } else { + error = kc->timer_set(timr, flags, &new_spec, rtn); + } + }
unlock_timer(timr, flag); if (error == TIMER_RETRY) { @@ -1007,14 +1024,20 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock, { struct k_clock *kc = clockid_to_kclock(which_clock); struct timespec new_tp; + struct timespec64 new_tp64;
- if (!kc || !kc->clock_set) + if (!kc || (!kc->clock_set && !kc->clock_set64)) return -EINVAL;
if (copy_from_user(&new_tp, tp, sizeof (*tp))) return -EFAULT;
- return kc->clock_set(which_clock, &new_tp); + if (kc->clock_set64) { + new_tp64 = timespec_to_timespec64(new_tp); + return kc->clock_set64(which_clock, &new_tp64); + } else { + return kc->clock_set(which_clock, &new_tp); + } }
SYSCALL_DEFINE2(clock_gettime, const clockid_t, which_clock, @@ -1022,12 +1045,18 @@ SYSCALL_DEFINE2(clock_gettime, const clockid_t, which_clock, { struct k_clock *kc = clockid_to_kclock(which_clock); struct timespec kernel_tp; + struct timespec64 kernel_tp64; int error;
if (!kc) return -EINVAL;
- error = kc->clock_get(which_clock, &kernel_tp); + if (kc->clock_get64) { + error = kc->clock_get64(which_clock, &kernel_tp64); + kernel_tp = timespec64_to_timespec(kernel_tp64); + } else { + error = kc->clock_get(which_clock, &kernel_tp); + }
if (!error && copy_to_user(tp, &kernel_tp, sizeof (kernel_tp))) error = -EFAULT; @@ -1063,12 +1092,18 @@ SYSCALL_DEFINE2(clock_getres, const clockid_t, which_clock, { struct k_clock *kc = clockid_to_kclock(which_clock); struct timespec rtn_tp; + struct timespec64 rtn_tp64; int error;
if (!kc) return -EINVAL;
- error = kc->clock_getres(which_clock, &rtn_tp); + if (kc->clock_getres64) { + error = kc->clock_getres64(which_clock, &rtn_tp64); + rtn_tp = timespec64_to_timespec(rtn_tp64); + } else { + error = kc->clock_getres(which_clock, &rtn_tp); + }
if (!error && tp && copy_to_user(tp, &rtn_tp, sizeof (rtn_tp))) error = -EFAULT;
On Mon, 20 Apr 2015, Baolin Wang wrote:
@@ -771,6 +771,7 @@ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct itimerspec __user *, setting) { struct itimerspec cur_setting;
- struct itimerspec64 cur_setting64; struct k_itimer *timr; struct k_clock *kc; unsigned long flags;
@@ -781,10 +782,16 @@ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, return -EINVAL; kc = clockid_to_kclock(timr->it_clock);
- if (WARN_ON_ONCE(!kc || !kc->timer_get))
- if (WARN_ON_ONCE(!kc || (!kc->timer_get && !kc->timer_get64))) { ret = -EINVAL;
- else
kc->timer_get(timr, &cur_setting);
- } else {
if (kc->timer_get64) {kc->timer_get64(timr, &cur_setting64);cur_setting = itimerspec64_to_itimerspec(cur_setting64);} else {kc->timer_get(timr, &cur_setting);}- }
This is really horrible. You add a metric ton of conditionals to every syscall just to remove them later again. I have not yet checked the end result, but this approach is error prone as hell and just introduces completely useless code churn.
It's useless because you do not factor out the guts of the syscall functions so we can reuse the very same logic for the future 2038 safe syscalls which we need to introduce for 32bit machines.
Take a look at the compat syscalls. They do the right thing.
COMPAT_SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct compat_itimerspec __user *, setting) { long err; mm_segment_t oldfs; struct itimerspec ts;
oldfs = get_fs(); set_fs(KERNEL_DS); err = sys_timer_gettime(timer_id, (struct itimerspec __user *) &ts); set_fs(oldfs); if (!err && put_compat_itimerspec(setting, &ts)) return -EFAULT; return err; }
So we can be clever and do the following:
1) Preparatory work in posix-timer.c (Patch #1)
- Split out the guts of the syscall and change the syscall implementation
static int __timer_gettime(timer_t timer_id, struct itimerspec *cur_setting) { struct k_itimer *timr; struct k_clock *kc; unsigned long flags; int ret = 0;
timr = lock_timer(timer_id, &flags); if (!timr) return -EINVAL;
kc = clockid_to_kclock(timr->it_clock); if (WARN_ON_ONCE(!kc || !kc->timer_get)) ret = -EINVAL; else kc->timer_get(timr, &cur_setting);
unlock_timer(timr, flags); return ret; }
/* Get the time remaining on a POSIX.1b interval timer. */ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct itimerspec __user *, setting) { struct itimerspec cur_setting; int ret = __timer_gettime(timer_id, &cur_setting);
if (!ret && copy_to_user(setting, &cur_setting, sizeof (cur_setting))) return -EFAULT;
return ret; }
2) Do the 64bit infrastructure work in posix-timer.c (Patch #2)
- Introduce k_clock->timer_get64() and provide a stub function
static int default_timer_get64(struct k_clock *kc, struct k_itimer *timr, struct itimerspec64 *cur_setting64) { struct itimerspec cur_setting;
kc->timer_get(timer, &cur_setting); return 0; }
- Add the following to posix_timers_register_clock()
if (kc->timer_get && !kc->timer_get64) kc->timer_get64 = default_timer_get64;
- Convert __timer_gettime to 64bit
-static int __timer_gettime(timer_t timer_id, struct itimerspec64 *cur_setting) +static int __timer_gettime(timer_t timer_id, struct itimerspec *cur_setting) { ... kc = clockid_to_kclock(timr->it_clock); + if (WARN_ON_ONCE(!kc || !kc->timer)) - if (WARN_ON_ONCE(!kc || !kc->timer_get64)) ret = -EINVAL; else - kc->timer_get(timr, &cur_setting); + kc->timer_get64(timr, &cur_setting);
unlock_timer(timr, flags); return ret; }
- Change the syscall implementation in the following way:
/* Get the time remaining on a POSIX.1b interval timer. */ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct itimerspec __user *, setting) { #ifdef CONFIG_64BIT struct itimerspec64 cur_setting; int ret = __timer_gettime(timer_id, &cur_setting); #else struct itimerspec64 cur_setting64; struct itimerspec cur_setting; int ret = __timer_gettime(timer_id, &cur_setting64);
if (!ret) cur_setting = itimerspec64_to_itimerspec(&cur_setting64); #endif if (!ret && copy_to_user(setting, &cur_setting, sizeof (cur_setting))) return -EFAULT; return ret; }
The result is two simple to review patches with minimal code churn.
The nice thing is that once we introduce new syscalls for 32bit machines, e.g. sys_timer_gettime64(), all we need to do is:
/* Get the time remaining on a POSIX.1b interval timer. */ SYSCALL_DEFINE2(timer_gettime64, timer_t, timer_id, struct itimerspec64 __user *, setting) { struct itimerspec64 cur_setting64; int ret = __timer_gettime(timer_id, &cur_setting64);
if (!ret && copy_to_user(setting, &cur_setting, sizeof (cur_setting))) return -EFAULT;
return ret; }
And on 64bit timer_gettime64() and timer_gettime() are the same, so we just need to do a clever mapping of timer_gettime() to timer_gettime64(). Not rocket science....
For 32 bit we provide the old timer_gettime() for non converted applications:
#ifdef CONFIG_32BIT_OLD_TIMESPEC_SYSCALLS /* Get the time remaining on a POSIX.1b interval timer in the old timespec format. */ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct itimerspec __user *, setting) { struct itimerspec64 cur_setting64; struct itimerspec cur_setting; int ret = __timer_gettime(timer_id, &cur_setting64);
if (!ret) { cur_setting = itimerspec64_to_itimerspec(&cur_setting64);
if (copy_to_user(setting, &cur_setting, sizeof (cur_setting))) return -EFAULT; } return ret; } #endif
Simple, isn't it? No useless churn. Proper refactoring for the next step. No useless copying for 64 bit.
3) Change one implementation after the other (Patches #3 - N)
4) Remove timer_get and the default implementation for timer_get64 and the hack in posix_timers_register_clock(). (Patch #N+1)
Send that lot out and let it review. Once this is fine we can move to the next syscall.
Thanks,
tglx
On Monday 20 April 2015 22:40:57 Thomas Gleixner wrote:
On Mon, 20 Apr 2015, Baolin Wang wrote:
@@ -771,6 +771,7 @@ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct itimerspec __user *, setting) { struct itimerspec cur_setting;
- struct itimerspec64 cur_setting64; struct k_itimer *timr; struct k_clock *kc; unsigned long flags;
@@ -781,10 +782,16 @@ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, return -EINVAL; kc = clockid_to_kclock(timr->it_clock);
- if (WARN_ON_ONCE(!kc || !kc->timer_get))
- if (WARN_ON_ONCE(!kc || (!kc->timer_get && !kc->timer_get64))) { ret = -EINVAL;
- else
kc->timer_get(timr, &cur_setting);
- } else {
if (kc->timer_get64) {kc->timer_get64(timr, &cur_setting64);cur_setting = itimerspec64_to_itimerspec(cur_setting64);} else {kc->timer_get(timr, &cur_setting);}- }
This is really horrible. You add a metric ton of conditionals to every syscall just to remove them later again. I have not yet checked the end result, but this approach is error prone as hell and just introduces completely useless code churn.
It's useless because you do not factor out the guts of the syscall functions so we can reuse the very same logic for the future 2038 safe syscalls which we need to introduce for 32bit machines.
Hi Thomas,
I should probably go ahead and introduce all the new syscalls in a separate patch series. I have my own ideas about how to get there, in a slightly different way from what you propose:
Take a look at the compat syscalls. They do the right thing.
COMPAT_SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct compat_itimerspec __user *, setting) { long err; mm_segment_t oldfs; struct itimerspec ts;
oldfs = get_fs(); set_fs(KERNEL_DS); err = sys_timer_gettime(timer_id, (struct itimerspec __user *) &ts); set_fs(oldfs); if (!err && put_compat_itimerspec(setting, &ts)) return -EFAULT; return err;}
As a side note, I want to kill off the get_fs()/set_fs() calls in the process. These always make me dizzy when I try to work out whether there is a potential security hole (there is not in this case), and they can be slow on some architectures.
So we can be clever and do the following:
Preparatory work in posix-timer.c (Patch #1)
Do the 64bit infrastructure work in posix-timer.c (Patch #2)
The result is two simple to review patches with minimal code churn.
This all sounds great, good idea.
The nice thing is that once we introduce new syscalls for 32bit machines, e.g. sys_timer_gettime64(), all we need to do is:
/* Get the time remaining on a POSIX.1b interval timer. */ SYSCALL_DEFINE2(timer_gettime64, timer_t, timer_id, struct itimerspec64 __user *, setting) { struct itimerspec64 cur_setting64; int ret = __timer_gettime(timer_id, &cur_setting64);
if (!ret && copy_to_user(setting, &cur_setting, sizeof (cur_setting))) return -EFAULT;
return ret; }
And on 64bit timer_gettime64() and timer_gettime() are the same, so we just need to do a clever mapping of timer_gettime() to timer_gettime64(). Not rocket science....
For 32 bit we provide the old timer_gettime() for non converted applications:
#ifdef CONFIG_32BIT_OLD_TIMESPEC_SYSCALLS /* Get the time remaining on a POSIX.1b interval timer in the old timespec format. */ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct itimerspec __user *, setting) { struct itimerspec64 cur_setting64; struct itimerspec cur_setting; int ret = __timer_gettime(timer_id, &cur_setting64);
if (!ret) { cur_setting = itimerspec64_to_itimerspec(&cur_setting64);
if (copy_to_user(setting, &cur_setting, sizeof (cur_setting))) return -EFAULT;} return ret; } #endif
Simple, isn't it? No useless churn. Proper refactoring for the next step. No useless copying for 64 bit.
My preferred solution is one where we end up with the same syscalls for both 32-bit and 64-bit, and basically use the compat_sys_timer_gettime() implementation (or a simplified version) for the existing , something like this:
diff --git a/arch/x86/syscalls/syscall_32.tbl b/arch/x86/syscalls/syscall_32.tbl index ef8187f9d28d..71e74a47a2cf 100644 --- a/arch/x86/syscalls/syscall_32.tbl +++ b/arch/x86/syscalls/syscall_32.tbl @@ -267,7 +267,7 @@ 258 i386 set_tid_address sys_set_tid_address 259 i386 timer_create sys_timer_create compat_sys_timer_create 260 i386 timer_settime sys_timer_settime compat_sys_timer_settime -261 i386 timer_gettime sys_timer_gettime compat_sys_timer_gettime +261 i386 timer_gettime compat_sys_timer_gettime 262 i386 timer_getoverrun sys_timer_getoverrun 263 i386 timer_delete sys_timer_delete 264 i386 clock_settime sys_clock_settime compat_sys_clock_settime @@ -365,3 +365,4 @@ 356 i386 memfd_create sys_memfd_create 357 i386 bpf sys_bpf 358 i386 execveat sys_execveat stub32_execveat +359 i386 timer_gettime64 sys_timer_gettime diff --git a/kernel/compat.c b/kernel/compat.c index 24f00610c575..15d4f5abb492 100644 --- a/kernel/compat.c +++ b/kernel/compat.c @@ -723,18 +723,14 @@ COMPAT_SYSCALL_DEFINE4(timer_settime, timer_t, timer_id, int, flags, COMPAT_SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct compat_itimerspec __user *, setting) { - long err; - mm_segment_t oldfs; - struct itimerspec ts; + struct itimerspec64 ts; + long ret;
- oldfs = get_fs(); - set_fs(KERNEL_DS); - err = sys_timer_gettime(timer_id, - (struct itimerspec __user *) &ts); - set_fs(oldfs); - if (!err && put_compat_itimerspec(setting, &ts)) - return -EFAULT; - return err; + ret = __timer_gettime(timer_id, &ts); + if (ret) + return ret; + + return put_compat_itimerspec(setting, &ts); }
COMPAT_SYSCALL_DEFINE2(clock_settime, clockid_t, which_clock, diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c index 31ea01f42e1f..c601f1969f5a 100644 --- a/kernel/time/posix-timers.c +++ b/kernel/time/posix-timers.c @@ -766,32 +766,27 @@ common_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting) cur_setting->it_value = ktime_to_timespec(remaining); }
+static int put_compat_itimerspec(struct __kernel_itimerspec64 __user *dst, + const struct itimerspec64 *src) +{ + if (__put_timespec64(&src->it_interval, &dst->it_interval) || + __put_timespec64(&src->it_value, &dst->it_value)) + return -EFAULT; + return 0; +} + /* Get the time remaining on a POSIX.1b interval timer. */ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, - struct itimerspec __user *, setting) + struct __kernel_itimerspec64 __user *, setting) { - struct itimerspec cur_setting; - struct k_itimer *timr; - struct k_clock *kc; - unsigned long flags; - int ret = 0; - - timr = lock_timer(timer_id, &flags); - if (!timr) - return -EINVAL; + struct itimerspec64 cur_setting; + int ret;
- kc = clockid_to_kclock(timr->it_clock); - if (WARN_ON_ONCE(!kc || !kc->timer_get)) - ret = -EINVAL; - else - kc->timer_get(timr, &cur_setting); + ret = __timer_gettime(timer_id, &cur_setting); + if (ret) + return ret;
- unlock_timer(timr, flags); - - if (!ret && copy_to_user(setting, &cur_setting, sizeof (cur_setting))) - return -EFAULT; - - return ret; + return put_itimerspec(setting, &cur_setting); }
/*
Note the use of a separate __kernel_itimerspec64 for the user interface here, which I think will be needed to hide the differences between the normal itimerspec on 64-bit machines, and the new itimerspec on 32-bit platforms that will be defined differently (using 'long long').
My plan for migration here is to temporarily #define __kernel_itimerspec64 as itimerspec on 32-bit architectures (which may be a bit confusing), and then introduce a new CONFIG_COMPAT_TIME option that is set by each architecture that has been converted over to use the new syscalls. This way we can do one syscall at a time at first, followed by one architecture at a time.
Unless you have serious objections to that plan, I'd like to work on a first version of this myself and send that out for clarifications.
I would also prefer not too many people to work on the syscalls, and would rather have Baolin not touch any of the syscall prototypes for the moment.
Arnd
On Tue, 21 Apr 2015, Arnd Bergmann wrote:
COMPAT_SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, struct compat_itimerspec __user *, setting)
As a side note, I want to kill off the get_fs()/set_fs() calls in the process. These always make me dizzy when I try to work out whether there is a potential security hole (there is not in this case), and they can be slow on some architectures.
Yeah. I have to take a deep breath every time I look at those :)
My preferred solution is one where we end up with the same syscalls for both 32-bit and 64-bit, and basically use the compat_sys_timer_gettime() implementation (or a simplified version) for the existing , something like this:
No objections from my side. I was not looking into the syscall magic yet. I just wanted to avoid the code churn and have the guts of the syscalls factored out for simple reusage.
....
Note the use of a separate __kernel_itimerspec64 for the user interface here, which I think will be needed to hide the differences between the normal itimerspec on 64-bit machines, and the new itimerspec on 32-bit platforms that will be defined differently (using 'long long').
Confused.
timespec64 / itimerspec64 should be the same independent of 64bit and 32bit. So why do we need another variant ?
I would also prefer not too many people to work on the syscalls, and would rather have Baolin not touch any of the syscall prototypes for the moment.
I did not ask him to change any of the syscall prototypes. I just wanted him to split out the guts of the syscall into a seperate static function to avoid all that code churn.
Thanks,
tglx
On Tuesday 21 April 2015 16:14:26 Thomas Gleixner wrote:
Note the use of a separate __kernel_itimerspec64 for the user interface here, which I think will be needed to hide the differences between the normal itimerspec on 64-bit machines, and the new itimerspec on 32-bit platforms that will be defined differently (using 'long long').
Confused.
timespec64 / itimerspec64 should be the same independent of 64bit and 32bit. So why do we need another variant ?
There are multiple reasons:
* On 64-bit systems, timespec64 would always be defined in the same way as struct timespec { __kernel_time_t tv_sec; long tv_nsec; }, with __kernel_time_t being 'long'. On 32-bit, we probably need to make both members 'long long' for the user space side, in order to share the syscall implementation with the kernel side, but we may also want to keep the internal timespec64 using a 'long' for tv_nsec, as we do today. This means that both the binary layout (padding or no padding) and the basic types (long or long long) are different between 32-bit and 64-bit, and between kernel and user space * We should not put 'struct timespec64' into the user space namespace, as applications might already use that identifier. This is similar to the __u32/u32 or __kernel_time_t/time_t tuple of types for interface and in-kernel uses. This is particularly important when embedding a timespec in another data structure. * My plan is to use a temporary hack where I actually define __kernel_timespec64 to look like the 32-bit version of timespec, as an intermediate step when converting all 32-bit architectures over to use the compat_*() syscalls in place of the existing ones, so I can change over the normal syscalls to use __kernel_timespec64 without having to change all architectures at once, or having to modify each syscall multiple times.
I would also prefer not too many people to work on the syscalls, and would rather have Baolin not touch any of the syscall prototypes for the moment.
I did not ask him to change any of the syscall prototypes. I just wanted him to split out the guts of the syscall into a seperate static function to avoid all that code churn.
Ok, I wasn't sure about that part, thanks for clarifying.
Arnd
On Tue, 21 Apr 2015, Arnd Bergmann wrote:
On Tuesday 21 April 2015 16:14:26 Thomas Gleixner wrote:
Note the use of a separate __kernel_itimerspec64 for the user interface here, which I think will be needed to hide the differences between the normal itimerspec on 64-bit machines, and the new itimerspec on 32-bit platforms that will be defined differently (using 'long long').
Confused.
timespec64 / itimerspec64 should be the same independent of 64bit and 32bit. So why do we need another variant ?
There are multiple reasons:
- On 64-bit systems, timespec64 would always be defined in the same way as struct timespec { __kernel_time_t tv_sec; long tv_nsec; }, with __kernel_time_t being 'long'. On 32-bit, we probably need to make both members 'long long' for the user space side, in order to share the syscall implementation with the kernel side, but we may also want to keep the internal timespec64 using a 'long' for tv_nsec, as we do today. This means that both the binary layout (padding or no padding) and the basic types (long or long long) are different between 32-bit and 64-bit, and between kernel and user space
So you want to avoid a compat syscall for 32bit applications on a 64bit kernel, right?
That burdens 32bit with the extra 'long long' in user space. Not sure whether user space folks will be happy about it.
- We should not put 'struct timespec64' into the user space namespace, as applications might already use that identifier. This is similar to the __u32/u32 or __kernel_time_t/time_t tuple of types for interface and in-kernel uses. This is particularly important when embedding a timespec in another data structure.
Fair enough.
- My plan is to use a temporary hack where I actually define __kernel_timespec64 to look like the 32-bit version of timespec, as an intermediate step when converting all 32-bit architectures over to use the compat_*() syscalls in place of the existing ones, so I can change over the normal syscalls to use __kernel_timespec64 without having to change all architectures at once, or having to modify each syscall multiple times.
Makes sense.
Thanks,
tglx
On Tuesday 21 April 2015 17:13:32 Thomas Gleixner wrote:
On Tue, 21 Apr 2015, Arnd Bergmann wrote:
On Tuesday 21 April 2015 16:14:26 Thomas Gleixner wrote:
Note the use of a separate __kernel_itimerspec64 for the user interface here, which I think will be needed to hide the differences between the normal itimerspec on 64-bit machines, and the new itimerspec on 32-bit platforms that will be defined differently (using 'long long').
Confused.
timespec64 / itimerspec64 should be the same independent of 64bit and 32bit. So why do we need another variant ?
There are multiple reasons:
- On 64-bit systems, timespec64 would always be defined in the same way as struct timespec { __kernel_time_t tv_sec; long tv_nsec; }, with __kernel_time_t being 'long'. On 32-bit, we probably need to make both members 'long long' for the user space side, in order to share the syscall implementation with the kernel side, but we may also want to keep the internal timespec64 using a 'long' for tv_nsec, as we do today. This means that both the binary layout (padding or no padding) and the basic types (long or long long) are different between 32-bit and 64-bit, and between kernel and user space
So you want to avoid a compat syscall for 32bit applications on a 64bit kernel, right?
That is the idea at the moment, yes. At least for the kernel-user interface.
That burdens 32bit with the extra 'long long' in user space. Not sure whether user space folks will be happy about it.
I know there are concerns about this, in particular because C11 and POSIX both require tv_nsec to be 'long', unlike timeval->tv_usec, which is a 'suseconds_t' and can be defined as 'long long'.
There are four possible ways that 32-bit user space could define timespec based on the uncontroversial (I hope) 'typedef long long time_t;'.
a)
struct timespec { time_t tv_sec; long long tv_nsec; /* or typedef long long snseconds_t */ };
This is not directly compatible with C11 or POSIX.1-2008, but it matches what we do inside of 64-bit kernels, so probably has the highest chance of working correctly in practice
b)
struct timespec { time_t tv_sec; long tv_nsec; };
This is the definition according to C11 and POSIX, and the main problem here is the padding, which may cause a 4-byte data leak and has the tv_nsec in the wrong place when used together with the timespec we have in the kernel on big-endian 64-bit machines.
c)
struct timespec { time_t tv_sec; #if __BITS_PER_LONG == 32 && __BYTE_ORDER == __BIG_ENDIAN long __pad; #endif long tv_nsec; /* or typedef long long snseconds_t */ #if __BITS_PER_LONG == 32 && __BYTE_ORDER == __LITTLE_ENDIAN long __pad; #endif };
This version could be used transparently by user space, but has the potential to cause problems with existing user space doing things like
struct timespec ts = { 0, 1000 }; /* one microsecond */
even though it is probably compliant.
d)
struct timespec { time_t tv_sec; #if __BITS_PER_LONG == 32 && __BYTE_ORDER == __BIG_ENDIAN long :32; #endif long tv_nsec; /* or typedef long long snseconds_t */ #if __BITS_PER_LONG == 32 && __BYTE_ORDER == __LITTLE_ENDIAN long :32; #endif };
This is very similar to c, but trades the problem I described above for a dependency on a gcc extension that is not part of C11 or any earlier standard.
From the kernel's point of view, b), c) and d) can all be
treated the same for output data, as we only ever pass back normalized timespec structures that have zeroes in the upper bits of timespec. However, for input to the kernel, we would require an extra conditional on 64-bit kernels to decide whether we ignore the upper bits (doing tv->tv_nsec &= 0xffffffff) or a structure that has the upper bits set needs to cause EINVAL. We could still do that in get_timespec() if we decide to not to go with a).
See also https://lwn.net/Articles/457089/ for an earlier discussion on the topic when debating the x32 ABI.
Arnd
On Tue, 21 Apr 2015, Arnd Bergmann wrote:
I know there are concerns about this, in particular because C11 and POSIX both require tv_nsec to be 'long', unlike timeval->tv_usec, which is a 'suseconds_t' and can be defined as 'long long'.
a)
struct timespec { time_t tv_sec; long long tv_nsec; /* or typedef long long snseconds_t */ };
This is not directly compatible with C11 or POSIX.1-2008, but it matches what we do inside of 64-bit kernels, so probably has the highest chance of working correctly in practice
After reading Linus rant in the x32 thread again (thanks for the reminder), and looking at b/c/d - which rate between ugly and butt ugly - I think we should go for a) and screw POSIX and C11 as those committee dinosaurs seem to completely ignore the 2038 problem on 32bit machines. At least I have not found any hint that these folks care at all. So why should we comply to something which is completely useless?
That also makes the question about the upper 32bits check moot, so it's the simplest and clearest of the possible solutions.
Thanks,
tglx
On Tue, 21 Apr 2015, Thomas Gleixner wrote:
On Tue, 21 Apr 2015, Arnd Bergmann wrote:
I know there are concerns about this, in particular because C11 and POSIX both require tv_nsec to be 'long', unlike timeval->tv_usec, which is a 'suseconds_t' and can be defined as 'long long'.
a)
struct timespec { time_t tv_sec; long long tv_nsec; /* or typedef long long snseconds_t */ };
This is not directly compatible with C11 or POSIX.1-2008, but it matches what we do inside of 64-bit kernels, so probably has the highest chance of working correctly in practice
After reading Linus rant in the x32 thread again (thanks for the reminder), and looking at b/c/d - which rate between ugly and butt ugly - I think we should go for a) and screw POSIX and C11 as those committee dinosaurs seem to completely ignore the 2038 problem on 32bit machines. At least I have not found any hint that these folks care at all. So why should we comply to something which is completely useless?
That also makes the question about the upper 32bits check moot, so it's the simplest and clearest of the possible solutions.
Second thoughts after some sleep.
So the outcome of this is going to be that user space libraries will not expose the syscall variant of
syscall_timespec64 { s64 tv_sec; s64 tv_nsec; };
to applications. The libs will translate them to spec conforming
timespec { time_t tv_sec; long tv_nsec; };
anyway. That means we have two translation steps on 32bit systems:
1) user space timespec -> syscall timespec64
2) syscall timespec64 -> scalar nsec s64 (ktime_t)
and the other way round. The kernel internal representation is simply s64 (nsec) based all over the place.
So we could save one translation step if we implement new syscalls which have a scalar nsec interface instead of the timespec/timeval cruft and let user space do the translation to whatever it wants.
So
sys_clock_nanosleep(const clockid_t which_clock, int flags, const struct timespec __user *expires, struct timespec __user *reminder)
would get the new syscall variant:
sys_clock_nanosleep_ns(const clockid_t which_clock, int flags, const s64 expires, s64 __user *reminder)
I personally would welcome such an interface as it makes user space programming simpler. Just (re)arming a periodic nanosleep based on absolute expiry time is horrible stupid today:
struct timespec expires; .... while () expires.tv_nsec += period.tv_nsec; expires.tv_sec += period.tv_sec; normalize_timespec(&expires); sys_clock_nanosleep(CLOCK_ID, ABS, &expires, NULL);
So with a scalar interface this would reduce to:
s64 expires; .... while () expires += period; sys_clock_nanosleep_ns(CLOCK_ID, ABS, &expires, NULL);
There is a difference both in text and storage size plus the avoidance of the two translation steps (one translation step on 64bit).
I know that this is non portable, but OTOH if I look at the non portable mechanisms which are used by data bases, java VMs and other apps which exist to squeeze the last cycles out of the system, there is certainly some value to that.
The portable/spec conforming apps can still use the user space assisted translated timespec/timeval mechanisms.
There is one caveat though: sys_clock_gettime and sys_gettimeofday will still need a syscall_timespec64 variant. We have no double translation steps there because we maintain the timespec representation in the timekeeping code for performance reasons to avoid the division in the syscall interface. But everything else can do nicely without the timespec cruft.
We really should talk to libc folks and high performance users about this before blindly adding a gazillion of new timespec64 based interfaces.
Thoughts?
Thanks,
tglx
On Wed, Apr 22, 2015 at 10:45:23AM +0200, Thomas Gleixner wrote:
So we could save one translation step if we implement new syscalls which have a scalar nsec interface instead of the timespec/timeval cruft and let user space do the translation to whatever it wants.
+1
I personally would welcome such an interface as it makes user space programming simpler. Just (re)arming a periodic nanosleep based on absolute expiry time is horrible stupid today:
Jup.
Thoughts?
Current user space example: The linuxptp programs are doing ns64 to timespec conversions to call into the kernel, which then does timespec to ns64 to talk to the hardware. I would bet that most (all?) use cases are better served with 64 bit nanosecond system calls.
Thanks, Richard
From: Thomas Gleixner
Sent: 22 April 2015 09:45 On Tue, 21 Apr 2015, Thomas Gleixner wrote:
On Tue, 21 Apr 2015, Arnd Bergmann wrote:
I know there are concerns about this, in particular because C11 and POSIX both require tv_nsec to be 'long', unlike timeval->tv_usec, which is a 'suseconds_t' and can be defined as 'long long'.
a)
struct timespec { time_t tv_sec; long long tv_nsec; /* or typedef long long snseconds_t */ };
This is not directly compatible with C11 or POSIX.1-2008, but it matches what we do inside of 64-bit kernels, so probably has the highest chance of working correctly in practice
After reading Linus rant in the x32 thread again (thanks for the reminder), and looking at b/c/d - which rate between ugly and butt ugly - I think we should go for a) and screw POSIX and C11 as those committee dinosaurs seem to completely ignore the 2038 problem on 32bit machines. At least I have not found any hint that these folks care at all. So why should we comply to something which is completely useless?
That also makes the question about the upper 32bits check moot, so it's the simplest and clearest of the possible solutions.
Second thoughts after some sleep.
So the outcome of this is going to be that user space libraries will not expose the syscall variant of
syscall_timespec64 { s64 tv_sec; s64 tv_nsec; };to applications. The libs will translate them to spec conforming
timespec { time_t tv_sec; long tv_nsec; };
anyway. That means we have two translation steps on 32bit systems:
user space timespec -> syscall timespec64
syscall timespec64 -> scalar nsec s64 (ktime_t)
and the other way round. The kernel internal representation is simply s64 (nsec) based all over the place.
Do you need the double-translation? If all the kernel uses a 64bit nsec value the in-kernel syscall stub can convert the user-supplied values appropriately before calling the standard function. Not that a syscall that takes a linear nsec value isn't useful.
FWIW I can't remember what NetBSD did when they extended time_t to 64bits.
David
On Wednesday 22 April 2015 10:45:23 Thomas Gleixner wrote:
On Tue, 21 Apr 2015, Thomas Gleixner wrote:
So we could save one translation step if we implement new syscalls which have a scalar nsec interface instead of the timespec/timeval cruft and let user space do the translation to whatever it wants.
So
sys_clock_nanosleep(const clockid_t which_clock, int flags, const struct timespec __user *expires, struct timespec __user *reminder)
would get the new syscall variant:
sys_clock_nanosleep_ns(const clockid_t which_clock, int flags, const s64 expires, s64 __user *reminder)
As you might expect, there are a number of complications with this approach:
- John Stultz likes to point out that it's easier to do one change at a time, so extending the interface to 64-bit has less potential of breaking things than a more fundamental change. I think it's useful to drop a lot of the syscalls when a more modern version is around (e.g. let libc implement usleep and nanosleep through clock_nanosleep), but keep the syscalls as close to the known-working 64-bit versions as we can. - The inode timestamp related syscalls (stat, utimes and variants thereof) require the full range of time64_t and cannot use ktime_t. - converting between timespec types of different size is cheap, converting timespec to ktime_t is still relatively cheap, but converting ktime_t to timespec is rather expensive (at least eight 32-bit multiplies, plus a few shifts and additions if you don't have 64-bit arithmetic). - ioctls that pass a timespec need to keep doing that or would require a source-level change in user space instead of recompiling.
I personally would welcome such an interface as it makes user space programming simpler. Just (re)arming a periodic nanosleep based on absolute expiry time is horrible stupid today:
struct timespec expires; .... while () expires.tv_nsec += period.tv_nsec; expires.tv_sec += period.tv_sec; normalize_timespec(&expires); sys_clock_nanosleep(CLOCK_ID, ABS, &expires, NULL);So with a scalar interface this would reduce to:
s64 expires; .... while () expires += period; sys_clock_nanosleep_ns(CLOCK_ID, ABS, &expires, NULL);There is a difference both in text and storage size plus the avoidance of the two translation steps (one translation step on 64bit).
We should probably look at it separately for each syscall. It's quite possible that we find a number of them for which it helps and others for which it hurts, so we need to see the big pictures.
There are also a few other calls that will never need 64-bit time_t because the range is limited by the need to only ever pass relative timeouts (select, poll, io_getevents, recvmmsg, clock_getres, rt_sigtimedwait, sched_rr_get_interval, getrusage, waitid, semtimedop, sysinfo), so we could actually leave them using a 32-bit structure and have the libc do the conversion.
I know that this is non portable, but OTOH if I look at the non portable mechanisms which are used by data bases, java VMs and other apps which exist to squeeze the last cycles out of the system, there is certainly some value to that.
The portable/spec conforming apps can still use the user space assisted translated timespec/timeval mechanisms.
There is one caveat though: sys_clock_gettime and sys_gettimeofday will still need a syscall_timespec64 variant. We have no double translation steps there because we maintain the timespec representation in the timekeeping code for performance reasons to avoid the division in the syscall interface. But everything else can do nicely without the timespec cruft.
We really should talk to libc folks and high performance users about this before blindly adding a gazillion of new timespec64 based interfaces.
I've started a list of affected syscalls at https://docs.google.com/spreadsheets/d/1HCYwHXxs48TsTb6IGUduNjQnmfRvMPzCN6T_...
Still adding more calls and description, let me know if you want edit permissions.
Arnd
On Wed, 22 Apr 2015, Arnd Bergmann wrote:
On Wednesday 22 April 2015 10:45:23 Thomas Gleixner wrote:
On Tue, 21 Apr 2015, Thomas Gleixner wrote:
So we could save one translation step if we implement new syscalls which have a scalar nsec interface instead of the timespec/timeval cruft and let user space do the translation to whatever it wants.
So
sys_clock_nanosleep(const clockid_t which_clock, int flags, const struct timespec __user *expires, struct timespec __user *reminder)
would get the new syscall variant:
sys_clock_nanosleep_ns(const clockid_t which_clock, int flags, const s64 expires, s64 __user *reminder)
As you might expect, there are a number of complications with this approach:
- John Stultz likes to point out that it's easier to do one change at a time, so extending the interface to 64-bit has less potential of breaking things than a more fundamental change. I think it's useful to drop a lot of the syscalls when a more modern version is around (e.g. let libc implement usleep and nanosleep through clock_nanosleep), but keep the syscalls as close to the known-working 64-bit versions as we can.
Well. I don't see a massive risk when implementing e.g. usleep/nanosleep & al with clock_nanosleep_ns().
- The inode timestamp related syscalls (stat, utimes and variants thereof) require the full range of time64_t and cannot use ktime_t.
I'm aware that there are a lot of interfaces which cannot use ktime_t. That's fine.
- converting between timespec types of different size is cheap, converting timespec to ktime_t is still relatively cheap, but converting ktime_t to timespec is rather expensive (at least eight 32-bit multiplies, plus a few shifts and additions if you don't have 64-bit arithmetic).
Right. That's what I said vs. gettime().
- ioctls that pass a timespec need to keep doing that or would require a source-level change in user space instead of recompiling.
No argument here.
We should probably look at it separately for each syscall. It's quite possible that we find a number of them for which it helps and others for which it hurts, so we need to see the big pictures.
Agreed.
There are also a few other calls that will never need 64-bit time_t because the range is limited by the need to only ever pass relative timeouts (select, poll, io_getevents, recvmmsg, clock_getres, rt_sigtimedwait, sched_rr_get_interval, getrusage, waitid, semtimedop, sysinfo), so we could actually leave them using a 32-bit structure and have the libc do the conversion.
Indeed.
I've started a list of affected syscalls at https://docs.google.com/spreadsheets/d/1HCYwHXxs48TsTb6IGUduNjQnmfRvMPzCN6T_...
Still adding more calls and description, let me know if you want edit permissions.
Only if you have a strong backup system for this file. My GUI foo is rather limited :)
Thanks,
tglx
On Wednesday 22 April 2015 13:07:44 Arnd Bergmann wrote:
I've started a list of affected syscalls at https://docs.google.com/spreadsheets/d/1HCYwHXxs48TsTb6IGUduNjQnmfRvMPzCN6T_...
Still adding more calls and description, let me know if you want edit permissions.
Got a first draft now, I'm relatively sure that the list is complete, but it's not the end of the world if I missed a syscall now.
Here are my findings, and I guess we should discuss these with the libc folks too. I'll group the syscalls according to subsystems:
=== clocks and timers ===
clock_gettime, clock_settime, clock_adjtime, clock_getres, clock_nanosleep, timer_gettime, timer_settime, timerfd_gettime, timerfd_settime:
these should be done consistently, either using timespec64 or 64-bit nanoseconds, either one works. 64-bit nanoseconds would simplify the kernel internally quite a bit by avoiding the double timekeeping (we keep track of both nanoseconds and timespec in the timekeeper struct). the downside of nanoseconds-only is that each existing caller would need a conversion in user space, where currently we can avoid the expensive ktime_to_ts() for some cases.
time, stime, gettimeofday, settimeofday, adjtimex, nanosleep, getitimer, setitimer: all deprecated => wontfix
=== i/o ===
pselect6, ppoll, io_getevents, recvmmsg: These currently pass a timespec into the kernel with *relative* timeouts. Internally, they convert it to ktime_t and back on the way out. We have three options: - leave as is, get the libc to convert 64-bit timespec to 32-bit timespec on the way into the kernel and back on the way out, which works because the relative timeout will not overflow - use ktime_t to make these more efficient in the kernel, at the expense of requiring user space to convert it (all except io_getevents pass back the remaining time). - leave the current behavior, but use 64-bit timespec.
select, old_selct, pselect6: deprecated
=== ipc ===
mq_timedsend, mqtimedreceive: These get an *absolute* timeout, so we have to change them. Internally they use ktime_t, so that would be the natural interface, but timespec64 would work as well.
semtimedop: This uses a relative timeout that is converted to jiffies internally, so using ktime_t would not be as natural, unless we rewrite the function to use hrtimers.
msgctl, semctl, shmctl: These have an output, which is a time_t that stores the absolute seconds value of the last time something happened. Internally this comes from get_seconds(), which has to be efficient anyway. The best way forward is probably to use a structure layout for these that is compatible with what 64-bit architectures do. Note that the structures sometimes have padding to deal with the extension of time_t to 64-bit, but not all architectures have that, and some (notably big-endian arm) have it in the wrong place, so my feeling is that we're better off not using that padding and instead doing something that works for everyone.
=== inodes and filesystems ===
utimesnsat, fstat64, fstatat64:
inode timestamps need to represent times before 1970 and way into the future, so we need 64-bit time_t here, I see no other alternatives here, so we have to pass struct timespec64 into utimensat, and create version 4 of 'struct stat' to pass into the future fstat and fstatat. I would use a version that matches the 64-bit layout of 'struct stat'.
utime, utimes, futimensat, oldstat, oldlstat, oldfstat, newstat, newlstat, newfstat, newfstatat, stat64 and lstat64: these are all deprecated now, we have to stop getting this wrong!
=== tasks ===
getrusage, waitid: these pass a 'struct rusage' that contains a 'struct timeval' with elapsed time. Again there are multiple options: - We could change rusage to contain a new 'struct relative_timeval' instead, with an unchanged layout, which makes the format incompatible with a standard libc that uses a 64-bit based timeval. - We could make the layout the same as on 64-bit machines, as x32 does, which is again incompatible with posix but would work better - We could make the layout what glibc expects, using 64-bit based timeval structures at the beginning. - We could define a new structure usings pure nanosecond counters.
rt_sigtimedwait: This passes a relative timespec value in back out, so we could keep the current layout and have glibc convert it, or change it to something else. The kernel internally converts to jiffies to call schedule_timeout.
futex: this passes a relative *or* absolute timespec in, so we have to change it. The kernel uses ktime_t internally here, so we could make the interface nanosecond based or stick with timespec64.
sched_rr_get_interval: This returns a timespec with the schedule interval to user space, using a 32-bit based format is fine here, or we could convert to timespec64. The kernel uses jiffies internally.
wait4: replaced by waitid
=== system wide ===
sysinfo: struct sysinfo contains '__kernel_long_t uptime', we can keep that, it's fine.
Arnd
On Wed, Apr 22, 2015 at 03:50:45PM +0200, Arnd Bergmann wrote:
time, stime, gettimeofday, settimeofday, adjtimex, nanosleep, getitimer, setitimer: all deprecated => wontfix
If adjtimex is deprecated, what will replace it? It is really important for ntp.
Thanks, Richard
On Wednesday 22 April 2015 16:54:32 Richard Cochran wrote:
On Wed, Apr 22, 2015 at 03:50:45PM +0200, Arnd Bergmann wrote:
time, stime, gettimeofday, settimeofday, adjtimex, nanosleep, getitimer, setitimer: all deprecated => wontfix
If adjtimex is deprecated, what will replace it? It is really important for ntp.
clock_adjtime(). Basically all all the basic time related syscalls are superceded by the clock_* calls that can take a clockid argument.
Arnd
On Wed, Apr 22, 2015 at 03:50:45PM +0200, Arnd Bergmann wrote:
On Wednesday 22 April 2015 13:07:44 Arnd Bergmann wrote:
...
select, old_selct, pselect6: deprecated
Small, copy&paste error here for pselect6.
Luc
On Wednesday 22 April 2015 17:14:47 Luc Van Oostenryck wrote:
On Wed, Apr 22, 2015 at 03:50:45PM +0200, Arnd Bergmann wrote:
On Wednesday 22 April 2015 13:07:44 Arnd Bergmann wrote:
...
select, old_selct, pselect6: deprecated
Small, copy&paste error here for pselect6.
Right, that should have been poll.
Arnd
This patch converts the timepsec type to timespec64 type, and converts the itimerspec type to itimerspec64 type for the k_clock callback functions.
This patch also converts the timespec type to timespec64 type for timekeeping_clocktai() function which is used only in the posix-timers.c file.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- include/linux/timekeeping.h | 4 +- kernel/time/posix-timers.c | 102 +++++++++++++++++++++++-------------------- kernel/time/timekeeping.h | 2 +- 3 files changed, 57 insertions(+), 51 deletions(-)
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h index c6d5ae9..bd3df93 100644 --- a/include/linux/timekeeping.h +++ b/include/linux/timekeeping.h @@ -242,9 +242,9 @@ static inline void get_monotonic_boottime64(struct timespec64 *ts) *ts = ktime_to_timespec64(ktime_get_boottime()); }
-static inline void timekeeping_clocktai(struct timespec *ts) +static inline void timekeeping_clocktai(struct timespec64 *ts) { - *ts = ktime_to_timespec(ktime_get_clocktai()); + *ts = ktime_to_timespec64(ktime_get_clocktai()); }
/* diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c index 9070387..47d1abf 100644 --- a/kernel/time/posix-timers.c +++ b/kernel/time/posix-timers.c @@ -132,9 +132,9 @@ static struct k_clock posix_clocks[MAX_CLOCKS]; static int common_nsleep(const clockid_t, int flags, struct timespec *t, struct timespec __user *rmtp); static int common_timer_create(struct k_itimer *new_timer); -static void common_timer_get(struct k_itimer *, struct itimerspec *); +static void common_timer_get(struct k_itimer *, struct itimerspec64 *); static int common_timer_set(struct k_itimer *, int, - struct itimerspec *, struct itimerspec *); + struct itimerspec64 *, struct itimerspec64 *); static int common_timer_del(struct k_itimer *timer);
static enum hrtimer_restart posix_timer_fn(struct hrtimer *data); @@ -203,17 +203,20 @@ static inline void unlock_timer(struct k_itimer *timr, unsigned long flags) }
/* Get clock_realtime */ -static int posix_clock_realtime_get(clockid_t which_clock, struct timespec *tp) +static int posix_clock_realtime_get(clockid_t which_clock, + struct timespec64 *tp) { - ktime_get_real_ts(tp); + ktime_get_real_ts64(tp); return 0; }
/* Set clock_realtime */ static int posix_clock_realtime_set(const clockid_t which_clock, - const struct timespec *tp) + const struct timespec64 *tp) { - return do_sys_settimeofday(tp, NULL); + struct timespec ts = timespec64_to_timespec(*tp); + + return do_sys_settimeofday(&ts, NULL); }
static int posix_clock_realtime_adj(const clockid_t which_clock, @@ -225,48 +228,51 @@ static int posix_clock_realtime_adj(const clockid_t which_clock, /* * Get monotonic time for posix timers */ -static int posix_ktime_get_ts(clockid_t which_clock, struct timespec *tp) +static int posix_ktime_get_ts(clockid_t which_clock, struct timespec64 *tp) { - ktime_get_ts(tp); + ktime_get_ts64(tp); return 0; }
/* * Get monotonic-raw time for posix timers */ -static int posix_get_monotonic_raw(clockid_t which_clock, struct timespec *tp) +static int posix_get_monotonic_raw(clockid_t which_clock, struct timespec64 *tp) { - getrawmonotonic(tp); + getrawmonotonic64(tp); return 0; }
-static int posix_get_realtime_coarse(clockid_t which_clock, struct timespec *tp) +static int posix_get_realtime_coarse(clockid_t which_clock, + struct timespec64 *tp) { - *tp = current_kernel_time(); + *tp = current_kernel_time64(); return 0; }
static int posix_get_monotonic_coarse(clockid_t which_clock, - struct timespec *tp) + struct timespec64 *tp) { - *tp = get_monotonic_coarse(); + *tp = get_monotonic_coarse64(); return 0; }
-static int posix_get_coarse_res(const clockid_t which_clock, struct timespec *tp) +static int posix_get_coarse_res(const clockid_t which_clock, + struct timespec64 *tp) { - *tp = ktime_to_timespec(KTIME_LOW_RES); + *tp = ktime_to_timespec64(KTIME_LOW_RES); return 0; }
-static int posix_get_boottime(const clockid_t which_clock, struct timespec *tp) +static int posix_get_boottime(const clockid_t which_clock, + struct timespec64 *tp) { - get_monotonic_boottime(tp); + get_monotonic_boottime64(tp); return 0; }
-static int posix_get_tai(clockid_t which_clock, struct timespec *tp) +static int posix_get_tai(clockid_t which_clock, struct timespec64 *tp) { timekeeping_clocktai(tp); return 0; @@ -278,57 +284,57 @@ static int posix_get_tai(clockid_t which_clock, struct timespec *tp) static __init int init_posix_timers(void) { struct k_clock clock_realtime = { - .clock_getres = hrtimer_get_res, - .clock_get = posix_clock_realtime_get, - .clock_set = posix_clock_realtime_set, + .clock_getres64 = hrtimer_get_res64, + .clock_get64 = posix_clock_realtime_get, + .clock_set64 = posix_clock_realtime_set, .clock_adj = posix_clock_realtime_adj, .nsleep = common_nsleep, .nsleep_restart = hrtimer_nanosleep_restart, .timer_create = common_timer_create, - .timer_set = common_timer_set, - .timer_get = common_timer_get, + .timer_set64 = common_timer_set, + .timer_get64 = common_timer_get, .timer_del = common_timer_del, }; struct k_clock clock_monotonic = { - .clock_getres = hrtimer_get_res, - .clock_get = posix_ktime_get_ts, + .clock_getres64 = hrtimer_get_res64, + .clock_get64 = posix_ktime_get_ts, .nsleep = common_nsleep, .nsleep_restart = hrtimer_nanosleep_restart, .timer_create = common_timer_create, - .timer_set = common_timer_set, - .timer_get = common_timer_get, + .timer_set64 = common_timer_set, + .timer_get64 = common_timer_get, .timer_del = common_timer_del, }; struct k_clock clock_monotonic_raw = { - .clock_getres = hrtimer_get_res, - .clock_get = posix_get_monotonic_raw, + .clock_getres64 = hrtimer_get_res64, + .clock_get64 = posix_get_monotonic_raw, }; struct k_clock clock_realtime_coarse = { - .clock_getres = posix_get_coarse_res, - .clock_get = posix_get_realtime_coarse, + .clock_getres64 = posix_get_coarse_res, + .clock_get64 = posix_get_realtime_coarse, }; struct k_clock clock_monotonic_coarse = { - .clock_getres = posix_get_coarse_res, - .clock_get = posix_get_monotonic_coarse, + .clock_getres64 = posix_get_coarse_res, + .clock_get64 = posix_get_monotonic_coarse, }; struct k_clock clock_tai = { - .clock_getres = hrtimer_get_res, - .clock_get = posix_get_tai, + .clock_getres64 = hrtimer_get_res64, + .clock_get64 = posix_get_tai, .nsleep = common_nsleep, .nsleep_restart = hrtimer_nanosleep_restart, .timer_create = common_timer_create, - .timer_set = common_timer_set, - .timer_get = common_timer_get, + .timer_set64 = common_timer_set, + .timer_get64 = common_timer_get, .timer_del = common_timer_del, }; struct k_clock clock_boottime = { - .clock_getres = hrtimer_get_res, - .clock_get = posix_get_boottime, + .clock_getres64 = hrtimer_get_res64, + .clock_get64 = posix_get_boottime, .nsleep = common_nsleep, .nsleep_restart = hrtimer_nanosleep_restart, .timer_create = common_timer_create, - .timer_set = common_timer_set, - .timer_get = common_timer_get, + .timer_set64 = common_timer_set, + .timer_get64 = common_timer_get, .timer_del = common_timer_del, };
@@ -726,7 +732,7 @@ static struct k_itimer *__lock_timer(timer_t timer_id, unsigned long *flags) * report. */ static void -common_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting) +common_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting) { ktime_t now, remaining, iv; struct hrtimer *timer = &timr->it.real.timer; @@ -737,7 +743,7 @@ common_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting)
/* interval timer ? */ if (iv.tv64) - cur_setting->it_interval = ktime_to_timespec(iv); + cur_setting->it_interval = ktime_to_timespec64(iv); else if (!hrtimer_active(timer) && (timr->it_sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE) return; @@ -763,7 +769,7 @@ common_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting) if ((timr->it_sigev_notify & ~SIGEV_THREAD_ID) != SIGEV_NONE) cur_setting->it_value.tv_nsec = 1; } else - cur_setting->it_value = ktime_to_timespec(remaining); + cur_setting->it_value = ktime_to_timespec64(remaining); }
/* Get the time remaining on a POSIX.1b interval timer. */ @@ -830,7 +836,7 @@ SYSCALL_DEFINE1(timer_getoverrun, timer_t, timer_id) /* timr->it_lock is taken. */ static int common_timer_set(struct k_itimer *timr, int flags, - struct itimerspec *new_setting, struct itimerspec *old_setting) + struct itimerspec64 *new_setting, struct itimerspec64 *old_setting) { struct hrtimer *timer = &timr->it.real.timer; enum hrtimer_mode mode; @@ -859,10 +865,10 @@ common_timer_set(struct k_itimer *timr, int flags, hrtimer_init(&timr->it.real.timer, timr->it_clock, mode); timr->it.real.timer.function = posix_timer_fn;
- hrtimer_set_expires(timer, timespec_to_ktime(new_setting->it_value)); + hrtimer_set_expires(timer, timespec64_to_ktime(new_setting->it_value));
/* Convert interval */ - timr->it.real.interval = timespec_to_ktime(new_setting->it_interval); + timr->it.real.interval = timespec64_to_ktime(new_setting->it_interval);
/* SIGEV_NONE timers are not queued ! See common_timer_get */ if (((timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE)) { diff --git a/kernel/time/timekeeping.h b/kernel/time/timekeeping.h index 1d91416..144af14 100644 --- a/kernel/time/timekeeping.h +++ b/kernel/time/timekeeping.h @@ -15,7 +15,7 @@ extern u64 timekeeping_max_deferment(void); extern int timekeeping_inject_offset(struct timespec *ts); extern s32 timekeeping_get_tai_offset(void); extern void timekeeping_set_tai_offset(s32 tai_offset); -extern void timekeeping_clocktai(struct timespec *ts); +extern void timekeeping_clocktai(struct timespec64 *ts); extern int timekeeping_suspend(void); extern void timekeeping_resume(void);
On Mon, 20 Apr 2015, Baolin Wang wrote:
/* Set clock_realtime */ static int posix_clock_realtime_set(const clockid_t which_clock,
const struct timespec *tp)
const struct timespec64 *tp){
- return do_sys_settimeofday(tp, NULL);
- struct timespec ts = timespec64_to_timespec(*tp);
- return do_sys_settimeofday(&ts, NULL);
Sigh. No. We first provide a proper function for this, which takes a timespec64, i.e. do_sys_settimeofday64() instead of having this wrapper mess all over the place.
/* SIGEV_NONE timers are not queued ! See common_timer_get */ if (((timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE)) { diff --git a/kernel/time/timekeeping.h b/kernel/time/timekeeping.h index 1d91416..144af14 100644 --- a/kernel/time/timekeeping.h +++ b/kernel/time/timekeeping.h @@ -15,7 +15,7 @@ extern u64 timekeeping_max_deferment(void); extern int timekeeping_inject_offset(struct timespec *ts); extern s32 timekeeping_get_tai_offset(void); extern void timekeeping_set_tai_offset(s32 tai_offset); -extern void timekeeping_clocktai(struct timespec *ts); +extern void timekeeping_clocktai(struct timespec64 *ts);
# git grep timekeeping_clocktai() is your friend.
Thanks,
tglx
On 21 April 2015 at 04:48, Thomas Gleixner tglx@linutronix.de wrote:
On Mon, 20 Apr 2015, Baolin Wang wrote:
/* Set clock_realtime */ static int posix_clock_realtime_set(const clockid_t which_clock,
const struct timespec *tp)
const struct timespec64 *tp){
return do_sys_settimeofday(tp, NULL);
struct timespec ts = timespec64_to_timespec(*tp);return do_sys_settimeofday(&ts, NULL);Sigh. No. We first provide a proper function for this, which takes a timespec64, i.e. do_sys_settimeofday64() instead of having this wrapper mess all over the place.
Thanks for your comments,but if use do_sys_settimeofday64() here that will introduce a security bug: do_sys_settimeofday contains a capability check that normally prevents non-root users from setting the time.
With your change, any user can set the system time.
/* SIGEV_NONE timers are not queued ! See common_timer_get */ if (((timr->it_sigev_notify & ~SIGEV_THREAD_ID) == SIGEV_NONE)) {diff --git a/kernel/time/timekeeping.h b/kernel/time/timekeeping.h index 1d91416..144af14 100644 --- a/kernel/time/timekeeping.h +++ b/kernel/time/timekeeping.h @@ -15,7 +15,7 @@ extern u64 timekeeping_max_deferment(void); extern int timekeeping_inject_offset(struct timespec *ts); extern s32 timekeeping_get_tai_offset(void); extern void timekeeping_set_tai_offset(s32 tai_offset); -extern void timekeeping_clocktai(struct timespec *ts); +extern void timekeeping_clocktai(struct timespec64 *ts);
# git grep timekeeping_clocktai() is your friend.
Thanks,
tglx
On Tuesday 21 April 2015 16:36:13 Baolin Wang wrote:
On 21 April 2015 at 04:48, Thomas Gleixner tglx@linutronix.de wrote:
On Mon, 20 Apr 2015, Baolin Wang wrote:
/* Set clock_realtime */ static int posix_clock_realtime_set(const clockid_t which_clock,
const struct timespec *tp)
const struct timespec64 *tp){
return do_sys_settimeofday(tp, NULL);
struct timespec ts = timespec64_to_timespec(*tp);return do_sys_settimeofday(&ts, NULL);Sigh. No. We first provide a proper function for this, which takes a timespec64, i.e. do_sys_settimeofday64() instead of having this wrapper mess all over the place.
Thanks for your comments,but if use do_sys_settimeofday64() here that will introduce a security bug: do_sys_settimeofday contains a capability check that normally prevents non-root users from setting the time.
With your change, any user can set the system time.
He was asking for a new do_sys_settimeofday64 function to be added, not using the low-level do_settimeofday64.
Arnd
On 21 April 2015 at 16:45, Arnd Bergmann arnd@arndb.de wrote:
On Tuesday 21 April 2015 16:36:13 Baolin Wang wrote:
On 21 April 2015 at 04:48, Thomas Gleixner tglx@linutronix.de wrote:
On Mon, 20 Apr 2015, Baolin Wang wrote:
/* Set clock_realtime */ static int posix_clock_realtime_set(const clockid_t which_clock,
const struct timespec *tp)
const struct timespec64 *tp){
return do_sys_settimeofday(tp, NULL);
struct timespec ts = timespec64_to_timespec(*tp);return do_sys_settimeofday(&ts, NULL);Sigh. No. We first provide a proper function for this, which takes a timespec64, i.e. do_sys_settimeofday64() instead of having this wrapper mess all over the place.
Thanks for your comments,but if use do_sys_settimeofday64() here that will introduce a security bug: do_sys_settimeofday contains a capability check that normally prevents non-root users from setting the time.
With your change, any user can set the system time.
He was asking for a new do_sys_settimeofday64 function to be added, not using the low-level do_settimeofday64.
Arnd
Sorry for the misunderstand, i'll fix that in next patch. Thanks.
This patch converts to the 64bit methods for k_clock callback function, that converts the timespec type to timespec64 type and converts the itimerspec type to itimerspec64 type.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- drivers/char/mmtimer.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/drivers/char/mmtimer.c b/drivers/char/mmtimer.c index 3d6c067..213d0bb 100644 --- a/drivers/char/mmtimer.c +++ b/drivers/char/mmtimer.c @@ -478,18 +478,18 @@ static int sgi_clock_period; static struct timespec sgi_clock_offset; static int sgi_clock_period;
-static int sgi_clock_get(clockid_t clockid, struct timespec *tp) +static int sgi_clock_get(clockid_t clockid, struct timespec64 *tp) { u64 nsec;
nsec = rtc_time() * sgi_clock_period + sgi_clock_offset.tv_nsec; - *tp = ns_to_timespec(nsec); + *tp = ns_to_timespec64(nsec); tp->tv_sec += sgi_clock_offset.tv_sec; return 0; };
-static int sgi_clock_set(const clockid_t clockid, const struct timespec *tp) +static int sgi_clock_set(const clockid_t clockid, const struct timespec64 *tp) {
u64 nsec; @@ -657,7 +657,7 @@ static int sgi_timer_del(struct k_itimer *timr) }
/* Assumption: it_lock is already held with irq's disabled */ -static void sgi_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting) +static void sgi_timer_get(struct k_itimer *timr, struct itimerspec64 *cur_setting) {
if (timr->it.mmtimer.clock == TIMER_OFF) { @@ -668,14 +668,14 @@ static void sgi_timer_get(struct k_itimer *timr, struct itimerspec *cur_setting) return; }
- cur_setting->it_interval = ns_to_timespec(timr->it.mmtimer.incr * sgi_clock_period); - cur_setting->it_value = ns_to_timespec((timr->it.mmtimer.expires - rtc_time()) * sgi_clock_period); + cur_setting->it_interval = ns_to_timespec64(timr->it.mmtimer.incr * sgi_clock_period); + cur_setting->it_value = ns_to_timespec64((timr->it.mmtimer.expires - rtc_time()) * sgi_clock_period); }
static int sgi_timer_set(struct k_itimer *timr, int flags, - struct itimerspec * new_setting, - struct itimerspec * old_setting) + struct itimerspec64 *new_setting, + struct itimerspec64 *old_setting) { unsigned long when, period, irqflags; int err = 0; @@ -687,8 +687,8 @@ static int sgi_timer_set(struct k_itimer *timr, int flags, sgi_timer_get(timr, old_setting);
sgi_timer_del(timr); - when = timespec_to_ns(&new_setting->it_value); - period = timespec_to_ns(&new_setting->it_interval); + when = timespec64_to_ns(&new_setting->it_value); + period = timespec64_to_ns(&new_setting->it_interval);
if (when == 0) /* Clear timer */ @@ -699,11 +699,9 @@ static int sgi_timer_set(struct k_itimer *timr, int flags, return -ENOMEM;
if (flags & TIMER_ABSTIME) { - struct timespec n; unsigned long now;
- getnstimeofday(&n); - now = timespec_to_ns(&n); + now = ktime_get_real_ns(); if (when > now) when -= now; else @@ -765,7 +763,7 @@ static int sgi_timer_set(struct k_itimer *timr, int flags, return err; }
-static int sgi_clock_getres(const clockid_t which_clock, struct timespec *tp) +static int sgi_clock_getres(const clockid_t which_clock, struct timespec64 *tp) { tp->tv_sec = 0; tp->tv_nsec = sgi_clock_period; @@ -773,13 +771,13 @@ static int sgi_clock_getres(const clockid_t which_clock, struct timespec *tp) }
static struct k_clock sgi_clock = { - .clock_set = sgi_clock_set, - .clock_get = sgi_clock_get, - .clock_getres = sgi_clock_getres, + .clock_set64 = sgi_clock_set, + .clock_get64 = sgi_clock_get, + .clock_getres64 = sgi_clock_getres, .timer_create = sgi_timer_create, - .timer_set = sgi_timer_set, + .timer_set64 = sgi_timer_set, .timer_del = sgi_timer_del, - .timer_get = sgi_timer_get + .timer_get64 = sgi_timer_get };
/**
This patch changes to the new methods with timespec64/itimerspec64 type of k_clock structure, and converts the timespec/itimerspec type to timespec64/itimerspec64 typein alarmtimer.c file.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- kernel/time/alarmtimer.c | 43 ++++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 21 deletions(-)
diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index 1b001ed..68186e1 100644 --- a/kernel/time/alarmtimer.c +++ b/kernel/time/alarmtimer.c @@ -489,35 +489,36 @@ static enum alarmtimer_restart alarm_handle_timer(struct alarm *alarm, /** * alarm_clock_getres - posix getres interface * @which_clock: clockid - * @tp: timespec to fill + * @tp: timespec64 to fill * * Returns the granularity of underlying alarm base clock */ -static int alarm_clock_getres(const clockid_t which_clock, struct timespec *tp) +static int alarm_clock_getres(const clockid_t which_clock, + struct timespec64 *tp) { clockid_t baseid = alarm_bases[clock2alarm(which_clock)].base_clockid;
if (!alarmtimer_get_rtcdev()) return -EINVAL;
- return hrtimer_get_res(baseid, tp); + return hrtimer_get_res64(baseid, tp); }
/** * alarm_clock_get - posix clock_get interface * @which_clock: clockid - * @tp: timespec to fill. + * @tp: timespec64 to fill. * * Provides the underlying alarm base time. */ -static int alarm_clock_get(clockid_t which_clock, struct timespec *tp) +static int alarm_clock_get(clockid_t which_clock, struct timespec64 *tp) { struct alarm_base *base = &alarm_bases[clock2alarm(which_clock)];
if (!alarmtimer_get_rtcdev()) return -EINVAL;
- *tp = ktime_to_timespec(base->gettime()); + *tp = ktime_to_timespec64(base->gettime()); return 0; }
@@ -547,24 +548,24 @@ static int alarm_timer_create(struct k_itimer *new_timer) /** * alarm_timer_get - posix timer_get interface * @new_timer: k_itimer pointer - * @cur_setting: itimerspec data to fill + * @cur_setting: itimerspec64 data to fill * * Copies out the current itimerspec data */ static void alarm_timer_get(struct k_itimer *timr, - struct itimerspec *cur_setting) + struct itimerspec64 *cur_setting) { ktime_t relative_expiry_time = alarm_expires_remaining(&(timr->it.alarm.alarmtimer));
if (ktime_to_ns(relative_expiry_time) > 0) { - cur_setting->it_value = ktime_to_timespec(relative_expiry_time); + cur_setting->it_value = ktime_to_timespec64(relative_expiry_time); } else { cur_setting->it_value.tv_sec = 0; cur_setting->it_value.tv_nsec = 0; }
- cur_setting->it_interval = ktime_to_timespec(timr->it.alarm.interval); + cur_setting->it_interval = ktime_to_timespec64(timr->it.alarm.interval); }
/** @@ -588,14 +589,14 @@ static int alarm_timer_del(struct k_itimer *timr) * alarm_timer_set - posix timer_set interface * @timr: k_itimer pointer to be deleted * @flags: timer flags - * @new_setting: itimerspec to be used - * @old_setting: itimerspec being replaced + * @new_setting: itimerspec64 to be used + * @old_setting: itimerspec64 being replaced * * Sets the timer to new_setting, and starts the timer. */ static int alarm_timer_set(struct k_itimer *timr, int flags, - struct itimerspec *new_setting, - struct itimerspec *old_setting) + struct itimerspec64 *new_setting, + struct itimerspec64 *old_setting) { ktime_t exp;
@@ -613,8 +614,8 @@ static int alarm_timer_set(struct k_itimer *timr, int flags, return TIMER_RETRY;
/* start the timer */ - timr->it.alarm.interval = timespec_to_ktime(new_setting->it_interval); - exp = timespec_to_ktime(new_setting->it_value); + timr->it.alarm.interval = timespec64_to_ktime(new_setting->it_interval); + exp = timespec64_to_ktime(new_setting->it_value); /* Convert (if necessary) to absolute time */ if (flags != TIMER_ABSTIME) { ktime_t now; @@ -670,7 +671,7 @@ static int alarmtimer_do_nsleep(struct alarm *alarm, ktime_t absexp)
/** - * update_rmtp - Update remaining timespec value + * update_rmtp - Update remaining timespec64 value * @exp: expiration time * @type: timer type * @rmtp: user pointer to remaining timepsec value @@ -824,12 +825,12 @@ static int __init alarmtimer_init(void) int error = 0; int i; struct k_clock alarm_clock = { - .clock_getres = alarm_clock_getres, - .clock_get = alarm_clock_get, + .clock_getres64 = alarm_clock_getres, + .clock_get64 = alarm_clock_get, .timer_create = alarm_timer_create, - .timer_set = alarm_timer_set, + .timer_set64 = alarm_timer_set, .timer_del = alarm_timer_del, - .timer_get = alarm_timer_get, + .timer_get64 = alarm_timer_get, .nsleep = alarm_timer_nsleep, };
This patch converts the posix clock operations over to the new methods with timespec64/itimerspec64 type to making them ready for 2038, and it is based on the ptp patch series.
And also changes to the 64bit methods for k_clock structure, that converts the timespec/itimerspec type to timespec64/itimerspec64 type.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- drivers/ptp/ptp_clock.c | 26 ++++++++------------------ include/linux/posix-clock.h | 10 +++++----- kernel/time/posix-clock.c | 20 ++++++++++---------- 3 files changed, 23 insertions(+), 33 deletions(-)
diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index bee8270..8c086e7 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -97,32 +97,24 @@ static s32 scaled_ppm_to_ppb(long ppm)
/* posix clock implementation */
-static int ptp_clock_getres(struct posix_clock *pc, struct timespec *tp) +static int ptp_clock_getres(struct posix_clock *pc, struct timespec64 *tp) { tp->tv_sec = 0; tp->tv_nsec = 1; return 0; }
-static int ptp_clock_settime(struct posix_clock *pc, const struct timespec *tp) +static int ptp_clock_settime(struct posix_clock *pc, + const struct timespec64 *tp) { struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock); - struct timespec64 ts = timespec_to_timespec64(*tp); - - return ptp->info->settime64(ptp->info, &ts); + return ptp->info->settime64(ptp->info, tp); }
-static int ptp_clock_gettime(struct posix_clock *pc, struct timespec *tp) +static int ptp_clock_gettime(struct posix_clock *pc, struct timespec64 *tp) { struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock); - struct timespec64 ts; - int err; - - err = ptp->info->gettime64(ptp->info, &ts); - if (!err) - *tp = timespec64_to_timespec(ts); - - return err; + return ptp->info->gettime64(ptp->info, tp); }
static int ptp_clock_adjtime(struct posix_clock *pc, struct timex *tx) @@ -134,8 +126,7 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct timex *tx) ops = ptp->info;
if (tx->modes & ADJ_SETOFFSET) { - struct timespec ts; - ktime_t kt; + struct timespec64 ts; s64 delta;
ts.tv_sec = tx->time.tv_sec; @@ -147,8 +138,7 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct timex *tx) if ((unsigned long) ts.tv_nsec >= NSEC_PER_SEC) return -EINVAL;
- kt = timespec_to_ktime(ts); - delta = ktime_to_ns(kt); + delta = timespec64_to_ns(&ts); err = ops->adjtime(ops, delta); } else if (tx->modes & ADJ_FREQUENCY) { s32 ppb = scaled_ppm_to_ppb(tx->freq); diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h index 34c4498..fd7e22c 100644 --- a/include/linux/posix-clock.h +++ b/include/linux/posix-clock.h @@ -59,23 +59,23 @@ struct posix_clock_operations {
int (*clock_adjtime)(struct posix_clock *pc, struct timex *tx);
- int (*clock_gettime)(struct posix_clock *pc, struct timespec *ts); + int (*clock_gettime)(struct posix_clock *pc, struct timespec64 *ts);
- int (*clock_getres) (struct posix_clock *pc, struct timespec *ts); + int (*clock_getres)(struct posix_clock *pc, struct timespec64 *ts);
int (*clock_settime)(struct posix_clock *pc, - const struct timespec *ts); + const struct timespec64 *ts);
int (*timer_create) (struct posix_clock *pc, struct k_itimer *kit);
int (*timer_delete) (struct posix_clock *pc, struct k_itimer *kit);
void (*timer_gettime)(struct posix_clock *pc, - struct k_itimer *kit, struct itimerspec *tsp); + struct k_itimer *kit, struct itimerspec64 *tsp);
int (*timer_settime)(struct posix_clock *pc, struct k_itimer *kit, int flags, - struct itimerspec *tsp, struct itimerspec *old); + struct itimerspec64 *tsp, struct itimerspec64 *old); /* * Optional character device methods: */ diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c index ce033c7..e21e4c1 100644 --- a/kernel/time/posix-clock.c +++ b/kernel/time/posix-clock.c @@ -297,7 +297,7 @@ out: return err; }
-static int pc_clock_gettime(clockid_t id, struct timespec *ts) +static int pc_clock_gettime(clockid_t id, struct timespec64 *ts) { struct posix_clock_desc cd; int err; @@ -316,7 +316,7 @@ static int pc_clock_gettime(clockid_t id, struct timespec *ts) return err; }
-static int pc_clock_getres(clockid_t id, struct timespec *ts) +static int pc_clock_getres(clockid_t id, struct timespec64 *ts) { struct posix_clock_desc cd; int err; @@ -335,7 +335,7 @@ static int pc_clock_getres(clockid_t id, struct timespec *ts) return err; }
-static int pc_clock_settime(clockid_t id, const struct timespec *ts) +static int pc_clock_settime(clockid_t id, const struct timespec64 *ts) { struct posix_clock_desc cd; int err; @@ -399,7 +399,7 @@ static int pc_timer_delete(struct k_itimer *kit) return err; }
-static void pc_timer_gettime(struct k_itimer *kit, struct itimerspec *ts) +static void pc_timer_gettime(struct k_itimer *kit, struct itimerspec64 *ts) { clockid_t id = kit->it_clock; struct posix_clock_desc cd; @@ -414,7 +414,7 @@ static void pc_timer_gettime(struct k_itimer *kit, struct itimerspec *ts) }
static int pc_timer_settime(struct k_itimer *kit, int flags, - struct itimerspec *ts, struct itimerspec *old) + struct itimerspec64 *ts, struct itimerspec64 *old) { clockid_t id = kit->it_clock; struct posix_clock_desc cd; @@ -435,12 +435,12 @@ static int pc_timer_settime(struct k_itimer *kit, int flags, }
struct k_clock clock_posix_dynamic = { - .clock_getres = pc_clock_getres, - .clock_set = pc_clock_settime, - .clock_get = pc_clock_gettime, + .clock_getres64 = pc_clock_getres, + .clock_set64 = pc_clock_settime, + .clock_get64 = pc_clock_gettime, .clock_adj = pc_clock_adjtime, .timer_create = pc_timer_create, - .timer_set = pc_timer_settime, + .timer_set64 = pc_timer_settime, .timer_del = pc_timer_delete, - .timer_get = pc_timer_gettime, + .timer_get64 = pc_timer_gettime, };
This patch introduces some functions for converting cputime to timespec64 and back, that repalce the timespec type with timespec64 type, as well as for arch/s390 and arch/powerpc architecture.
And these new methods will replace the old cputime_to_timespec/timespec_to_cputime function to ready for 2038 issue. The cputime_to_timespec/timespec_to_cputime functions are moved to include/linux/cputime.h file for removing conveniently.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- arch/powerpc/include/asm/cputime.h | 6 +++--- arch/s390/include/asm/cputime.h | 8 ++++---- include/asm-generic/cputime_jiffies.h | 10 +++++----- include/linux/cputime.h | 15 +++++++++++++++ include/linux/jiffies.h | 3 +++ kernel/time/time.c | 21 +++++++++++++++++++++ 6 files changed, 51 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/include/asm/cputime.h b/arch/powerpc/include/asm/cputime.h index e245255..5dda5c0 100644 --- a/arch/powerpc/include/asm/cputime.h +++ b/arch/powerpc/include/asm/cputime.h @@ -154,9 +154,9 @@ static inline cputime_t secs_to_cputime(const unsigned long sec) }
/* - * Convert cputime <-> timespec + * Convert cputime <-> timespec64 */ -static inline void cputime_to_timespec(const cputime_t ct, struct timespec *p) +static inline void cputime_to_timespec64(const cputime_t ct, struct timespec64 *p) { u64 x = (__force u64) ct; unsigned int frac; @@ -168,7 +168,7 @@ static inline void cputime_to_timespec(const cputime_t ct, struct timespec *p) p->tv_nsec = x; }
-static inline cputime_t timespec_to_cputime(const struct timespec *p) +static inline cputime_t timespec64_to_cputime(const struct timespec64 *p) { u64 ct;
diff --git a/arch/s390/include/asm/cputime.h b/arch/s390/include/asm/cputime.h index b91e960..1266697 100644 --- a/arch/s390/include/asm/cputime.h +++ b/arch/s390/include/asm/cputime.h @@ -89,16 +89,16 @@ static inline cputime_t secs_to_cputime(const unsigned int s) }
/* - * Convert cputime to timespec and back. + * Convert cputime to timespec64 and back. */ -static inline cputime_t timespec_to_cputime(const struct timespec *value) +static inline cputime_t timespec64_to_cputime(const struct timespec64 *value) { unsigned long long ret = value->tv_sec * CPUTIME_PER_SEC; return (__force cputime_t)(ret + __div(value->tv_nsec * CPUTIME_PER_USEC, NSEC_PER_USEC)); }
-static inline void cputime_to_timespec(const cputime_t cputime, - struct timespec *value) +static inline void cputime_to_timespec64(const cputime_t cputime, + struct timespec64 *value) { unsigned long long __cputime = (__force unsigned long long) cputime; #ifndef CONFIG_64BIT diff --git a/include/asm-generic/cputime_jiffies.h b/include/asm-generic/cputime_jiffies.h index fe386fc..ec77c0b 100644 --- a/include/asm-generic/cputime_jiffies.h +++ b/include/asm-generic/cputime_jiffies.h @@ -44,12 +44,12 @@ typedef u64 __nocast cputime64_t; #define secs_to_cputime(sec) jiffies_to_cputime((sec) * HZ)
/* - * Convert cputime to timespec and back. + * Convert cputime to timespec64 and abck. */ -#define timespec_to_cputime(__val) \ - jiffies_to_cputime(timespec_to_jiffies(__val)) -#define cputime_to_timespec(__ct,__val) \ - jiffies_to_timespec(cputime_to_jiffies(__ct),__val) +#define timespec64_to_cputime(__val) \ + jiffies_to_cputime(timespec64_to_jiffies(__val)) +#define cputime_to_timespec64(__ct,__val) \ + jiffies_to_timespec64(cputime_to_jiffies(__ct),__val)
/* * Convert cputime to timeval and back. diff --git a/include/linux/cputime.h b/include/linux/cputime.h index f2eb2ee..f01896f 100644 --- a/include/linux/cputime.h +++ b/include/linux/cputime.h @@ -13,4 +13,19 @@ usecs_to_cputime((__nsecs) / NSEC_PER_USEC) #endif
+static inline cputime_t timespec_to_cputime(const struct timespec *ts) +{ + struct timespec64 ts64 = timespec_to_timespec64(*ts); + return timespec64_to_cputime(&ts64); +} + +static inline void cputime_to_timespec(const cputime_t cputime, + struct timespec *value) +{ + struct timespec64 *ts64; + + *ts64 = timespec_to_timespec64(*value); + cputime_to_timespec64(cputime, ts64); +} + #endif /* __LINUX_CPUTIME_H */ diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h index c367cbd..dbaa4ee 100644 --- a/include/linux/jiffies.h +++ b/include/linux/jiffies.h @@ -293,6 +293,9 @@ extern unsigned long usecs_to_jiffies(const unsigned int u); extern unsigned long timespec_to_jiffies(const struct timespec *value); extern void jiffies_to_timespec(const unsigned long jiffies, struct timespec *value); +extern unsigned long timespec64_to_jiffies(const struct timespec64 *value); +extern void jiffies_to_timespec64(const unsigned long jiffies, + struct timespec64 *value); extern unsigned long timeval_to_jiffies(const struct timeval *value); extern void jiffies_to_timeval(const unsigned long jiffies, struct timeval *value); diff --git a/kernel/time/time.c b/kernel/time/time.c index 2c85b77..d5978c5 100644 --- a/kernel/time/time.c +++ b/kernel/time/time.c @@ -606,6 +606,27 @@ jiffies_to_timespec(const unsigned long jiffies, struct timespec *value) } EXPORT_SYMBOL(jiffies_to_timespec);
+unsigned long +timespec64_to_jiffies(const struct timespec64 *value) +{ + return __timespec_to_jiffies(value->tv_sec, value->tv_nsec); +} +EXPORT_SYMBOL(timespec64_to_jiffies); + +void +jiffies_to_timespec64(const unsigned long jiffies, struct timespec64 *value) +{ + /* + * Convert jiffies to nanoseconds and separate with + * one divide. + */ + u32 rem; + value->tv_sec = div_u64_rem((u64)jiffies * TICK_NSEC, + NSEC_PER_SEC, &rem); + value->tv_nsec = rem; +} +EXPORT_SYMBOL(jiffies_to_timespec64); + /* * We could use a similar algorithm to timespec_to_jiffies (with a * different multiplier for usec instead of nsec). But this has a
On Mon, 20 Apr 2015, Baolin Wang wrote:
This patch introduces some functions for converting cputime to timespec64 and back, that repalce the timespec type with timespec64 type, as well as for arch/s390 and arch/powerpc architecture.
No. We want a patch which adds the functions and then a patch which uses them. This stuff is tricky and hard to review. So please split the patches into smaller chunks.
+unsigned long +timespec64_to_jiffies(const struct timespec64 *value) +{
- return __timespec_to_jiffies(value->tv_sec, value->tv_nsec);
+} +EXPORT_SYMBOL(timespec64_to_jiffies);
So we have now two exports which are doing exactly the same thing. Copy and paste is wonderful, right?
What about exporting __timespec_to_jiffies() and providing inlines for timespec_to_jiffies() and timespec64_to_jiffies() ?
EXPORT_SYMBOL is not just a stupid annotation. Its impact on the resulting kernel size is larger than the actual function implementation.
+void +jiffies_to_timespec64(const unsigned long jiffies, struct timespec64 *value) +{
- /*
* Convert jiffies to nanoseconds and separate with* one divide.*/- u32 rem;
- value->tv_sec = div_u64_rem((u64)jiffies * TICK_NSEC,
NSEC_PER_SEC, &rem);- value->tv_nsec = rem;
+} +EXPORT_SYMBOL(jiffies_to_timespec64);
Sigh.
Thanks,
tglx
This patch changes to the new methods of k_clock structure with timespec64 type, converts the timespec/itimerspec type to timespec64/itimerspec64 type for the callback function in posix-cpu-timers.c file.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- kernel/time/posix-cpu-timers.c | 83 +++++++++++++++++++++------------------- 1 file changed, 44 insertions(+), 39 deletions(-)
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c index 0075da7..51cfead 100644 --- a/kernel/time/posix-cpu-timers.c +++ b/kernel/time/posix-cpu-timers.c @@ -52,7 +52,7 @@ static int check_clock(const clockid_t which_clock) }
static inline unsigned long long -timespec_to_sample(const clockid_t which_clock, const struct timespec *tp) +timespec64_to_sample(const clockid_t which_clock, const struct timespec64 *tp) { unsigned long long ret;
@@ -60,19 +60,19 @@ timespec_to_sample(const clockid_t which_clock, const struct timespec *tp) if (CPUCLOCK_WHICH(which_clock) == CPUCLOCK_SCHED) { ret = (unsigned long long)tp->tv_sec * NSEC_PER_SEC + tp->tv_nsec; } else { - ret = cputime_to_expires(timespec_to_cputime(tp)); + ret = cputime_to_expires(timespec64_to_cputime(tp)); } return ret; }
-static void sample_to_timespec(const clockid_t which_clock, +static void sample_to_timespec64(const clockid_t which_clock, unsigned long long expires, - struct timespec *tp) + struct timespec64 *tp) { if (CPUCLOCK_WHICH(which_clock) == CPUCLOCK_SCHED) - *tp = ns_to_timespec(expires); + *tp = ns_to_timespec64(expires); else - cputime_to_timespec((__force cputime_t)expires, tp); + cputime_to_timespec64((__force cputime_t)expires, tp); }
/* @@ -141,7 +141,7 @@ static inline unsigned long long virt_ticks(struct task_struct *p) }
static int -posix_cpu_clock_getres(const clockid_t which_clock, struct timespec *tp) +posix_cpu_clock_getres(const clockid_t which_clock, struct timespec64 *tp) { int error = check_clock(which_clock); if (!error) { @@ -160,7 +160,7 @@ posix_cpu_clock_getres(const clockid_t which_clock, struct timespec *tp) }
static int -posix_cpu_clock_set(const clockid_t which_clock, const struct timespec *tp) +posix_cpu_clock_set(const clockid_t which_clock, const struct timespec64 *tp) { /* * You can never reset a CPU clock, but we check for other errors @@ -263,7 +263,7 @@ static int cpu_clock_sample_group(const clockid_t which_clock,
static int posix_cpu_clock_get_task(struct task_struct *tsk, const clockid_t which_clock, - struct timespec *tp) + struct timespec64 *tp) { int err = -EINVAL; unsigned long long rtn; @@ -277,13 +277,14 @@ static int posix_cpu_clock_get_task(struct task_struct *tsk, }
if (!err) - sample_to_timespec(which_clock, rtn, tp); + sample_to_timespec64(which_clock, rtn, tp);
return err; }
-static int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp) +static int posix_cpu_clock_get(const clockid_t which_clock, + struct timespec64 *tp) { const pid_t pid = CPUCLOCK_PID(which_clock); int err = -EINVAL; @@ -598,7 +599,7 @@ static inline void posix_cpu_timer_kick_nohz(void) { } * and try again. (This happens when the timer is in the middle of firing.) */ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags, - struct itimerspec *new, struct itimerspec *old) + struct itimerspec64 *new, struct itimerspec64 *old) { unsigned long flags; struct sighand_struct *sighand; @@ -608,7 +609,7 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags,
WARN_ON_ONCE(p == NULL);
- new_expires = timespec_to_sample(timer->it_clock, &new->it_value); + new_expires = timespec64_to_sample(timer->it_clock, &new->it_value);
/* * Protect against sighand release/switch in exit/exec and p->cpu_timers @@ -669,7 +670,7 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags, bump_cpu_timer(timer, val); if (val < timer->it.cpu.expires) { old_expires = timer->it.cpu.expires - val; - sample_to_timespec(timer->it_clock, + sample_to_timespec64(timer->it_clock, old_expires, &old->it_value); } else { @@ -709,7 +710,7 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags, * Install the new reload setting, and * set up the signal and overrun bookkeeping. */ - timer->it.cpu.incr = timespec_to_sample(timer->it_clock, + timer->it.cpu.incr = timespec64_to_sample(timer->it_clock, &new->it_interval);
/* @@ -734,7 +735,7 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags, ret = 0; out: if (old) { - sample_to_timespec(timer->it_clock, + sample_to_timespec64(timer->it_clock, old_incr, &old->it_interval); } if (!ret) @@ -742,7 +743,7 @@ static int posix_cpu_timer_set(struct k_itimer *timer, int timer_flags, return ret; }
-static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec *itp) +static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec64 *itp) { unsigned long long now; struct task_struct *p = timer->it.cpu.task; @@ -752,7 +753,7 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec *itp) /* * Easy part: convert the reload time. */ - sample_to_timespec(timer->it_clock, + sample_to_timespec64(timer->it_clock, timer->it.cpu.incr, &itp->it_interval);
if (timer->it.cpu.expires == 0) { /* Timer not armed at all. */ @@ -782,7 +783,7 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec *itp) * Call the timer disarmed, nothing else to do. */ timer->it.cpu.expires = 0; - sample_to_timespec(timer->it_clock, timer->it.cpu.expires, + sample_to_timespec64(timer->it_clock, timer->it.cpu.expires, &itp->it_value); } else { cpu_timer_sample_group(timer->it_clock, p, &now); @@ -791,7 +792,7 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec *itp) }
if (now < timer->it.cpu.expires) { - sample_to_timespec(timer->it_clock, + sample_to_timespec64(timer->it_clock, timer->it.cpu.expires - now, &itp->it_value); } else { @@ -1248,6 +1249,8 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags, struct timespec *rqtp, struct itimerspec *it) { struct k_itimer timer; + struct timespec64 ts64; + struct itimerspec64 *it64; int error;
/* @@ -1260,13 +1263,14 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags, error = posix_cpu_timer_create(&timer); timer.it_process = current; if (!error) { - static struct itimerspec zero_it; + static struct itimerspec64 zero_it;
memset(it, 0, sizeof *it); it->it_value = *rqtp;
spin_lock_irq(&timer.it_lock); - error = posix_cpu_timer_set(&timer, flags, it, NULL); + *it64 = itimerspec_to_itimerspec64(*it); + error = posix_cpu_timer_set(&timer, flags, it64, NULL); if (error) { spin_unlock_irq(&timer.it_lock); return error; @@ -1295,8 +1299,9 @@ static int do_cpu_nanosleep(const clockid_t which_clock, int flags, /* * We were interrupted by a signal. */ - sample_to_timespec(which_clock, timer.it.cpu.expires, rqtp); - error = posix_cpu_timer_set(&timer, 0, &zero_it, it); + sample_to_timespec64(which_clock, timer.it.cpu.expires, &ts64); + *rqtp = timespec64_to_timespec(ts64); + error = posix_cpu_timer_set(&timer, 0, &zero_it, it64); if (!error) { /* * Timer is now unarmed, deletion can not fail. @@ -1395,12 +1400,12 @@ static long posix_cpu_nsleep_restart(struct restart_block *restart_block) #define THREAD_CLOCK MAKE_THREAD_CPUCLOCK(0, CPUCLOCK_SCHED)
static int process_cpu_clock_getres(const clockid_t which_clock, - struct timespec *tp) + struct timespec64 *tp) { return posix_cpu_clock_getres(PROCESS_CLOCK, tp); } static int process_cpu_clock_get(const clockid_t which_clock, - struct timespec *tp) + struct timespec64 *tp) { return posix_cpu_clock_get(PROCESS_CLOCK, tp); } @@ -1420,12 +1425,12 @@ static long process_cpu_nsleep_restart(struct restart_block *restart_block) return -EINVAL; } static int thread_cpu_clock_getres(const clockid_t which_clock, - struct timespec *tp) + struct timespec64 *tp) { return posix_cpu_clock_getres(THREAD_CLOCK, tp); } static int thread_cpu_clock_get(const clockid_t which_clock, - struct timespec *tp) + struct timespec64 *tp) { return posix_cpu_clock_get(THREAD_CLOCK, tp); } @@ -1436,37 +1441,37 @@ static int thread_cpu_timer_create(struct k_itimer *timer) }
struct k_clock clock_posix_cpu = { - .clock_getres = posix_cpu_clock_getres, - .clock_set = posix_cpu_clock_set, - .clock_get = posix_cpu_clock_get, + .clock_getres64 = posix_cpu_clock_getres, + .clock_set64 = posix_cpu_clock_set, + .clock_get64 = posix_cpu_clock_get, .timer_create = posix_cpu_timer_create, .nsleep = posix_cpu_nsleep, .nsleep_restart = posix_cpu_nsleep_restart, - .timer_set = posix_cpu_timer_set, + .timer_set64 = posix_cpu_timer_set, .timer_del = posix_cpu_timer_del, - .timer_get = posix_cpu_timer_get, + .timer_get64 = posix_cpu_timer_get, };
static __init int init_posix_cpu_timers(void) { struct k_clock process = { - .clock_getres = process_cpu_clock_getres, - .clock_get = process_cpu_clock_get, + .clock_getres64 = process_cpu_clock_getres, + .clock_get64 = process_cpu_clock_get, .timer_create = process_cpu_timer_create, .nsleep = process_cpu_nsleep, .nsleep_restart = process_cpu_nsleep_restart, }; struct k_clock thread = { - .clock_getres = thread_cpu_clock_getres, - .clock_get = thread_cpu_clock_get, + .clock_getres64 = thread_cpu_clock_getres, + .clock_get64 = thread_cpu_clock_get, .timer_create = thread_cpu_timer_create, }; - struct timespec ts; + struct timespec64 ts;
posix_timers_register_clock(CLOCK_PROCESS_CPUTIME_ID, &process); posix_timers_register_clock(CLOCK_THREAD_CPUTIME_ID, &thread);
- cputime_to_timespec(cputime_one_jiffy, &ts); + cputime_to_timespec64(cputime_one_jiffy, &ts); onecputick = ts.tv_nsec; WARN_ON(ts.tv_sec != 0);
All of the k_clock users have been converted to the new methods. This patch removes the older methods with timepsec/itimerspec type. As a result, the k_clock structure is ready for the year 2038.
Signed-off-by: Baolin Wang baolin.wang@linaro.org --- include/linux/posix-timers.h | 9 ------ kernel/time/posix-timers.c | 72 +++++++++++++++++------------------------- 2 files changed, 29 insertions(+), 52 deletions(-)
diff --git a/include/linux/posix-timers.h b/include/linux/posix-timers.h index 35786c5..7c3dae2 100644 --- a/include/linux/posix-timers.h +++ b/include/linux/posix-timers.h @@ -97,29 +97,20 @@ struct k_itimer { };
struct k_clock { - int (*clock_getres) (const clockid_t which_clock, struct timespec *tp); int (*clock_getres64) (const clockid_t which_clock, struct timespec64 *tp); - int (*clock_set) (const clockid_t which_clock, - const struct timespec *tp); int (*clock_set64) (const clockid_t which_clock, const struct timespec64 *tp); - int (*clock_get) (const clockid_t which_clock, struct timespec * tp); int (*clock_get64) (const clockid_t which_clock, struct timespec64 *tp); int (*clock_adj) (const clockid_t which_clock, struct timex *tx); int (*timer_create) (struct k_itimer *timer); int (*nsleep) (const clockid_t which_clock, int flags, struct timespec *, struct timespec __user *); long (*nsleep_restart) (struct restart_block *restart_block); - int (*timer_set) (struct k_itimer * timr, int flags, - struct itimerspec * new_setting, - struct itimerspec * old_setting); int (*timer_set64) (struct k_itimer *timr, int flags, struct itimerspec64 *new_setting, struct itimerspec64 *old_setting); int (*timer_del) (struct k_itimer * timr); #define TIMER_RETRY 1 - void (*timer_get) (struct k_itimer * timr, - struct itimerspec * cur_setting); void (*timer_get64) (struct k_itimer *timr, struct itimerspec64 *cur_setting); }; diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c index 47d1abf..3196ec0 100644 --- a/kernel/time/posix-timers.c +++ b/kernel/time/posix-timers.c @@ -528,13 +528,13 @@ void posix_timers_register_clock(const clockid_t clock_id, return; }
- if (!new_clock->clock_get && !new_clock->clock_get64) { - printk(KERN_WARNING "POSIX clock id %d lacks clock_get() and clock_get64()\n", + if (!new_clock->clock_get64) { + printk(KERN_WARNING "POSIX clock id %d lacks clock_get64()\n", clock_id); return; } - if (!new_clock->clock_getres && !new_clock->clock_getres64) { - printk(KERN_WARNING "POSIX clock id %d lacks clock_getres() and clock_getres64()\n", + if (!!new_clock->clock_getres64) { + printk(KERN_WARNING "POSIX clock id %d lacks clock_getres64()\n", clock_id); return; } @@ -585,7 +585,7 @@ static struct k_clock *clockid_to_kclock(const clockid_t id) return (id & CLOCKFD_MASK) == CLOCKFD ? &clock_posix_dynamic : &clock_posix_cpu;
- if (id >= MAX_CLOCKS || (!posix_clocks[id].clock_getres && !posix_clocks[id].clock_getres64)) + if (id >= MAX_CLOCKS || !posix_clocks[id].clock_getres64) return NULL; return &posix_clocks[id]; } @@ -788,15 +788,11 @@ SYSCALL_DEFINE2(timer_gettime, timer_t, timer_id, return -EINVAL;
kc = clockid_to_kclock(timr->it_clock); - if (WARN_ON_ONCE(!kc || (!kc->timer_get && !kc->timer_get64))) { + if (WARN_ON_ONCE(!kc || !kc->timer_get64)) { ret = -EINVAL; } else { - if (kc->timer_get64) { - kc->timer_get64(timr, &cur_setting64); - cur_setting = itimerspec64_to_itimerspec(cur_setting64); - } else { - kc->timer_get(timr, &cur_setting); - } + kc->timer_get64(timr, &cur_setting64); + cur_setting = itimerspec64_to_itimerspec(cur_setting64); }
unlock_timer(timr, flags); @@ -911,18 +907,14 @@ retry: return -EINVAL;
kc = clockid_to_kclock(timr->it_clock); - if (WARN_ON_ONCE(!kc || (!kc->timer_set && !kc->timer_set64))) { + if (WARN_ON_ONCE(!kc || !kc->timer_set64)) { error = -EINVAL; } else { - if (kc->timer_set64) { - new_spec64 = itimerspec_to_itimerspec64(new_spec); - error = kc->timer_set64(timr, flags, &new_spec64, - &old_spec64); - if (old_setting) - old_spec = itimerspec64_to_itimerspec(old_spec64); - } else { - error = kc->timer_set(timr, flags, &new_spec, rtn); - } + new_spec64 = itimerspec_to_itimerspec64(new_spec); + error = kc->timer_set64(timr, flags, &new_spec64, + &old_spec64); + if (old_setting) + old_spec = itimerspec64_to_itimerspec(old_spec64); }
unlock_timer(timr, flag); @@ -1032,18 +1024,14 @@ SYSCALL_DEFINE2(clock_settime, const clockid_t, which_clock, struct timespec new_tp; struct timespec64 new_tp64;
- if (!kc || (!kc->clock_set && !kc->clock_set64)) + if (!kc || !kc->clock_set64) return -EINVAL;
if (copy_from_user(&new_tp, tp, sizeof (*tp))) return -EFAULT;
- if (kc->clock_set64) { - new_tp64 = timespec_to_timespec64(new_tp); - return kc->clock_set64(which_clock, &new_tp64); - } else { - return kc->clock_set(which_clock, &new_tp); - } + new_tp64 = timespec_to_timespec64(new_tp); + return kc->clock_set64(which_clock, &new_tp64); }
SYSCALL_DEFINE2(clock_gettime, const clockid_t, which_clock, @@ -1057,14 +1045,13 @@ SYSCALL_DEFINE2(clock_gettime, const clockid_t, which_clock, if (!kc) return -EINVAL;
- if (kc->clock_get64) { - error = kc->clock_get64(which_clock, &kernel_tp64); - kernel_tp = timespec64_to_timespec(kernel_tp64); - } else { - error = kc->clock_get(which_clock, &kernel_tp); - } + error = kc->clock_get64(which_clock, &kernel_tp64); + if (!error) + return error; + + kernel_tp = timespec64_to_timespec(kernel_tp64);
- if (!error && copy_to_user(tp, &kernel_tp, sizeof (kernel_tp))) + if (copy_to_user(tp, &kernel_tp, sizeof (kernel_tp))) error = -EFAULT;
return error; @@ -1104,14 +1091,13 @@ SYSCALL_DEFINE2(clock_getres, const clockid_t, which_clock, if (!kc) return -EINVAL;
- if (kc->clock_getres64) { - error = kc->clock_getres64(which_clock, &rtn_tp64); - rtn_tp = timespec64_to_timespec(rtn_tp64); - } else { - error = kc->clock_getres(which_clock, &rtn_tp); - } + error = kc->clock_getres64(which_clock, &rtn_tp64); + if (!error) + return error; + + rtn_tp = timespec64_to_timespec(rtn_tp64);
- if (!error && tp && copy_to_user(tp, &rtn_tp, sizeof (rtn_tp))) + if (tp && copy_to_user(tp, &rtn_tp, sizeof (rtn_tp))) error = -EFAULT;
return error;
On Mon, Apr 20, 2015 at 01:57:39PM +0800, Baolin Wang wrote:
@@ -911,18 +907,14 @@ retry: return -EINVAL; kc = clockid_to_kclock(timr->it_clock);
- if (WARN_ON_ONCE(!kc || (!kc->timer_set && !kc->timer_set64))) {
- if (WARN_ON_ONCE(!kc || !kc->timer_set64)) { error = -EINVAL; } else {
if (kc->timer_set64) {new_spec64 = itimerspec_to_itimerspec64(new_spec);error = kc->timer_set64(timr, flags, &new_spec64,&old_spec64);if (old_setting)old_spec = itimerspec64_to_itimerspec(old_spec64);} else {error = kc->timer_set(timr, flags, &new_spec, rtn);}
new_spec64 = itimerspec_to_itimerspec64(new_spec);error = kc->timer_set64(timr, flags, &new_spec64,&old_spec64);
This statement can fit on one line.
if (old_setting)old_spec = itimerspec64_to_itimerspec(old_spec64);unlock_timer(timr, flag);
@@ -1057,14 +1045,13 @@ SYSCALL_DEFINE2(clock_gettime, const clockid_t, which_clock, if (!kc) return -EINVAL;
- if (kc->clock_get64) {
error = kc->clock_get64(which_clock, &kernel_tp64);kernel_tp = timespec64_to_timespec(kernel_tp64);- } else {
error = kc->clock_get(which_clock, &kernel_tp);- }
- error = kc->clock_get64(which_clock, &kernel_tp64);
- if (!error)
return error;
Wrong test, should be: if (error) ...
- kernel_tp = timespec64_to_timespec(kernel_tp64);
- if (!error && copy_to_user(tp, &kernel_tp, sizeof (kernel_tp)))
The (!error && ...) was correct here!
- if (copy_to_user(tp, &kernel_tp, sizeof (kernel_tp))) error = -EFAULT;
return error;
You can simplify this like so:
return copy_to_user(tp, &kernel_tp, sizeof(kernel_tp)) ? -EFAULT : 0;
@@ -1104,14 +1091,13 @@ SYSCALL_DEFINE2(clock_getres, const clockid_t, which_clock, if (!kc) return -EINVAL;
- if (kc->clock_getres64) {
error = kc->clock_getres64(which_clock, &rtn_tp64);rtn_tp = timespec64_to_timespec(rtn_tp64);- } else {
error = kc->clock_getres(which_clock, &rtn_tp);- }
- error = kc->clock_getres64(which_clock, &rtn_tp64);
- if (!error)
return error;
Also wrong.
- rtn_tp = timespec64_to_timespec(rtn_tp64);
- if (!error && tp && copy_to_user(tp, &rtn_tp, sizeof (rtn_tp)))
- if (tp && copy_to_user(tp, &rtn_tp, sizeof (rtn_tp))) error = -EFAULT;
return error; -- 1.7.9.5
Thanks, Richard
On 20 April 2015 at 16:42, Richard Cochran richardcochran@gmail.com wrote:
On Mon, Apr 20, 2015 at 01:57:39PM +0800, Baolin Wang wrote:
@@ -911,18 +907,14 @@ retry: return -EINVAL;
kc = clockid_to_kclock(timr->it_clock);
if (WARN_ON_ONCE(!kc || (!kc->timer_set && !kc->timer_set64))) {
if (WARN_ON_ONCE(!kc || !kc->timer_set64)) { error = -EINVAL; } else {
if (kc->timer_set64) {new_spec64 = itimerspec_to_itimerspec64(new_spec);error = kc->timer_set64(timr, flags, &new_spec64,&old_spec64);if (old_setting)old_spec =itimerspec64_to_itimerspec(old_spec64);
} else {error = kc->timer_set(timr, flags, &new_spec, rtn);}
new_spec64 = itimerspec_to_itimerspec64(new_spec);error = kc->timer_set64(timr, flags, &new_spec64,&old_spec64);This statement can fit on one line.
if (old_setting)old_spec = itimerspec64_to_itimerspec(old_spec64); } unlock_timer(timr, flag);@@ -1057,14 +1045,13 @@ SYSCALL_DEFINE2(clock_gettime, const clockid_t,
which_clock,
if (!kc) return -EINVAL;
if (kc->clock_get64) {error = kc->clock_get64(which_clock, &kernel_tp64);kernel_tp = timespec64_to_timespec(kernel_tp64);} else {error = kc->clock_get(which_clock, &kernel_tp);}
error = kc->clock_get64(which_clock, &kernel_tp64);if (!error)return error;Wrong test, should be: if (error) ...
kernel_tp = timespec64_to_timespec(kernel_tp64);
if (!error && copy_to_user(tp, &kernel_tp, sizeof (kernel_tp)))The (!error && ...) was correct here!
if (copy_to_user(tp, &kernel_tp, sizeof (kernel_tp))) error = -EFAULT; return error;You can simplify this like so:
return copy_to_user(tp, &kernel_tp, sizeof(kernel_tp)) ? -EFAULT :0;
@@ -1104,14 +1091,13 @@ SYSCALL_DEFINE2(clock_getres, const clockid_t,
which_clock,
if (!kc) return -EINVAL;
if (kc->clock_getres64) {error = kc->clock_getres64(which_clock, &rtn_tp64);rtn_tp = timespec64_to_timespec(rtn_tp64);} else {error = kc->clock_getres(which_clock, &rtn_tp);}
error = kc->clock_getres64(which_clock, &rtn_tp64);if (!error)return error;Also wrong.
rtn_tp = timespec64_to_timespec(rtn_tp64);
if (!error && tp && copy_to_user(tp, &rtn_tp, sizeof (rtn_tp)))
if (tp && copy_to_user(tp, &rtn_tp, sizeof (rtn_tp))) error = -EFAULT; return error;-- 1.7.9.5
Thanks, Richard
Thanks for your comments, i'll fix these mistakes in next patch series.
-
Arnd Bergmann -
Baolin Wang -
David Laight -
Luc Van Oostenryck -
Richard Cochran -
Sergei Shtylyov -
Thomas Gleixner