Re: [PATCH v5 2/3] input: evdev: Replace timeval with timespec64

[Dmitry Torokhov]

On Sat, Jan 06, 2018 at 01:43:34PM -0800, Deepa Dinamani wrote:
> On Tue, Jan 2, 2018 at 7:35 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > On Tue, Jan 2, 2018 at 7:46 AM, Dmitry Torokhov
> > <dmitry.torokhov@xxxxxxxxx> 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