| Message ID | 20171218051844.10193-3-deepa.kernel@gmail.com (mailing list archive) |
|---|---|
| State | Under Review |
| Headers | show |
Hi Deepa, On Sun, Dec 17, 2017 at 09:18:43PM -0800, Deepa Dinamani wrote: > struct timeval is not y2038 safe. > > All references to timeval in the kernel will be replaced > by y2038 safe structures. > Replace all references to timeval with y2038 safe > struct timespec64 here. > > struct input_event will be changed in a different patch. > > Signed-off-by: Deepa Dinamani <deepa.kernel@gmail.com> > Reviewed-by: Arnd Bergmann <arnd@arndb.de> > Acked-by: Peter Hutterer <peter.hutterer@who-t.net> > --- > drivers/input/evdev.c | 37 +++++++++++++++++++++++-------------- > 1 file changed, 23 insertions(+), 14 deletions(-) > > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c > index 0193dd4f0452..3171c4882d80 100644 > --- a/drivers/input/evdev.c > +++ b/drivers/input/evdev.c > @@ -156,15 +156,22 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type) > static void __evdev_queue_syn_dropped(struct evdev_client *client) > { > struct input_event ev; > - ktime_t time; > + struct timespec64 ts; > > - time = client->clk_type == EV_CLK_REAL ? > - ktime_get_real() : > - client->clk_type == EV_CLK_MONO ? > - ktime_get() : > - ktime_get_boottime(); > + switch (client->clk_type) { > + case EV_CLK_REAL: > + ktime_get_real_ts64(&ts); > + break; > + case EV_CLK_MONO: > + ktime_get_ts64(&ts); > + break; > + case EV_CLK_BOOT: > + get_monotonic_boottime64(&ts); > + break; > + } > > - ev.time = ktime_to_timeval(time); > + ev.time.tv_sec = ts.tv_sec; > + ev.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC; > ev.type = EV_SYN; > ev.code = SYN_DROPPED; > ev.value = 0; > @@ -257,17 +264,20 @@ static void __pass_event(struct evdev_client *client, > > static void evdev_pass_values(struct evdev_client *client, > const struct input_value *vals, unsigned int count, > - ktime_t *ev_time) > + struct timespec64 *ev_time) > { > struct evdev *evdev = client->evdev; > const struct input_value *v; > struct input_event event; > + struct timespec64 ts; > bool wakeup = false; > > if (client->revoked) > return; > > - event.time = ktime_to_timeval(ev_time[client->clk_type]); > + ts = ev_time[client->clk_type]; > + event.time.tv_sec = ts.tv_sec; > + event.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC; > > /* Interrupts are disabled, just acquire the lock. */ > spin_lock(&client->buffer_lock); > @@ -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. Also, why can't we keep using ktime_t internally? It is y2038 safe, right? 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. Thanks.
On Tue, Jan 2, 2018 at 7:46 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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. > 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. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 2, 2018 at 7:35 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, Jan 2, 2018 at 7:46 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> 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. -Deepa -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@arndb.de> wrote: > > On Tue, Jan 2, 2018 at 7:46 AM, Dmitry Torokhov > > <dmitry.torokhov@gmail.com> 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!
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c index 0193dd4f0452..3171c4882d80 100644 --- a/drivers/input/evdev.c +++ b/drivers/input/evdev.c @@ -156,15 +156,22 @@ static void __evdev_flush_queue(struct evdev_client *client, unsigned int type) static void __evdev_queue_syn_dropped(struct evdev_client *client) { struct input_event ev; - ktime_t time; + struct timespec64 ts; - time = client->clk_type == EV_CLK_REAL ? - ktime_get_real() : - client->clk_type == EV_CLK_MONO ? - ktime_get() : - ktime_get_boottime(); + switch (client->clk_type) { + case EV_CLK_REAL: + ktime_get_real_ts64(&ts); + break; + case EV_CLK_MONO: + ktime_get_ts64(&ts); + break; + case EV_CLK_BOOT: + get_monotonic_boottime64(&ts); + break; + } - ev.time = ktime_to_timeval(time); + ev.time.tv_sec = ts.tv_sec; + ev.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC; ev.type = EV_SYN; ev.code = SYN_DROPPED; ev.value = 0; @@ -257,17 +264,20 @@ static void __pass_event(struct evdev_client *client, static void evdev_pass_values(struct evdev_client *client, const struct input_value *vals, unsigned int count, - ktime_t *ev_time) + struct timespec64 *ev_time) { struct evdev *evdev = client->evdev; const struct input_value *v; struct input_event event; + struct timespec64 ts; bool wakeup = false; if (client->revoked) return; - event.time = ktime_to_timeval(ev_time[client->clk_type]); + ts = ev_time[client->clk_type]; + event.time.tv_sec = ts.tv_sec; + event.time.tv_usec = ts.tv_nsec / NSEC_PER_USEC; /* Interrupts are disabled, just acquire the lock. */ spin_lock(&client->buffer_lock); @@ -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]); rcu_read_lock();