[PATCH] input: allocate event circular buffer separately

[PATCH] input: allocate event circular buffer separately

[Mika Kuoppala]

If struct evdev_client is added to the already power of two
buffer allocation and the buffer is large, for multitouch devices,
the allocation will spill over into the the next page.
Alloc buffer separately instead of binding it to evdev_client struct
to avoid multipage kmalloc.

Reviewed-by: Andi Shyti <andi.shyti@nokia.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@nokia.com>
---
 drivers/input/evdev.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 6288d7d..c8d4ca3 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -51,7 +51,7 @@ struct evdev_client {
 	struct evdev *evdev;
 	struct list_head node;
 	unsigned int bufsize;
-	struct input_event buffer[];
+	struct input_event *buffer;
 };
 
 static struct evdev *evdev_table[EVDEV_MINORS];
@@ -268,6 +268,7 @@ static int evdev_release(struct inode *inode, struct file *file)
 	evdev_detach_client(evdev, client);
 	if (client->use_wake_lock)
 		wake_lock_destroy(&client->wake_lock);
+	kfree(client->buffer);
 	kfree(client);
 
 	evdev_close_device(evdev);
@@ -309,14 +310,18 @@ static int evdev_open(struct inode *inode, struct file *file)
 
 	bufsize = evdev_compute_buffer_size(evdev->handle.dev);
 
-	client = kzalloc(sizeof(struct evdev_client) +
-				bufsize * sizeof(struct input_event),
-			 GFP_KERNEL);
+	client = kzalloc(sizeof(struct evdev_client), GFP_KERNEL);
 	if (!client) {
 		error = -ENOMEM;
 		goto err_put_evdev;
 	}
 
+	client->buffer = kzalloc(bufsize * sizeof(struct input_event),
+				GFP_KERNEL);
+	if (!client->buffer) {
+		error = -ENOMEM;
+		goto err_free_client;
+	}
 	client->bufsize = bufsize;
 	spin_lock_init(&client->buffer_lock);
 	snprintf(client->name, sizeof(client->name), "%s-%d",
@@ -326,13 +331,15 @@ static int evdev_open(struct inode *inode, struct file *file)
 
 	error = evdev_open_device(evdev);
 	if (error)
-		goto err_free_client;
+		goto err_free_buffer;
 
 	file->private_data = client;
 	nonseekable_open(inode, file);
 
 	return 0;
 
+ err_free_buffer:
+	kfree(client->buffer);
  err_free_client:
 	evdev_detach_client(evdev, client);
 	kfree(client);
-- 
1.7.4.1

Re: [PATCH] input: allocate event circular buffer separately

[Dmitry Torokhov]

Hi Mika,

On Tue, May 15, 2012 at 03:46:24PM +0300, Mika Kuoppala wrote:
> If struct evdev_client is added to the already power of two
> buffer allocation and the buffer is large, for multitouch devices,
> the allocation will spill over into the the next page.
> Alloc buffer separately instead of binding it to evdev_client struct
> to avoid multipage kmalloc.

Not counting the event buffer, size of evdev_client is fairly small
(under 100 bytes?) so I wonder how often this split actually helps (i.e.
buffer and client each are less than page size but combined are more).

Thanks.

-- 
Dmitry

Re: [PATCH] input: allocate event circular buffer separately

[Mika Kuoppala]

On ke, 2012-05-16 at 12:40 -0700, ext Dmitry Torokhov wrote:
> Hi Mika,
> 
> On Tue, May 15, 2012 at 03:46:24PM +0300, Mika Kuoppala wrote:
> > If struct evdev_client is added to the already power of two
> > buffer allocation and the buffer is large, for multitouch devices,
> > the allocation will spill over into the the next page.
> > Alloc buffer separately instead of binding it to evdev_client struct
> > to avoid multipage kmalloc.
> 
> Not counting the event buffer, size of evdev_client is fairly small
> (under 100 bytes?) so I wonder how often this split actually helps (i.e.
> buffer and client each are less than page size but combined are more).

With normal input devices this never happens as the default event buffer
size seem to be 1024 (64 * 16) bytes. But the event buffer size
heuristics for multi touch devices this happens very easily. The
heuristics will look for ABS_MT_TRACKING_ID min and max values to
determine the amount of contacts delivered between two syn events. With
10 contacts and moderate amount of ABS_MT* events between MT_SYNS, you
end up easily to more than a page worth of stuff.

See driver/input/input.c:input_estimate_events_per_packet()

-Mika

Re: [PATCH] input: allocate event circular buffer separately

[Dmitry Torokhov]

On Fri, May 18, 2012 at 11:52:54AM +0300, Mika Kuoppala wrote:
> On ke, 2012-05-16 at 12:40 -0700, ext Dmitry Torokhov wrote:
> > Hi Mika,
> > 
> > On Tue, May 15, 2012 at 03:46:24PM +0300, Mika Kuoppala wrote:
> > > If struct evdev_client is added to the already power of two
> > > buffer allocation and the buffer is large, for multitouch devices,
> > > the allocation will spill over into the the next page.
> > > Alloc buffer separately instead of binding it to evdev_client struct
> > > to avoid multipage kmalloc.
> > 
> > Not counting the event buffer, size of evdev_client is fairly small
> > (under 100 bytes?) so I wonder how often this split actually helps (i.e.
> > buffer and client each are less than page size but combined are more).
> 
> With normal input devices this never happens as the default event buffer
> size seem to be 1024 (64 * 16) bytes. But the event buffer size
> heuristics for multi touch devices this happens very easily. The
> heuristics will look for ABS_MT_TRACKING_ID min and max values to
> determine the amount of contacts delivered between two syn events. With
> 10 contacts and moderate amount of ABS_MT* events between MT_SYNS, you
> end up easily to more than a page worth of stuff.
> 
> See driver/input/input.c:input_estimate_events_per_packet()

That was not my question. I was askig how often it happens that not
only input_estimate_events_per_packet() * <packet size> produces large
value, but it is less than 4K, but close enough to 4K so that
event_client allocated together with the buffer pushes it up over 4K.

Thanks.

-- 
Dmitry