On 6/14/19 2:37 PM, Thomas Gleixner wrote:
> On Wed, 12 Jun 2019, Dmitry Safonov wrote:
>> ---
>> fs/timerfd.c | 3 +++
>> include/linux/time_namespace.h | 18 ++++++++++++++++++
>> kernel/time_namespace.c | 27 +++++++++++++++++++++++++++
>> 3 files changed, 48 insertions(+)
>
> Again, please split that into:
>
> 1) Introduce the new function
>
> 2) Make use of it
Will do
>
>> diff --git a/fs/timerfd.c b/fs/timerfd.c
>> index 6a6fc8aa1de7..9b0c2f65e7e8 100644
>> --- a/fs/timerfd.c
>> +++ b/fs/timerfd.c
>> @@ -26,6 +26,7 @@
>> #include <linux/syscalls.h>
>> #include <linux/compat.h>
>> #include <linux/rcupdate.h>
>> +#include <linux/time_namespace.h>
>>
>> struct timerfd_ctx {
>> union {
>> @@ -196,6 +197,8 @@ static int timerfd_setup(struct timerfd_ctx *ctx, int flags,
>> }
>>
>> if (texp != 0) {
>> + if (flags & TFD_TIMER_ABSTIME)
>> + texp = timens_ktime_to_host(clockid, texp);
>> if (isalarm(ctx)) {
>> if (flags & TFD_TIMER_ABSTIME)
>> alarm_start(&ctx->t.alarm, texp);
>> diff --git a/include/linux/time_namespace.h b/include/linux/time_namespace.h
>> index 1dda8af6b9fe..d32b55fad953 100644
>> --- a/include/linux/time_namespace.h
>> +++ b/include/linux/time_namespace.h
>> @@ -56,6 +56,19 @@ static inline void timens_add_boottime(struct timespec64 *ts)
>> *ts = timespec64_add(*ts, ns_offsets->boottime);
>> }
>>
>> +ktime_t do_timens_ktime_to_host(clockid_t clockid, ktime_t tim,
>> + struct timens_offsets *offsets);
>> +static inline ktime_t timens_ktime_to_host(clockid_t clockid, ktime_t tim)
>> +{
>> + struct timens_offsets *offsets = current->nsproxy->time_ns->offsets;
>> +
>> + if (!offsets) /* fast-path for the root time namespace */
>
> Can you please avoid tail comments. They break the reading flow. Aside of
> that I don't see the value of documenting the obvious.
>
>> +ktime_t do_timens_ktime_to_host(clockid_t clockid, ktime_t tim, struct timens_offsets *ns_offsets)
>
> Please line break the arguments
>
> ktime_t do_timens_ktime_to_host(clockid_t clockid, ktime_t tim,
> struct timens_offsets *ns_offsets)
Sure
>
>> +{
>> + ktime_t koff;
>> +
>> + switch (clockid) {
>> + case CLOCK_MONOTONIC:
>> + koff = timespec64_to_ktime(ns_offsets->monotonic);
>> + break;
>> + case CLOCK_BOOTTIME:
>> + case CLOCK_BOOTTIME_ALARM:
>> + koff = timespec64_to_ktime(ns_offsets->boottime);
>> + break;
>> + default:
>> + return tim;
>> + }
>> +
>> + /* tim - off has to be in [0, KTIME_MAX) */
>
> Please be more elaborate why the below conditions can happen at all.
>
>> + if (tim < koff)
>> + tim = 0;
>> + else if (KTIME_MAX - tim < -koff)
>> + tim = KTIME_MAX;
>> + else
>> + tim = ktime_sub(tim, koff);
Thanks,
Dmitry
