Re: [PATCH 21/23] y2038: itimer: change implementation to timespec64

[Steven Rostedt]

On Wed, 13 Nov 2019 23:28:47 +0100 (CET)
Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> On Fri, 8 Nov 2019, Arnd Bergmann wrote:
> >  TRACE_EVENT(itimer_state,
> >  
> > -	TP_PROTO(int which, const struct itimerval *const value,
> > +	TP_PROTO(int which, const struct itimerspec64 *const value,
> >  		 unsigned long long expires),
> >  
> >  	TP_ARGS(which, value, expires),
> > @@ -321,12 +321,12 @@ TRACE_EVENT(itimer_state,
> >  		__entry->which		= which;
> >  		__entry->expires	= expires;
> >  		__entry->value_sec	= value->it_value.tv_sec;
> > -		__entry->value_usec	= value->it_value.tv_usec;
> > +		__entry->value_usec	= value->it_value.tv_nsec / NSEC_PER_USEC;
> >  		__entry->interval_sec	= value->it_interval.tv_sec;
> > -		__entry->interval_usec	= value->it_interval.tv_usec;
> > +		__entry->interval_usec	= value->it_interval.tv_nsec / NSEC_PER_USEC;  
> 
> Hmm, having a division in a tracepoint is clearly suboptimal.

Right, we should move the division into the TP_printk()

		__entry->interval_nsec = alue->it_interval.tv_nsec;

> 
> >  	),
> >  
> > -	TP_printk("which=%d expires=%llu it_value=%ld.%ld it_interval=%ld.%ld",
> > +	TP_printk("which=%d expires=%llu it_value=%ld.%06ld it_interval=%ld.%06ld",  
> 
> We print only 6 digits after the . so that would be even correct w/o a
> division. But it probably does not matter much.

Well, we still need the division in the printk, otherwise it will print
more than 6. That's just the minimum and it will print the full number.


		__entry->interval_nsec / NSEC_PER_USEC


-- Steve

> 
> > @@ -197,19 +207,13 @@ static void set_cpu_itimer(struct task_struct *tsk, unsigned int clock_id,
> >  #define timeval_valid(t) \
> >  	(((t)->tv_sec >= 0) && (((unsigned long) (t)->tv_usec) < USEC_PER_SEC))  
> 
> Hrm, why do we have yet another incarnation of timeval_valid()? Can we
> please have only one (the inline version)?
> 
> Thanks,
> 
> 	tglx