LinuxLists.cc - [PATCH v2 1/4] input: Set default events per packet.

[permalink] [raw]

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

This patch modifies evdev so that it only becomes readable when
the buffer contains an EV_SYN event other than SYN_MT_REPORT.

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: [email protected]
---
drivers/input/evdev.c | 19 ++++++++++++-------
1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index bd78dc0..fc1d907 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -41,6 +41,7 @@ struct evdev {
struct evdev_client {
int head;
int tail;
+ int end; /* index one past the last readable event */
bool drop_packet;
spinlock_t buffer_lock; /* protects access to buffer, head and tail */
struct fasync_struct *fasync;
@@ -74,14 +75,17 @@ static void evdev_pass_event(struct evdev_client *client,
}
cur = client->head++;
client->head &= client->bufsize - 1;
- if (likely(client->head != client->tail))
+ if (likely(client->head != client->tail)) {
client->buffer[cur] = *event;
- else {
+ if (full_sync)
+ client->end = client->head;
+ } else {
client->buffer[cur].time = event->time;
client->buffer[cur].type = EV_SYN;
client->buffer[cur].code = SYN_DROPPED;
client->buffer[cur].value = 0;
client->tail = cur;
+ client->end = client->head;
if (!full_sync) {
client->drop_packet = true;
full_sync = true;
@@ -122,7 +126,8 @@ static void evdev_event(struct input_handle *handle,

rcu_read_unlock();

- wake_up_interruptible(&evdev->wait);
+ if (full_sync)
+ wake_up_interruptible(&evdev->wait);
}

static int evdev_fasync(int fd, struct file *file, int on)
@@ -381,7 +386,7 @@ static int evdev_fetch_next_event(struct evdev_client *client,

spin_lock_irq(&client->buffer_lock);

- have_event = client->head != client->tail;
+ have_event = client->end != client->tail;
if (have_event) {
*event = client->buffer[client->tail++];
client->tail &= client->bufsize - 1;
@@ -403,12 +408,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->end == client->tail && evdev->exist &&
(file->f_flags & O_NONBLOCK))
return -EAGAIN;

retval = wait_event_interruptible(evdev->wait,
- client->head != client->tail || !evdev->exist);
+ client->end != client->tail || !evdev->exist);
if (retval)
return retval;

@@ -437,7 +442,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->end != client->tail)
mask |= POLLIN | POLLRDNORM;

return mask;
--
1.7.0.4

2011-04-02 06:55:22

[permalink] [raw]

Subject: [PATCH v2 3/4] input: evdev: Indicate buffer overrun with SYN_DROPPED.

From: Jeff Brown <[email protected]>

Add a new EV_SYN code, SYN_DROPPED, to inform the client when input
events have been dropped from the evdev input buffer due to a
buffer overrun. All subsequent events for the current packet are
also dropped so that the next packet starts right after SYN_DROPPED.

The client should use SYN_DROPPED as a hint to reset its internal state.

Slightly changed the signal condition when using fasync to send
the signal only for non-MT sync packets such that the signal is
only sent at the end of the packet. This makes the code more
consistent in its handling of packet sync boundaries.

Signed-off-by: [email protected]
---
drivers/input/evdev.c | 45 +++++++++++++++++++++++++++++++++++----------
include/linux/input.h | 1 +
2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 7f42d3a..bd78dc0 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -41,6 +41,7 @@ struct evdev {
struct evdev_client {
int head;
int tail;
+ bool drop_packet;
spinlock_t buffer_lock; /* protects access to buffer, head and tail */
struct fasync_struct *fasync;
struct evdev *evdev;
@@ -53,21 +54,42 @@ static struct evdev *evdev_table[EVDEV_MINORS];
static DEFINE_MUTEX(evdev_table_mutex);

static void evdev_pass_event(struct evdev_client *client,
- struct input_event *event)
+ struct input_event *event, bool full_sync)
{
+ int cur;
+
/*
* Interrupts are disabled, just acquire the lock.
- * Make sure we don't leave with the client buffer
- * "empty" by having client->head == client->tail.
+ * When the client buffer is full, drain the buffer and enqueue a
+ * SYN_DROPPED event to let the client know that events were dropped
+ * and the last packet was incomplete. We then consume all remaining
+ * events from the dropped packet until the next packet begins.
*/
spin_lock(&client->buffer_lock);
- do {
- client->buffer[client->head++] = *event;
- client->head &= client->bufsize - 1;
- } while (client->head == client->tail);
+ if (client->drop_packet) {
+ if (full_sync)
+ client->drop_packet = false;
+ spin_unlock(&client->buffer_lock);
+ return;
+ }
+ cur = client->head++;
+ client->head &= client->bufsize - 1;
+ if (likely(client->head != client->tail))
+ client->buffer[cur] = *event;
+ else {
+ client->buffer[cur].time = event->time;
+ client->buffer[cur].type = EV_SYN;
+ client->buffer[cur].code = SYN_DROPPED;
+ client->buffer[cur].value = 0;
+ client->tail = cur;
+ if (!full_sync) {
+ client->drop_packet = true;
+ full_sync = true;
+ }
+ }
spin_unlock(&client->buffer_lock);

- if (event->type == EV_SYN)
+ if (full_sync)
kill_fasync(&client->fasync, SIGIO, POLL_IN);
}

@@ -80,20 +102,23 @@ static void evdev_event(struct input_handle *handle,
struct evdev *evdev = handle->private;
struct evdev_client *client;
struct input_event event;
+ bool full_sync;

do_gettimeofday(&event.time);
event.type = type;
event.code = code;
event.value = value;

+ full_sync = (type == EV_SYN && code != SYN_MT_REPORT);
+
rcu_read_lock();

client = rcu_dereference(evdev->grab);
if (client)
- evdev_pass_event(client, &event);
+ evdev_pass_event(client, &event, full_sync);
else
list_for_each_entry_rcu(client, &evdev->client_list, node)
- evdev_pass_event(client, &event);
+ evdev_pass_event(client, &event, full_sync);

rcu_read_unlock();

diff --git a/include/linux/input.h b/include/linux/input.h
index f3a7794..71d3651 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -167,6 +167,7 @@ struct input_keymap_entry {
#define SYN_REPORT 0
#define SYN_CONFIG 1
#define SYN_MT_REPORT 2
+#define SYN_DROPPED 3

/*
* Keys and buttons
--
1.7.0.4

2011-04-02 06:55:19

[permalink] [raw]

Subject: [PATCH v2 2/4] hid: hid-input: Remove obsolete default events per packet setting.

From: Jeff Brown <[email protected]>

The hardcoded default is no longer needed because the input core now
calculates an appropriate value when the input device is registered.

Signed-off-by: [email protected]
---
drivers/hid/hid-input.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 33dde87..4b348e0 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -665,10 +665,6 @@ mapped:

input_abs_set_res(input, usage->code,
hidinput_calc_abs_res(field, usage->code));
-
- /* use a larger default input buffer for MT devices */
- if (usage->code == ABS_MT_POSITION_X && input->hint_events_per_packet == 0)
- input_set_events_per_packet(input, 60);
}

if (usage->type == EV_ABS &&
--
1.7.0.4

2011-04-04 21:33:46

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] input: evdev: Indicate buffer overrun with SYN_DROPPED.

Hi Jeff,

On Fri, Apr 01, 2011 at 11:54:18PM -0700, Jeff Brown wrote:
> /*
> * Interrupts are disabled, just acquire the lock.
> - * Make sure we don't leave with the client buffer
> - * "empty" by having client->head == client->tail.
> + * When the client buffer is full, drain the buffer and enqueue a
> + * SYN_DROPPED event to let the client know that events were dropped
> + * and the last packet was incomplete. We then consume all remaining
> + * events from the dropped packet until the next packet begins.

I do not think we (kernel) should be doing this. Clients should take
care and not allow overruns but if they happen the pain should be on the
client to restore the context. It has to query the device to get its
current state and it can drop the events till next sync as well.

Therefore I intend to apply the patch in the form below.

Thanks!

--
Dmitry

Input: evdev - indicate buffer overrun with SYN_DROPPED

From: Jeff Brown <[email protected]>

Add a new EV_SYN code, SYN_DROPPED, to inform the client when input
events have been dropped from the evdev input buffer due to a
buffer overrun. The client should use this event as a hint to
reset its state or ignore all following events until the next
packet begins.

Signed-off-by: Jeff Brown <[email protected]>
[[email protected]: Implement Henrik's suggestion and drop old events in
case of overflow.]
Signed-off-by: Dmitry Torokhov <[email protected]>
---

Documentation/input/event-codes.txt | 6 ++++++
drivers/input/evdev.c | 33 +++++++++++++++++++++------------
include/linux/input.h | 1 +
3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
index d6732a4..e0326a0 100644
--- a/Documentation/input/event-codes.txt
+++ b/Documentation/input/event-codes.txt
@@ -79,6 +79,12 @@ defined only by when they are sent in the evdev event stream.
- Used to synchronize and separate touch events. See the
multi-touch-protocol.txt document for more information.

+* SYN_DROPPED:
+ - Used to indicate buffer overrun in the evdev client's event queue.
+ Client should ignore all events up to and including next SYN_REPORT
+ event and query the device (using EVIOCG* ioctls) to obtain its
+ current state.
+
EV_KEY:
----------
EV_KEY events take the form KEY_<name> or BTN_<name>. For example, KEY_A is used
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 7f42d3a..f1c0910 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -39,13 +39,13 @@ struct evdev {
};

struct evdev_client {
- int head;
- int tail;
+ unsigned int head;
+ unsigned int tail;
spinlock_t buffer_lock; /* protects access to buffer, head and tail */
struct fasync_struct *fasync;
struct evdev *evdev;
struct list_head node;
- int bufsize;
+ unsigned int bufsize;
struct input_event buffer[];
};

@@ -55,16 +55,25 @@ static DEFINE_MUTEX(evdev_table_mutex);
static void evdev_pass_event(struct evdev_client *client,
struct input_event *event)
{
- /*
- * Interrupts are disabled, just acquire the lock.
- * Make sure we don't leave with the client buffer
- * "empty" by having client->head == client->tail.
- */
+ /* Interrupts are disabled, just acquire the lock. */
spin_lock(&client->buffer_lock);
- do {
- client->buffer[client->head++] = *event;
- client->head &= client->bufsize - 1;
- } while (client->head == client->tail);
+
+ client->buffer[client->head++] = *event;
+ 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;
+ }
+
spin_unlock(&client->buffer_lock);

if (event->type == EV_SYN)
diff --git a/include/linux/input.h b/include/linux/input.h
index f3a7794..71d3651 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -167,6 +167,7 @@ struct input_keymap_entry {
#define SYN_REPORT 0
#define SYN_CONFIG 1
#define SYN_MT_REPORT 2
+#define SYN_DROPPED 3

/*
* Keys and buttons

2011-04-04 21:37:09

[permalink] [raw]

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

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 <[email protected]>

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 <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---

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

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;

Thanks.

--
Dmitry

2011-04-04 21:53:02

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] input: evdev: Indicate buffer overrun with SYN_DROPPED.

Hi Dmitry,

On Mon, Apr 4, 2011 at 2:33 PM, Dmitry Torokhov
<[email protected]> wrote:
> On Fri, Apr 01, 2011 at 11:54:18PM -0700, Jeff Brown wrote:
>> + ? ? ?* When the client buffer is full, drain the buffer and enqueue a
>> + ? ? ?* SYN_DROPPED event to let the client know that events were dropped
>> + ? ? ?* and the last packet was incomplete. ?We then consume all remaining
>> + ? ? ?* events from the dropped packet until the next packet begins.
>
> I do not think we (kernel) should be doing this. Clients should take
> care and not allow overruns but if they happen the pain should be on the
> client to restore the context. It has to query the device to get its
> current state and it can drop the events till next sync as well.

That's fine. We should document SYN_DROPPED and mention that the
client should wait until after the next SYN_REPORT to get back in
sync.

> Therefore I intend to apply the patch in the form below.

Looks good to me. It's more like my original patch.

Jeff.

2011-04-04 22:16:55

[permalink] [raw]

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

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

On Mon, Apr 4, 2011 at 2:36 PM, Dmitry Torokhov
<[email protected]> 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.

2011-04-04 22:46:18

[permalink] [raw]

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

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
> <[email protected]> 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.

--
Dmitry

2011-04-05 00:34:51

[permalink] [raw]

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

Dmitry,

On Mon, Apr 4, 2011 at 3:46 PM, Dmitry Torokhov
<[email protected]> 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.

2011-04-05 12:02:37

[permalink] [raw]

Subject: Re: [PATCH v2 3/4] input: evdev: Indicate buffer overrun with SYN_DROPPED.

> Input: evdev - indicate buffer overrun with SYN_DROPPED
>
> From: Jeff Brown <[email protected]>
>
> Add a new EV_SYN code, SYN_DROPPED, to inform the client when input
> events have been dropped from the evdev input buffer due to a
> buffer overrun. The client should use this event as a hint to
> reset its state or ignore all following events until the next
> packet begins.
>
> Signed-off-by: Jeff Brown <[email protected]>
> [[email protected]: Implement Henrik's suggestion and drop old events in
> case of overflow.]
> Signed-off-by: Dmitry Torokhov <[email protected]>
> ---
>
> Documentation/input/event-codes.txt | 6 ++++++
> drivers/input/evdev.c | 33 +++++++++++++++++++++------------
> include/linux/input.h | 1 +
> 3 files changed, 28 insertions(+), 12 deletions(-)
>
>
> diff --git a/Documentation/input/event-codes.txt b/Documentation/input/event-codes.txt
> index d6732a4..e0326a0 100644
> --- a/Documentation/input/event-codes.txt
> +++ b/Documentation/input/event-codes.txt
> @@ -79,6 +79,12 @@ defined only by when they are sent in the evdev event stream.
> - Used to synchronize and separate touch events. See the
> multi-touch-protocol.txt document for more information.
>
> +* SYN_DROPPED:
> + - Used to indicate buffer overrun in the evdev client's event queue.
> + Client should ignore all events up to and including next SYN_REPORT
> + event and query the device (using EVIOCG* ioctls) to obtain its
> + current state.
> +
> EV_KEY:
> ----------
> EV_KEY events take the form KEY_<name> or BTN_<name>. For example, KEY_A is used
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 7f42d3a..f1c0910 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -39,13 +39,13 @@ struct evdev {
> };
>
> struct evdev_client {
> - int head;
> - int tail;
> + unsigned int head;
> + unsigned int tail;
> spinlock_t buffer_lock; /* protects access to buffer, head and tail */
> struct fasync_struct *fasync;
> struct evdev *evdev;
> struct list_head node;
> - int bufsize;
> + unsigned int bufsize;
> struct input_event buffer[];
> };
>
> @@ -55,16 +55,25 @@ static DEFINE_MUTEX(evdev_table_mutex);
> static void evdev_pass_event(struct evdev_client *client,
> struct input_event *event)
> {
> - /*
> - * Interrupts are disabled, just acquire the lock.
> - * Make sure we don't leave with the client buffer
> - * "empty" by having client->head == client->tail.
> - */
> + /* Interrupts are disabled, just acquire the lock. */
> spin_lock(&client->buffer_lock);
> - do {
> - client->buffer[client->head++] = *event;
> - client->head &= client->bufsize - 1;
> - } while (client->head == client->tail);
> +
> + client->buffer[client->head++] = *event;
> + 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;
> + }
> +
> spin_unlock(&client->buffer_lock);
>
> if (event->type == EV_SYN)
> diff --git a/include/linux/input.h b/include/linux/input.h
> index f3a7794..71d3651 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -167,6 +167,7 @@ struct input_keymap_entry {
> #define SYN_REPORT 0
> #define SYN_CONFIG 1
> #define SYN_MT_REPORT 2
> +#define SYN_DROPPED 3
>
> /*
> * Keys and buttons

Acked-by: Henrik Rydberg <[email protected]>

Thanks,
Henrik

2011-04-05 12:03:04

[permalink] [raw]

Subject: Re: [PATCH v2 4/4] input: evdev: Make device readable only when it contains a complete 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.

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

2011-04-05 12:16:22

[permalink] [raw]

Subject: Re: [PATCH v2 1/4] input: Set default events per packet.

On Fri, Apr 01, 2011 at 11:54:16PM -0700, Jeff Brown wrote:
> From: Jeff Brown <[email protected]>
>
> Calculate a default based on the number of ABS axes, REL axes,
> and MT slots for the device during input device registration.
>
> Signed-off-by: [email protected]
> ---

Reviewed-by: Henrik Rydberg <[email protected]>

Thanks,
Henrik

2011-04-05 12:19:56

[permalink] [raw]

Subject: Re: [PATCH v2 2/4] hid: hid-input: Remove obsolete default events per packet setting.

On Fri, Apr 01, 2011 at 11:54:17PM -0700, Jeff Brown wrote:
> From: Jeff Brown <[email protected]>
>
> The hardcoded default is no longer needed because the input core now
> calculates an appropriate value when the input device is registered.
>
> Signed-off-by: [email protected]
> ---

Reviewed-by: Henrik Rydberg <[email protected]>

Thanks,
Henrik

2011-04-05 16:32:48

[permalink] [raw]

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

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

2011-04-05 16:39:04

[permalink] [raw]

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

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
> <[email protected]> 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.

--
Dmitry

2011-04-11 20:15:45