Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 13 Nov 2019 21:06:11 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Arnd Bergmann <arnd@arndb.de>, y2038@lists.linaro.org,
        Ingo Molnar <mingo@redhat.com>, linux-kernel@vger.kernel.org,
        Anna-Maria Gleixner <anna-maria@linutronix.de>,
        Frederic Weisbecker <frederic@kernel.org>,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Kees Cook <keescook@chromium.org>
Subject: Re: [PATCH 21/23] y2038: itimer: change implementation to
 timespec64
Message-ID: <20191113210611.515868a4@oasis.local.home>
In-Reply-To: <alpine.DEB.2.21.1911132306070.2507@nanos.tec.linutronix.de>
References: <20191108210236.1296047-1-arnd@arndb.de>
        <20191108211323.1806194-12-arnd@arndb.de>
        <alpine.DEB.2.21.1911132306070.2507@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 13 Nov 2019 23:28:47 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> 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