From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dmitry Torokhov Subject: Re: [PATCH v5 2/3] input: evdev: Replace timeval with timespec64 Date: Mon, 8 Jan 2018 12:54:00 -0800 Message-ID: <20180108205400.3tsq2egipygaov72@dtor-ws> References: <20171218051844.10193-1-deepa.kernel@gmail.com> <20171218051844.10193-3-deepa.kernel@gmail.com> <20180102064617.zmv6ospfkectaa37@dtor-ws> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mail-pg0-f66.google.com ([74.125.83.66]:44919 "EHLO mail-pg0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757783AbeAHUyE (ORCPT ); Mon, 8 Jan 2018 15:54:04 -0500 Content-Disposition: inline In-Reply-To: Sender: linux-input-owner@vger.kernel.org List-Id: linux-input@vger.kernel.org To: Deepa Dinamani Cc: Arnd Bergmann , "open list:HID CORE LAYER" , Linux Kernel Mailing List , Peter Hutterer , y2038 Mailman List On Sat, Jan 06, 2018 at 01:43:34PM -0800, Deepa Dinamani wrote: > On Tue, Jan 2, 2018 at 7:35 AM, Arnd Bergmann wrote: > > On Tue, Jan 2, 2018 at 7:46 AM, Dmitry Torokhov > > wrote: > >> On Sun, Dec 17, 2017 at 09:18:43PM -0800, Deepa Dinamani wrote: > >>> @@ -304,12 +314,11 @@ static void evdev_events(struct input_handle *handle, > >>> { > >>> struct evdev *evdev = handle->private; > >>> struct evdev_client *client; > >>> - ktime_t ev_time[EV_CLK_MAX]; > >>> + struct timespec64 ev_time[EV_CLK_MAX]; > >>> > >>> - ev_time[EV_CLK_MONO] = ktime_get(); > >>> - ev_time[EV_CLK_REAL] = ktime_mono_to_real(ev_time[EV_CLK_MONO]); > >>> - ev_time[EV_CLK_BOOT] = ktime_mono_to_any(ev_time[EV_CLK_MONO], > >>> - TK_OFFS_BOOT); > >>> + ktime_get_ts64(&ev_time[EV_CLK_MONO]); > >>> + ktime_get_real_ts64(&ev_time[EV_CLK_REAL]); > >>> + get_monotonic_boottime64(&ev_time[EV_CLK_BOOT]); > >> > >> This may result in different ev_time[] members holding different times, > >> whereas the original code would take one time sample and convert it to > >> different clocks. > > > > Is this important? On each client we only return one of the two > > times, and I would guess that you cannot rely on a correlation > > between timestamps on different devices, since the boot and real > > offsets can change over time. > > Right. I didn't think this was an issue either. > > >> Also, why can't we keep using ktime_t internally? It is y2038 safe, > >> right? > > > > Correct, but there may also be a performance difference if we get > > a lot of events, not sure if that matters. > > > >> I think you should drop this patch and adjust the 3rd one to > >> massage the input event timestamp patch to do ktime->timespec64->input > >> timestamp conversion. > > > > The change in __evdev_queue_syn_dropped still seems useful to me > > as ktime_get_*ts64() is a bit more efficient than ktime_get*() followed by > > a slow ktime_to_timespec64() or ktime_to_timeval(). > > > > For evdev_events(), doing a single ktime_get() followed by a > > ktime_to_timespec64/ktime_to_timeval can be faster than three > > ktime_get_*ts64 (depending on the hardware clock source), or > > it can be slower depending on the CPU and the clocksource > > hardware. Again, no idea if this matters at the usual rate of > > input events. > > > > I guess dropping the evdev_events() change and replacing it with a > > ktime_to_timespec64 change in evdev_pass_values() > > would be fine here, it should keep the current performance > > behavior and get rid of the timeval. > > I was trying to use timespec64 everywhere so that we would not have > conversions back and forth at the input layer. > I dropped the ktime_t conversions for now and merged this patch with > the next one as requested. > > Let me know if you would like to keep the changes Arnd preferred above > for __evdev_queue_syn_dropped(). I can submit a separate patch if this > is preferred. __evdev_queue_syn_dropped() is extremely cold path (hopefully, if it is not we have much bigger problems) so I'd leave it as is. Thanks! -- Dmitry