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

[Andrei Vagin]

Hi Thomas,

Thank you for the review.  I read your comments. All of them look
reasonable. I'm sorry that you had to comment a lot. Will fix in the
next version.

Thanks,
Andrei

On Thu, Apr 25, 2019 at 11:28:24PM +0200, Thomas Gleixner wrote:
> On Thu, 25 Apr 2019, Dmitry Safonov wrote:
> > From: Andrei Vagin <avagin@xxxxxxxxx>
> > 
> > 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