Re: [PATCH] [v2]Input: evdev - drop partial events after emptying the buffer

On Fri, Jan 8, 2016 at 10:34 AM, Peter Hutterer
[off-list ref] wrote:

On Mon, Jan 04, 2016 at 09:58:48PM +0530, Aniroop Mathur wrote:

quoted

This patch introduces concept to drop partial events in evdev handler
itself after emptying the buffer which are dropped by all evdev
clients in userspace after SYN_DROPPED occurs.
This in turn saves space of partial events in evdev handler buffer
and reduces evdev client reading requests.

Signed-off-by: Aniroop Mathur <redacted>
---
 drivers/input/evdev.c | 56 ++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 9 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index e9ae3d5..0a5ead8 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c

@@ -58,6 +58,7 @@ struct evdev_client {
      struct list_head node;
      unsigned int clk_type;
      bool revoked;
+     bool drop_pevent; /* specifies whether partial events need to be dropped */
      unsigned long *evmasks[EV_CNT];
      unsigned int bufsize;
      struct input_event buffer[];

@@ -192,6 +193,7 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
 {
      unsigned long flags;
      unsigned int clk_type;
+     struct input_event ev;

      switch (clkid) {

@@ -218,8 +220,26 @@ static int evdev_set_clk_type(struct evdev_client *client, unsigned int clkid)
              spin_lock_irqsave(&client->buffer_lock, flags);

              if (client->head != client->tail) {
-                     client->packet_head = client->head = client->tail;
+
+                     /* Store previous event */
+                     client->head--;
+                     client->head &= client->bufsize - 1;
+                     ev = client->buffer[client->head];
+
+                     client->packet_head = client->head = client->tail = 0;
                      __evdev_queue_syn_dropped(client);
+
+                     /*
+                      * If last packet is fully stored, queue SYN_REPORT
+                      * so that clients do not ignore next full packet.
+                      * And if last packet is NOT fully stored,
+                      * set drop_pevent to true to ignore partial events.
+                      */
+                     if (ev.type == EV_SYN && ev.code == SYN_REPORT) {
+                             ev.time = client->buffer[client->head - 1].time;

if client->head happens to be 0, your index won't be what it should be :)
you need the &= here and again after the next line:

I have set head to 0 after which syn_dropped is queued,
so head will be 1 always and hence it seems no need to take
care of head overflow here as min bufsize is 64.

quoted

+                             client->buffer[client->head++] = ev;

quoted

+                     } else if (ev.type != EV_SYN && ev.code != SYN_REPORT)
+                             client->drop_pevent = true;
              }

              spin_unlock_irqrestore(&client->buffer_lock, flags);

@@ -236,17 +256,27 @@ static void __pass_event(struct evdev_client *client,

      if (unlikely(client->head == client->tail)) {
              /*
-              * This effectively "drops" all unconsumed events, leaving
-              * EV_SYN/SYN_DROPPED plus the newest event in the queue.
+              * This effectively "drops" all unconsumed events, storing
+              * EV_SYN/SYN_DROPPED and the newest event in the queue but
+              * only if it is EV_SYN/SYN_REPORT so that clients can read
+              * the next full event packet. And set drop_pevent to true
+              * if last event packet is not stored completely in buffer
+              * to ignore upcoming partial events.
               */
-             client->tail = (client->head - 2) & (client->bufsize - 1);

-             client->buffer[client->tail].time = event->time;
-             client->buffer[client->tail].type = EV_SYN;
-             client->buffer[client->tail].code = SYN_DROPPED;
-             client->buffer[client->tail].value = 0;
+             client->tail = client->head = client->packet_head;

-             client->packet_head = client->tail;
+             client->buffer[client->head].time = event->time;
+             client->buffer[client->head].type = EV_SYN;
+             client->buffer[client->head].code = SYN_DROPPED;
+             client->buffer[client->head].value = 0;
+             client->head = (client->head + 1) & (client->bufsize -1);
+
+             if (event->type == EV_SYN && event->code == SYN_REPORT) {
+                     client->buffer[client->head++] = *event;
+                     client->head &= client->bufsize - 1;
+             } else if (event->type != EV_SYN && event->code != SYN_REPORT)
+                     client->drop_pevent = true;

given that this condition is almost exactly the same as above, you should
share the code. you should also look into checking whether this can be moved
to __evdev_queue_syn_dropped() anyway, it seems like it can with little
effort.

The protocol itself looks correct now though.

Thank you Mr. Peter.
I have already sent the v4 which moved the code to
__evdev_queue_syn_dropped() function. Please have a look.

Regards,
Aniroop

Cheers,
   Peter

quoted

      }

      if (event->type == EV_SYN && event->code == SYN_REPORT) {

@@ -284,6 +314,14 @@ static void evdev_pass_values(struct evdev_client *client,
                      wakeup = true;
              }

+             /* drop partial events of last packet but queue SYN_REPORT */
+             if (client->drop_pevent) {
+                     if (v->type == EV_SYN && v->code == SYN_REPORT)
+                             client->drop_pevent = false;
+                     else
+                             continue;
+             }
+
              event.type = v->type;
              event.code = v->code;
              event.value = v->value;
--

2.6.2