[CRIU] [PATCHv3 05/27] timerfd/timens: Take into account ns clock offsets

[Thomas Gleixner]

On Thu, 25 Apr 2019, Dmitry Safonov wrote:
> From: Andrei Vagin <avagin at gmail.com>
> 
> Make timerfd respect timens offsets.
> Provide a helper timens_ktime_to_host() that is useful to wire up
> timens to different kernel subsystems.

Yet another changelog which lacks meat.

> @@ -179,6 +180,8 @@ static int timerfd_setup(struct timerfd_ctx *ctx, int flags,
>  	htmode = (flags & TFD_TIMER_ABSTIME) ?
>  		HRTIMER_MODE_ABS: HRTIMER_MODE_REL;
>  
> +	htmode |= HRTIMER_MODE_NS;

Without looking further this time. My gut reaction is that this is
wrong. Name space adjustment is only valid for absolute timers not for
relative timers.

Aside of that the name sucks. MODE_NS is really not intuitive. It could be
NanoSeconds or whatever and quite some time(r) functions have a _ns element
already. Please look for something more inuitive and clearly related to
namespaces. We are not short of letters.

>  	texp = timespec64_to_ktime(ktmr->it_value);
>  	ctx->expired = 0;
>  	ctx->ticks = 0;
> @@ -197,9 +200,10 @@ static int timerfd_setup(struct timerfd_ctx *ctx, int flags,
>  
>  	if (texp != 0) {
>  		if (isalarm(ctx)) {
> -			if (flags & TFD_TIMER_ABSTIME)
> +			if (flags & TFD_TIMER_ABSTIME) {
> +				texp = timens_ktime_to_host(clockid, texp);

You are not serious about that inline function here? It's huge and
pointless bloat because the only time affected here is boot time, but the
compiler does not know that.

>  				alarm_start(&ctx->t.alarm, texp);

Make that:

   alarm_start_namespace(.....)

and that does:

void alarm_start_namespace(struct alarm *alarm, ktime_t expires)
{
	if (alarm->type == ALARM_BOOTTIME)
		expires = timens_sub_boottime(expires);
	alarm_start(alarm, expires);
}

Hmm?

> -			else
> +			} else
>  				alarm_start_relative(&ctx->t.alarm, texp);
>  		} else {
>  			hrtimer_start(&ctx->t.tmr, texp, htmode);
> diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
> index 2e8957eac4d4..4b9c89c797ee 100644
> --- a/include/linux/hrtimer.h
> +++ b/include/linux/hrtimer.h
> @@ -38,6 +38,7 @@ enum hrtimer_mode {
>  	HRTIMER_MODE_REL	= 0x01,
>  	HRTIMER_MODE_PINNED	= 0x02,
>  	HRTIMER_MODE_SOFT	= 0x04,
> +	HRTIMER_MODE_NS		= 0x08,
>  
>  	HRTIMER_MODE_ABS_PINNED = HRTIMER_MODE_ABS | HRTIMER_MODE_PINNED,
>  	HRTIMER_MODE_REL_PINNED = HRTIMER_MODE_REL | HRTIMER_MODE_PINNED,
> diff --git a/include/linux/time_namespace.h b/include/linux/time_namespace.h
> index 5f0da6858b10..988414f7f791 100644
> --- a/include/linux/time_namespace.h
> +++ b/include/linux/time_namespace.h
> @@ -56,6 +56,41 @@ static inline void timens_add_boottime(struct timespec64 *ts)
>                  *ts = timespec64_add(*ts, ns_offsets->monotonic_boottime_offset);
>  }
>  
> +static inline ktime_t timens_ktime_to_host(clockid_t clockid, ktime_t tim)
> +{
> +	struct timens_offsets *ns_offsets = current->nsproxy->time_ns->offsets;
> +	struct timespec64 *offset;
> +	ktime_t koff;
> +
> +	if (!ns_offsets)
> +		return tim;
> +
> +	switch (clockid) {
> +		case CLOCK_MONOTONIC:
> +		case CLOCK_MONOTONIC_RAW:
> +		case CLOCK_MONOTONIC_COARSE:

What's the point of COARSE and RAW? Neither of them can be used to arm
timers.

> +			offset = &ns_offsets->monotonic_time_offset;
> +			break;
> +		case CLOCK_BOOTTIME:
> +		case CLOCK_BOOTTIME_ALARM:
> +			offset = &ns_offsets->monotonic_boottime_offset;
> +			break;
> +		default:
> +			return tim;
> +	}
> +
> +	koff = timespec64_to_ktime(*offset);

What about storing both the timespec and the ktime_t representation?

> +	if (tim < koff)
> +		tim = 0;
> +	else if (KTIME_MAX - tim < -koff)
> +		tim = KTIME_MAX;

Blink!?! This is completely nonobvious and you're going to stare at it in
disbelief half a year from now. Comments exist for a reason.

> +	else
> +		tim = ktime_sub(tim, koff);
> +
> +	return tim;

This whole thing is way too large for inlining.

Please create a function which does the magic substraction, something like
ktime_sub_namespace_offset() and invoke it from the proper places, i.e. the
alarmtimer one.

> @@ -1069,6 +1070,8 @@ static int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
>  
>  	if (mode & HRTIMER_MODE_REL)
>  		tim = ktime_add_safe(tim, base->get_time());
> +	else if (mode & HRTIMER_MODE_NS)
> +		tim = timens_ktime_to_host(base->clockid, tim);

You can do the same as for alarmtime above:

hrtimer_start_namespace(struct hrtimer *timer, ktime_t tim,
			const enum hrtimer_mode mode)
{
	if (mode & HRTIMER_MODE_ABS) {
		switch(timer->base->clockid) {
		case CLOCK_MONOTONIC:
			tim = timens_sub_monotonic(tim);
			break;
		case CLOCK_BOOTTIME:
			tim = timens_sub_boottime(tim);
			break;
		}
	}
	hrtimer_start(timer, tim, mode);
}

Thanks,

	tglx