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

[Aniroop Mathur]

On Tue, Jan 12, 2016 at 6:22 AM, Peter Hutterer
<peter.hutterer@xxxxxxxxx> wrote:
> On Mon, Jan 11, 2016 at 07:46:53PM +0530, Aniroop Mathur wrote:
>> On Mon, Jan 11, 2016 at 9:41 AM, Peter Hutterer
>> <peter.hutterer@xxxxxxxxx> wrote:
>> > On Tue, Jan 05, 2016 at 03:26:41AM +0530, Aniroop Mathur wrote:
>> >> 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 and this in turn reduces
>> >> evdev client reading requests plus saves memory space filled by partial
>> >> events in evdev handler buffer.
>> >> Also, this patch prevents dropping of full packet by evdev client after
>> >> SYN_DROPPED occurs in case last stored event was SYN_REPORT.
>> >> (like after clock change request)
>> >
>> > this patch looks technically correct now, thanks. but tbh, especially after
>> > reading the EVIOCGBUFSIZE patch I'm wondering why you need to optimise this
>> > path. ignoring events after a SYN_DROPPED is easy in userspace, and you'll
>> > need the code to do so anyway because even with your patch, there is no API
>> > guarantee that this will always happen - if you rely on it, your code may
>> > break on a future kernel.
>> >
>> > From userland, this patch has no real effect, it only slightly reduces the
>> > chance that you get a SYN_DROPPED while reading events after a SYN_DROPPED
>> > has already occured. And if that's a problem, fixing the kernel is likely
>> > the wrong solution anyway. so yeah, still in doubt about whether this patch
>> > provides any real benefit.
>> >
>>
>> Hello Mr. Peter,
>>
>> I'm sorry that I'm really not able to understand you fully above.
>> Please, provide your opinion after seeing below reason of this patch and
>> elaborate more on above, if still required.
>>
>> As you can understand by seeing the patch code, this patch is required for
>> three reasons as follows:
>>
>> 1. To fix bug of dropping full valid packet events by userspace clients in case
>> last packet was fully stored and then syn_dropped occurs.
>>
>> For example:
>> Consider kernel buf contains:
>> ... SYN_REPORT REL_X, REL_Y, SYN_REPORT <one event space>
>> Now consider case that clock type is changed, so these events will be dropped
>> and syn_dropped will be queued.
>> OR
>> consider second case that new first packet event occur and that is stored in
>> last event space left, so all stored events will be dropped and syn_dropped
>> will be queued + newest event as per current code.
>> So now for first case, kernel buf contains: SYN_DROPPED
>> and for second case, kernel buf contains: SYN_DROPPED REL_X
>> Next consider that full packet is finished storing for both cases and
>> user-space is notified that events are ready to be read.
>> So now kernel buf contains: SYN_DROPPED REL_X REL_Y SYN_REPORT
>> But this new valid full packet event will be ignored by the userspace client
>> as mentioned in documentation that userspace clients ignore all events up to
>> and including next SYN_REPORT. As you know, this valid full event should not
>> be ignored by the userspace. So this patch fixes this bug.
>>

How about 1st point above? (a bug fix)

>> 2. To save small memory filled with partial events in evdev handler
>> kernel buffer after syn_dropped as these partial events will not be consumed by
>> clients anyways so no need to store them as well.
>>
>> For example:
>> Consider full packet as REL_X REL_Y REL_Z REL_RX REL_RY REL_RZ SYN_REPORT
>> as in case of magnetic sensor data which includes raw data along x, y, z axis
>> and noise data along x, y, z axis.
>> Consider kernel buf contains:
>> SYN_DROPPED REL_Y REL_Z REL_RX REL_RY REL_RZ SYN_REPORT REL_X REL_Y ...
>> As you know, REL_Y REL_Z REL_RX REL_RY REL_RZ are partial events that will not
>> be consumed by userspace clients, so this patch saves memory space of these
>> partial events by not storing them as it not consumed by clients as well.
>> With this patch, kernel buf will contain only required events:
>> SYN_DROPPED SYN_REPORT REL_X REL_Y ...
>>
>> 3. To reduce few looping iterations of reading partial events by user space
>> clients after syn_dropped occurs.
>>
>> For example:
>> Userspace client reads some events and userspace buf contains:
>> SYN_DROPPED REL_Y REL_Z REL_RX REL_RY REL_RZ SYN_REPORT REL_X <more events> ..
>> Client will process syn_dropped and decides to ignore next events until
>> syn_report occurs. So it processes REL_Y but ignores, processes REL_Z but
>> ignores, processes REL_RX but ignores, processes REL_RY but ignores, processes
>> REL_RZ but ignores and then it processes SYN_REPORT after which it decides to
>> not ignore any events any more. All this processing will basically be done in
>> a loop so with this patch extra looping cycles of partial events are removed
>> because with this patch userspace buf will contain:
>> SYN_DROPPED SYN_REPORT REL_X REL_Y <more events>
>> Hence we have saved a few looping cycles here.
>
> like I mentioned in the other email, I think you're optimising the wrong
> thing. SYN_DROPPED *must* be an exception. you need code to handle it, sure,
> but in the same way as you need code to handle read errors on a file. if
> you get a SYN_DROPPED during normal usage you need to optimise your
> application to read events faster, not hope that you can reduce the events
> after a SYN_DROPPED to avoid getting another one. and since this is only
> supposed to happen once every blue moon, saving a few cycles here does not
> gain you anything.
>

I understand what you mean.
It is rare that syn_dropped occur especially due to buffer overrun.
However, as you know it still can happen.
(especially in case of clock change request)

Even though a small improvement for a particular case,
combining 2nd and 3rd point, seems to be a "overall" plus point. Doesn't it?

BR,
Aniroop Mathur

> to give you an analogy: if you regularly end up with a flat tyre, you
> shouldn't focus on improving the efficiency of the pump. the problem is
> elsewhere.
>
> conincidentally, I strongly suggest that you use libevdev so you don't have
> to worry about these things. the code to handle SYN_DROPPED with libevdev is a
> couple of lines and otherwise identical to the normal code you'll have to
> deal with.
>
> Cheers,
>    Peter
>
>>
>> I also think that there is a need to change the patch title and description as
>> well to make it better:
>>
>> Subject: Drop partial events and queue syn_report after syn_dropped occurs.
>>
>> Description:
>> This patch introduces concept to drop partial events and queue syn_report
>> after syn_dropped which in turn does the following jobs:
>> - Fixes bug of dropping full valid packet events by userspace clients in case
>> last packet was fully stored and then syn_dropped occurs.
>> - Save small memory filled with partial events in evdev handler kernel buffer
>> after syn_dropped as these partial events will not be consumed by
>> clients anyways so no need to store them as well.
>> - Reduces few looping iterations of processing partial events by userspace
>> clients after syn_dropped occurs.
>>
>> Thanks.
>>
>> BR,
>> Aniroop Mathur
>>
>> > Cheers,
>> >    Peter
>> >
>> >> --
>> >> Please ignore v3.
>> >>
>> >> Signed-off-by: Aniroop Mathur <a.mathur@xxxxxxxxxxx>
>> >> ---
>> >>  drivers/input/evdev.c | 53 ++++++++++++++++++++++++++++++++++++++-------------
>> >>  1 file changed, 40 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> >> index e9ae3d5..15b6eb2 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 if partial events need to be dropped */
>> >>       unsigned long *evmasks[EV_CNT];
>> >>       unsigned int bufsize;
>> >>       struct input_event buffer[];
>> >> @@ -156,7 +157,12 @@ 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;
>> >> +     struct input_event *prev_ev;
>> >>       ktime_t time;
>> >> +     unsigned int mask = client->bufsize - 1;
>> >> +
>> >> +     /* Store previous event */
>> >> +     prev_ev = &client->buffer[(client->head - 1) & mask];
>> >>
>> >>       time = client->clk_type == EV_CLK_REAL ?
>> >>                       ktime_get_real() :
>> >> @@ -170,13 +176,33 @@ static void __evdev_queue_syn_dropped(struct evdev_client *client)
>> >>       ev.value = 0;
>> >>
>> >>       client->buffer[client->head++] = ev;
>> >> -     client->head &= client->bufsize - 1;
>> >> +     client->head &= mask;
>> >>
>> >>       if (unlikely(client->head == client->tail)) {
>> >>               /* drop queue but keep our SYN_DROPPED event */
>> >> -             client->tail = (client->head - 1) & (client->bufsize - 1);
>> >> +             client->tail = (client->head - 1) & mask;
>> >>               client->packet_head = client->tail;
>> >>       }
>> >> +
>> >> +     /*
>> >> +      * If last packet is NOT fully stored, set drop_pevent to true to
>> >> +      * ignore partial events and if last packet is fully stored, queue
>> >> +      * SYN_REPORT so that clients would not ignore next full packet.
>> >> +      */
>> >> +     if (prev_ev->type != EV_SYN && prev_ev->code != SYN_REPORT) {
>> >> +             client->drop_pevent = true;
>> >> +     } else if (prev_ev->type == EV_SYN && prev_ev->code == SYN_REPORT) {
>> >> +             prev_ev->time = ev.time;
>> >> +             client->buffer[client->head++] = *prev_ev;
>> >> +             client->head &= mask;
>> >> +             client->packet_head = client->head;
>> >> +
>> >> +             /* drop queue but keep our SYN_DROPPED & SYN_REPORT event */
>> >> +             if (unlikely(client->head == client->tail)) {
>> >> +                     client->tail = (client->head - 2) & mask;
>> >> +                     client->packet_head = client->tail;
>> >> +             }
>> >> +     }
>> >>  }
>> >>
>> >>  static void evdev_queue_syn_dropped(struct evdev_client *client)
>> >> @@ -235,18 +261,8 @@ static void __pass_event(struct evdev_client *client,
>> >>       client->head &= client->bufsize - 1;
>> >>
>> >>       if (unlikely(client->head == client->tail)) {
>> >> -             /*
>> >> -              * This effectively "drops" all unconsumed events, leaving
>> >> -              * EV_SYN/SYN_DROPPED plus the newest event in the queue.
>> >> -              */
>> >> -             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->packet_head = client->tail;
>> >> +             __evdev_queue_syn_dropped(client);
>> >>       }
>> >>
>> >>       if (event->type == EV_SYN && event->code == SYN_REPORT) {
>> >> @@ -284,6 +300,17 @@ static void evdev_pass_values(struct evdev_client *client,
>> >>                       wakeup = true;
>> >>               }
>> >>
>> >> +             /*
>> >> +              * drop partial events of last packet but queue SYN_REPORT
>> >> +              * so that clients would not ignore extra full packet.
>> >> +              */
>> >> +             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
>> >>
>> >> --
>> >> To unsubscribe from this list: send the line "unsubscribe linux-input" in
>> >> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> >>