[v2,4/4] input: evdev: Make device readable only when it contains a complete packet.

Commit Message

Dmitry Torokhov April 4, 2011, 9:36 p.m. UTC

On Fri, Apr 01, 2011 at 11:54:19PM -0700, Jeff Brown wrote:
> This patch modifies evdev so that it only becomes readable when
> the buffer contains an EV_SYN event other than SYN_MT_REPORT.

I think we should target SYN_REPORT directly. SYN_CONFIG is unused and
SYN_DROPPED is not interesting till next SYN_REPORT anyway. Given the
changes to the previous patch I have the following:


Input: evdev - only signal polls on full packets

From: Jeff Brown <[e-mail]>

This patch modifies evdev so that it only becomes readable when
the buffer contains an EV_SYN/SYN_REPORT event.

On SMP systems, it is possible for an evdev client blocked on poll()
to wake up and read events from the evdev ring buffer at the same
rate as they are enqueued.  This can result in high CPU usage,
particularly for MT devices, because the client ends up reading
events one at a time instead of reading complete packets.

We eliminate this problem by making the device readable only when
the buffer contains at least one complete packet.  This causes
clients to block until the entire packet is available.

Signed-off-by: Jeff Brown <[e-mail]>
Signed-off-by: Dmitry Torokhov <[e-mail]>
---

 drivers/input/evdev.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)



Thanks.

Comments

Jeff Brown April 4, 2011, 10:16 p.m. UTC | #1

Hi Dmitry,
I don't think the new patch is completely correct.

On Mon, Apr 4, 2011 at 2:36 PM, Dmitry Torokhov
<[e-mail]> wrote:
> I think we should target SYN_REPORT directly. SYN_CONFIG is unused and
> SYN_DROPPED is not interesting till next SYN_REPORT anyway. Given the
> changes to the previous patch I have the following:

Explicitly checking for SYN_REPORT makes sense.  I wasn't sure to do
with SYN_CONFIG before so I tried to keep the condition somewhat
conservative.

Per previous comments on an older iteration of this patch, it probably
makes sense to calculate this flag once in evdev_event and pass it to
evdev_pass_event.

bool full_sync = (type == EV_SYN && code == SYN_REPORT);

> @@ -41,6 +41,7 @@ struct evdev {
>  struct evdev_client {
>        unsigned int head;
>        unsigned int tail;
> +       unsigned int last_syn;  /* position of the last EV_SYN/SYN_REPORT */

This comment for last_syn is not quite right.  We need last_syn to
refer to the position just beyond the last sync.  Otherwise the device
will not become readable until another event is written there.  The
invariants for last_syn should be similar to those for head.

Whereas tail != head means buffer non-empty, tail != last_syn should
mean buffer is readable.

It looks like we almost maintain those invariants here, except for SYN_DROPPED.

>        spinlock_t buffer_lock; /* protects access to buffer, head and tail */
>        struct fasync_struct *fasync;
>        struct evdev *evdev;
> @@ -72,12 +73,16 @@ static void evdev_pass_event(struct evdev_client *client,
>                client->buffer[client->tail].type = EV_SYN;
>                client->buffer[client->tail].code = SYN_DROPPED;
>                client->buffer[client->tail].value = 0;
> +

Should use client->head here so that the SYN_DROPPED is readable.

> +               client->last_syn = client->tail;
>        }
>
>        spin_unlock(&client->buffer_lock);

Can use full_sync or something equivalent instead of repeating the
condition on EV_SYN / SYN_REPORT here.

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

I don't think it's safe to modify last_syn outside of the spin lock.
This should be done above.

> +               client->last_syn = client->head;
>                kill_fasync(&client->fasync, SIGIO, POLL_IN);
> +       }
>  }

MISSING: We need to also modify evdev_event to only call
wake_up_interruptible when enqueuing a sync.  It does not make sense
to wake up waiters unless the device is about to become readable
again.

This also means we should wake after having written SYN_DROPPED.  We
might need to make evdev_pass_event return (or take by reference) a
boolean that indicates whether at least one client has become
readable.

Pseudo-code:

if (full_sync || evdev_became_readable_for_a_client_due_to_syn_dropped)
    wake_up_interruptible(&evdev->wait);

>  /*
> @@ -387,12 +392,12 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
>        if (count < input_event_size())
>                return -EINVAL;
> -       if (client->head == client->tail && evdev->exist &&
> +       if (client->last_syn == client->tail && evdev->exist &&
>            (file->f_flags & O_NONBLOCK))
>                return -EAGAIN;
>
>        retval = wait_event_interruptible(evdev->wait,
> -               client->head != client->tail || !evdev->exist);
> +               client->last_syn != client->tail || !evdev->exist);
>        if (retval)
>                return retval;
>
> @@ -421,7 +426,7 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
>        poll_wait(file, &evdev->wait, wait);
>
>        mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
> -       if (client->head != client->tail)
> +       if (client->last_syn != client->tail)
>                mask |= POLLIN | POLLRDNORM;
>
>        return mask;

It looks to me like this patch isn't based on top of your previous
patch for SYN_DROPPED.  Specifically, the SYN_DROPPED should be
inserted before the newly enqueued event but I don't see that above.

Jeff.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov April 4, 2011, 10:46 p.m. UTC | #2

On Mon, Apr 04, 2011 at 03:16:13PM -0700, Jeffrey Brown wrote:
> Hi Dmitry,
> I don't think the new patch is completely correct.
> 
> On Mon, Apr 4, 2011 at 2:36 PM, Dmitry Torokhov
> <[e-mail]> wrote:
> > I think we should target SYN_REPORT directly. SYN_CONFIG is unused and
> > SYN_DROPPED is not interesting till next SYN_REPORT anyway. Given the
> > changes to the previous patch I have the following:
> 
> Explicitly checking for SYN_REPORT makes sense.  I wasn't sure to do
> with SYN_CONFIG before so I tried to keep the condition somewhat
> conservative.
> 
> Per previous comments on an older iteration of this patch, it probably
> makes sense to calculate this flag once in evdev_event and pass it to
> evdev_pass_event.
> 
> bool full_sync = (type == EV_SYN && code == SYN_REPORT);

I am not sure what is cheaper - 2 conditionals or stack manipulation
needed to push another argument if we happed to be register-starved.

> 
> > @@ -41,6 +41,7 @@ struct evdev {
> >  struct evdev_client {
> >        unsigned int head;
> >        unsigned int tail;
> > +       unsigned int last_syn;  /* position of the last EV_SYN/SYN_REPORT */
> 
> This comment for last_syn is not quite right.  We need last_syn to
> refer to the position just beyond the last sync.  Otherwise the device
> will not become readable until another event is written there.  The
> invariants for last_syn should be similar to those for head.

Hm, yes, comment is incorrect. Given this fact I do not like the name
anymore either (nor do I like 'end'). Need to think about something
better.

> 
> Whereas tail != head means buffer non-empty, tail != last_syn should
> mean buffer is readable.
> 
> It looks like we almost maintain those invariants here, except for SYN_DROPPED.
> 
> >        spinlock_t buffer_lock; /* protects access to buffer, head and tail */
> >        struct fasync_struct *fasync;
> >        struct evdev *evdev;
> > @@ -72,12 +73,16 @@ static void evdev_pass_event(struct evdev_client *client,
> >                client->buffer[client->tail].type = EV_SYN;
> >                client->buffer[client->tail].code = SYN_DROPPED;
> >                client->buffer[client->tail].value = 0;
> > +
> 
> Should use client->head here so that the SYN_DROPPED is readable.

It is readable, but we do not want to signal on it.

> 
> > +               client->last_syn = client->tail;
> >        }
> >
> >        spin_unlock(&client->buffer_lock);
> 
> Can use full_sync or something equivalent instead of repeating the
> condition on EV_SYN / SYN_REPORT here.
> 
> > -       if (event->type == EV_SYN)
> > +       if (event->type == EV_SYN && event->code == SYN_REPORT) {
> 
> I don't think it's safe to modify last_syn outside of the spin lock.
> This should be done above.

This is the only writer, plus we are running under event_lock with
interrupts off, so it is safe.

> 
> > +               client->last_syn = client->head;
> >                kill_fasync(&client->fasync, SIGIO, POLL_IN);
> > +       }
> >  }
> 
> MISSING: We need to also modify evdev_event to only call
> wake_up_interruptible when enqueuing a sync.  It does not make sense
> to wake up waiters unless the device is about to become readable
> again.

Right, I'll add it.

> 
> This also means we should wake after having written SYN_DROPPED.  We
> might need to make evdev_pass_event return (or take by reference) a
> boolean that indicates whether at least one client has become
> readable.

Why? Why would we not want to wait till the next SYNC to deliver
DROPPED?

> 
> Pseudo-code:
> 
> if (full_sync || evdev_became_readable_for_a_client_due_to_syn_dropped)
>     wake_up_interruptible(&evdev->wait);
> 
> >  /*
> > @@ -387,12 +392,12 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
> >        if (count < input_event_size())
> >                return -EINVAL;
> > -       if (client->head == client->tail && evdev->exist &&
> > +       if (client->last_syn == client->tail && evdev->exist &&
> >            (file->f_flags & O_NONBLOCK))
> >                return -EAGAIN;
> >
> >        retval = wait_event_interruptible(evdev->wait,
> > -               client->head != client->tail || !evdev->exist);
> > +               client->last_syn != client->tail || !evdev->exist);
> >        if (retval)
> >                return retval;
> >
> > @@ -421,7 +426,7 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
> >        poll_wait(file, &evdev->wait, wait);
> >
> >        mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
> > -       if (client->head != client->tail)
> > +       if (client->last_syn != client->tail)
> >                mask |= POLLIN | POLLRDNORM;
> >
> >        return mask;
> 
> It looks to me like this patch isn't based on top of your previous
> patch for SYN_DROPPED.  Specifically, the SYN_DROPPED should be
> inserted before the newly enqueued event but I don't see that above.

Yes it does - please check the chunk for evdevPass_event again.

Jeff Brown April 5, 2011, 12:34 a.m. UTC | #3

Dmitry,

On Mon, Apr 4, 2011 at 3:46 PM, Dmitry Torokhov
<[e-mail]> wrote:
> On Mon, Apr 04, 2011 at 03:16:13PM -0700, Jeffrey Brown wrote:
>> bool full_sync = (type == EV_SYN && code == SYN_REPORT);
>
> I am not sure what is cheaper - 2 conditionals or stack manipulation
> needed to push another argument if we happed to be register-starved.

Not a question of the computational cost.  It was mostly done to avoid
repeating the same predicate in multiple places.  This was one of the
suggested improvements to my earlier patch.

>> This comment for last_syn is not quite right.  We need last_syn to
>> refer to the position just beyond the last sync.  Otherwise the device
>> will not become readable until another event is written there.  The
>> invariants for last_syn should be similar to those for head.
>
> Hm, yes, comment is incorrect. Given this fact I do not like the name
> anymore either (nor do I like 'end'). Need to think about something
> better.

Heh, I faced this very same dilemma.  I tried 'last_sync',
'readable_tail', 'read_end', and others before settling on 'end' and a
descriptive comment.

>> Should use client->head here so that the SYN_DROPPED is readable.
>
> It is readable, but we do not want to signal on it.

I think we do want to signal on it.  We should signal whenever the
device becomes readable.

Signaling on dropped is useful in the case where a misbehaving device
driver fails to ever call input_sync.  If that happens, we might
enqueue a dropped event and then never wake up the client which makes
the issue hard to diagnose.

>> I don't think it's safe to modify last_syn outside of the spin lock.
>> This should be done above.
>
> This is the only writer, plus we are running under event_lock with
> interrupts off, so it is safe.

The value will be read concurrently by evdev_fetch_next_event.  So if
this were safe, then we wouldn't need the spin lock at all.

At the very least for the sake of consistency, I think we should keep
the buffer manipulations within the guarded region.

Jeff.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Henrik Rydberg April 5, 2011, 12:03 p.m. UTC | #4

> >> Should use client->head here so that the SYN_DROPPED is readable.
> >
> > It is readable, but we do not want to signal on it.
> 
> I think we do want to signal on it.  We should signal whenever the
> device becomes readable.
> 
> Signaling on dropped is useful in the case where a misbehaving device
> driver fails to ever call input_sync.  If that happens, we might
> enqueue a dropped event and then never wake up the client which makes
> the issue hard to diagnose.

A device that never wakes up the client seems like a detectable
symptom. I agree with Dmitry, the dropped event is more of a note in
passing, and as such can stay in the pipe until a real EV_SYN event
comes along.

> >> I don't think it's safe to modify last_syn outside of the spin lock.
> >> This should be done above.
> >
> > This is the only writer, plus we are running under event_lock with
> > interrupts off, so it is safe.
> 
> The value will be read concurrently by evdev_fetch_next_event.  So if
> this were safe, then we wouldn't need the spin lock at all.

The spinlock ensures atomic read/write of the event buffer. The
position into the buffer does not need the lock.

> At the very least for the sake of consistency, I think we should keep
> the buffer manipulations within the guarded region.

Sounds reasonable.

Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dmitry Torokhov April 5, 2011, 4:32 p.m. UTC | #5

On Tue, Apr 05, 2011 at 02:03:27PM +0200, Henrik Rydberg wrote:
> > >> Should use client->head here so that the SYN_DROPPED is readable.
> > >
> > > It is readable, but we do not want to signal on it.
> > 
> > I think we do want to signal on it.  We should signal whenever the
> > device becomes readable.
> > 
> > Signaling on dropped is useful in the case where a misbehaving device
> > driver fails to ever call input_sync.  If that happens, we might
> > enqueue a dropped event and then never wake up the client which makes
> > the issue hard to diagnose.
> 
> A device that never wakes up the client seems like a detectable
> symptom. I agree with Dmitry, the dropped event is more of a note in
> passing, and as such can stay in the pipe until a real EV_SYN event
> comes along.

Also we have evbug module to report raw event stream from theinput core
without evdev involvement. It should show missing SYN_REPORT should such
a driver appear.

> 
> > >> I don't think it's safe to modify last_syn outside of the spin lock.
> > >> This should be done above.
> > >
> > > This is the only writer, plus we are running under event_lock with
> > > interrupts off, so it is safe.
> > 
> > The value will be read concurrently by evdev_fetch_next_event.  So if
> > this were safe, then we wouldn't need the spin lock at all.
> 
> The spinlock ensures atomic read/write of the event buffer. The
> position into the buffer does not need the lock.
> 
> > At the very least for the sake of consistency, I think we should keep
> > the buffer manipulations within the guarded region.
> 
> Sounds reasonable.

OK, we can pull kill_fasync inside spin_lock/unlock pair, it should
change nothing.

Dmitry Torokhov April 5, 2011, 4:38 p.m. UTC | #6

On Mon, Apr 04, 2011 at 05:34:09PM -0700, Jeffrey Brown wrote:
> Dmitry,
> 
> On Mon, Apr 4, 2011 at 3:46 PM, Dmitry Torokhov
> <[e-mail]> wrote:
> > On Mon, Apr 04, 2011 at 03:16:13PM -0700, Jeffrey Brown wrote:
> >> bool full_sync = (type == EV_SYN && code == SYN_REPORT);
> >
> > I am not sure what is cheaper - 2 conditionals or stack manipulation
> > needed to push another argument if we happed to be register-starved.
> 
> Not a question of the computational cost.  It was mostly done to avoid
> repeating the same predicate in multiple places.  This was one of the
> suggested improvements to my earlier patch.
> 
> >> This comment for last_syn is not quite right.  We need last_syn to
> >> refer to the position just beyond the last sync.  Otherwise the device
> >> will not become readable until another event is written there.  The
> >> invariants for last_syn should be similar to those for head.
> >
> > Hm, yes, comment is incorrect. Given this fact I do not like the name
> > anymore either (nor do I like 'end'). Need to think about something
> > better.
> 
> Heh, I faced this very same dilemma.  I tried 'last_sync',
> 'readable_tail', 'read_end', and others before settling on 'end' and a
> descriptive comment.

'packet_head' maybe? Similar to the head but for whole event packet?

> 
> >> Should use client->head here so that the SYN_DROPPED is readable.
> >
> > It is readable, but we do not want to signal on it.
> 
> I think we do want to signal on it.  We should signal whenever the
> device becomes readable.
> 
> Signaling on dropped is useful in the case where a misbehaving device
> driver fails to ever call input_sync.  If that happens, we might
> enqueue a dropped event and then never wake up the client which makes
> the issue hard to diagnose.
> 
> >> I don't think it's safe to modify last_syn outside of the spin lock.
> >> This should be done above.
> >
> > This is the only writer, plus we are running under event_lock with
> > interrupts off, so it is safe.
> 
> The value will be read concurrently by evdev_fetch_next_event.  So if
> this were safe, then we wouldn't need the spin lock at all.

Before we started changing tail to advance to SYN_DROPPED position we
could probably drop the buffer lock and sprinkle memory barriers (to
ensure, for example, that buffer is written before advancing head).

Now we do need to protect buffer and head/tail but the new field can be
updated outside the lock.

> 
> At the very least for the sake of consistency, I think we should keep
> the buffer manipulations within the guarded region.
> 

OK, we can do that too. As I said, we are running with interrupts off
and with even_lock acquired so we can pull update to the new field along
with kill_fasync inside the buffer lock.

Thanks.

Jeff Brown April 11, 2011, 8:15 p.m. UTC | #7

Hi Dmitry,

On Tue, Apr 5, 2011 at 9:38 AM, Dmitry Torokhov
<[e-mail]> wrote:
>> Heh, I faced this very same dilemma.  I tried 'last_sync',
>> 'readable_tail', 'read_end', and others before settling on 'end' and a
>> descriptive comment.
>
> 'packet_head' maybe? Similar to the head but for whole event packet?

That sounds good.  It marks the position one past the end of the most
recent packet, or equivalently the beginning of the next packet (if
there is one).  :-)

> Before we started changing tail to advance to SYN_DROPPED position we
> could probably drop the buffer lock and sprinkle memory barriers (to
> ensure, for example, that buffer is written before advancing head).
>
> Now we do need to protect buffer and head/tail but the new field can be
> updated outside the lock.

That sounds kind of complicated.  Memory barriers and volatile
reads/writers aren't free.  Acquiring / releasing the lock already
performs the necessary memory barriers and keeps the code simple since
we can rely on mutual exclusion guarantees to preserve our invariants.
 I doubt the buffer lock is highly contended so although we could
probably do without it, I'm not sure it's worth the necessary
contortions.  *shrug*

Any further progress on this patch?

Thanks,
Jeff.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index f1c0910..5398761 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -41,6 +41,7 @@  struct evdev {
 struct evdev_client {
 	unsigned int head;
 	unsigned int tail;
+	unsigned int last_syn;	/* position of the last EV_SYN/SYN_REPORT */
 	spinlock_t buffer_lock; /* protects access to buffer, head and tail */
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
@@ -72,12 +73,16 @@  static void evdev_pass_event(struct evdev_client *client,
 		client->buffer[client->tail].type = EV_SYN;
 		client->buffer[client->tail].code = SYN_DROPPED;
 		client->buffer[client->tail].value = 0;
+
+		client->last_syn = client->tail;
 	}
 
 	spin_unlock(&client->buffer_lock);
 
-	if (event->type == EV_SYN)
+	if (event->type == EV_SYN && event->code == SYN_REPORT) {
+		client->last_syn = client->head;
 		kill_fasync(&client->fasync, SIGIO, POLL_IN);
+	}
 }
 
 /*
@@ -387,12 +392,12 @@  static ssize_t evdev_read(struct file *file, char __user *buffer,
 	if (count < input_event_size())
 		return -EINVAL;
 
-	if (client->head == client->tail && evdev->exist &&
+	if (client->last_syn == client->tail && evdev->exist &&
 	    (file->f_flags & O_NONBLOCK))
 		return -EAGAIN;
 
 	retval = wait_event_interruptible(evdev->wait,
-		client->head != client->tail || !evdev->exist);
+		client->last_syn != client->tail || !evdev->exist);
 	if (retval)
 		return retval;
 
@@ -421,7 +426,7 @@  static unsigned int evdev_poll(struct file *file, poll_table *wait)
 	poll_wait(file, &evdev->wait, wait);
 
 	mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
-	if (client->head != client->tail)
+	if (client->last_syn != client->tail)
 		mask |= POLLIN | POLLRDNORM;
 
 	return mask;