Message-ID: <20160112005255.GD7838@jelly.redhat.com>
Date:	Tue, 12 Jan 2016 10:52:55 +1000
From:	Peter Hutterer <peter.hutterer@...-t.net>
To:	Aniroop Mathur <aniroop.mathur@...il.com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Aniroop Mathur <a.mathur@...sung.com>,
	Benjamin Tissoires <benjamin.tissoires@...il.com>,
	David Herrmann <dh.herrmann@...il.com>,
	Henrik Rydberg <rydberg@...math.org>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	s.samuel@...sung.com, r.mahale@...sung.com
Subject: Re: [PATCH] [v4]Input: evdev - drop partial events after emptying
 the buffer

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@...-t.net> 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.
> 
> 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.

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@...sung.com>
> >> ---
> >>  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@...r.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >>

Powered by blists - more mailing lists