use persistent allocation for cursor blinking.

use persistent allocation for cursor blinking.

[Dave Jones]

Every time the console cursor blinks, we do a kmalloc/kfree pair.
This patch turns that into a single allocation.

This allocation was the most frequent kmalloc I saw on my test box.

Signed-off-by: Dave Jones <davej@redhat.com>


--- linux-2.6.14/drivers/video/console/softcursor.c~	2005-12-28 18:40:08.000000000 -0500
+++ linux-2.6.14/drivers/video/console/softcursor.c	2005-12-28 18:45:50.000000000 -0500
@@ -23,7 +23,9 @@ int soft_cursor(struct fb_info *info, st
 	unsigned int buf_align = info->pixmap.buf_align - 1;
 	unsigned int i, size, dsize, s_pitch, d_pitch;
 	struct fb_image *image;
-	u8 *dst, *src;
+	u8 *dst;
+	static u8 *src=NULL;
+	static int allocsize=0;
 
 	if (info->state != FBINFO_STATE_RUNNING)
 		return 0;
@@ -31,9 +33,15 @@ int soft_cursor(struct fb_info *info, st
 	s_pitch = (cursor->image.width + 7) >> 3;
 	dsize = s_pitch * cursor->image.height;
 
-	src = kmalloc(dsize + sizeof(struct fb_image), GFP_ATOMIC);
-	if (!src)
-		return -ENOMEM;
+	if (dsize + sizeof(struct fb_image) != allocsize) {
+		if (src != NULL)
+			kfree(src);
+		allocsize = dsize + sizeof(struct fb_image);
+
+		src = kmalloc(allocsize, GFP_ATOMIC);
+		if (!src)
+			return -ENOMEM;
+	}
 
 	image = (struct fb_image *) (src + dsize);
 	*image = cursor->image;
@@ -61,7 +69,6 @@ int soft_cursor(struct fb_info *info, st
 	fb_pad_aligned_buffer(dst, d_pitch, src, s_pitch, image->height);
 	image->data = dst;
 	info->fbops->fb_imageblit(info, image);
-	kfree(src);
 	return 0;
 }
 

-- 
http://www.codemonkey.org.uk

Re: use persistent allocation for cursor blinking.

[Roland Dreier]

 > Every time the console cursor blinks, we do a kmalloc/kfree pair.
 > This patch turns that into a single allocation.

A naive question from someone who knows nothing about this subsystem:
is there any possibility of concurrent calls into this function, for
example if there are multiple cursors on a multiheaded system?

 - R.

Re: use persistent allocation for cursor blinking.

[Dave Jones]

On Tue, Aug 01, 2006 at 12:15:35PM -0700, Roland Dreier wrote:
 >  > Every time the console cursor blinks, we do a kmalloc/kfree pair.
 >  > This patch turns that into a single allocation.
 > 
 > A naive question from someone who knows nothing about this subsystem:
 > is there any possibility of concurrent calls into this function, for
 > example if there are multiple cursors on a multiheaded system?

It's all called under the console_sem iirc.

		Dave

-- 
http://www.codemonkey.org.uk

Re: use persistent allocation for cursor blinking.

[Alan Cox]

Ar Maw, 2006-08-01 am 12:15 -0700, ysgrifennodd Roland Dreier:
>  > Every time the console cursor blinks, we do a kmalloc/kfree pair.
>  > This patch turns that into a single allocation.
> 
> A naiive question from someone who knows nothing about this subsystem:
> is there any possibility of concurrent calls into this function, for
> example if there are multiple cursors on a multiheaded system?

We don't do console multihead so its basically OK. Moving all the
console globals into a struct so we can have multiple instances would be
a good thing [tm] and it would make sense for the variable to end up in
said structure if it was done.

Definitely a janitor job there.

Alan

[PATCH] fbcon: Use persistent allocation for cursor blinking.

[Antonino A. Daplas]

From: Dave Jones <davej@redhat.com>

Every time the console cursor blinks, we do a kmalloc/kfree pair.
This patch turns that into a single allocation.

This allocation was the most frequent kmalloc I saw on my test box.

[adaplas]
Per Alan's suggestion, move global variables to fbcon's private structure.
This would also avoid resource leaks when fbcon is unloaded.

Signed-off-by: Dave Jones <davej@redhat.com>
Acked-by: Alan Cox <alan@redhat.com>
Signed-off-by: Antonino Daplas <adaplas@pol.net>
---

Alan Cox wrote:
> Ar Maw, 2006-08-01 am 12:15 -0700, ysgrifennodd Roland Dreier:
>>  > Every time the console cursor blinks, we do a kmalloc/kfree pair.
>>  > This patch turns that into a single allocation.

Thanks for doing this.

>>
>> A naiive question from someone who knows nothing about this subsystem:
>> is there any possibility of concurrent calls into this function, for
>> example if there are multiple cursors on a multiheaded system?
> 
> We don't do console multihead so its basically OK. Moving all the
> console globals into a struct so we can have multiple instances would be
> a good thing [tm] and it would make sense for the variable to end up in
> said structure if it was done.
> 

Here's an update. Taking Alan's cue, I just moved the global variables
to struct fbcon_ops.

Tony


 drivers/video/console/fbcon.c      |    3 +++
 drivers/video/console/fbcon.h      |    2 ++
 drivers/video/console/softcursor.c |   31 +++++++++++++++++++++----------
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index 390439b..6165fd9 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -3225,7 +3225,10 @@ #endif
 			module_put(info->fbops->owner);
 
 			if (info->fbcon_par) {
+				struct fbcon_ops *ops = info->fbcon_par;
+
 				fbcon_del_cursor_timer(info);
+				kfree(ops->cursor_src);
 				kfree(info->fbcon_par);
 				info->fbcon_par = NULL;
 			}
diff --git a/drivers/video/console/fbcon.h b/drivers/video/console/fbcon.h
index f244ad0..0b73ae9 100644
--- a/drivers/video/console/fbcon.h
+++ b/drivers/video/console/fbcon.h
@@ -80,6 +80,8 @@ struct fbcon_ops {
 	char  *cursor_data;
 	u8    *fontbuffer;
 	u8    *fontdata;
+	u8    *cursor_src;
+	u8     cursor_size;
 	u32    fd_size;
 };
     /*
diff --git a/drivers/video/console/softcursor.c b/drivers/video/console/softcursor.c
index 557c563..7d07d83 100644
--- a/drivers/video/console/softcursor.c
+++ b/drivers/video/console/softcursor.c
@@ -20,11 +20,12 @@ #include "fbcon.h"
 
 int soft_cursor(struct fb_info *info, struct fb_cursor *cursor)
 {
+	struct fbcon_ops *ops = info->fbcon_par;
 	unsigned int scan_align = info->pixmap.scan_align - 1;
 	unsigned int buf_align = info->pixmap.buf_align - 1;
 	unsigned int i, size, dsize, s_pitch, d_pitch;
 	struct fb_image *image;
-	u8 *dst, *src;
+	u8 *dst;
 
 	if (info->state != FBINFO_STATE_RUNNING)
 		return 0;
@@ -32,11 +33,19 @@ int soft_cursor(struct fb_info *info, st
 	s_pitch = (cursor->image.width + 7) >> 3;
 	dsize = s_pitch * cursor->image.height;
 
-	src = kmalloc(dsize + sizeof(struct fb_image), GFP_ATOMIC);
-	if (!src)
-		return -ENOMEM;
+	if (dsize + sizeof(struct fb_image) != ops->cursor_size) {
+		if (ops->cursor_src != NULL)
+			kfree(ops->cursor_src);
+		ops->cursor_size = dsize + sizeof(struct fb_image);
 
-	image = (struct fb_image *) (src + dsize);
+		ops->cursor_src = kmalloc(ops->cursor_size, GFP_ATOMIC);
+		if (!ops->cursor_src) {
+			ops->cursor_size = 0;
+			return -ENOMEM;
+		}
+	}
+
+	image = (struct fb_image *) (ops->cursor_src + dsize);
 	*image = cursor->image;
 	d_pitch = (s_pitch + scan_align) & ~scan_align;
 
@@ -48,21 +57,23 @@ int soft_cursor(struct fb_info *info, st
 		switch (cursor->rop) {
 		case ROP_XOR:
 			for (i = 0; i < dsize; i++)
-				src[i] = image->data[i] ^ cursor->mask[i];
+				ops->cursor_src[i] = image->data[i] ^
+					cursor->mask[i];
 			break;
 		case ROP_COPY:
 		default:
 			for (i = 0; i < dsize; i++)
-				src[i] = image->data[i] & cursor->mask[i];
+				ops->cursor_src[i] = image->data[i] &
+					cursor->mask[i];
 			break;
 		}
 	} else
-		memcpy(src, image->data, dsize);
+		memcpy(ops->cursor_src, image->data, dsize);
 
-	fb_pad_aligned_buffer(dst, d_pitch, src, s_pitch, image->height);
+	fb_pad_aligned_buffer(dst, d_pitch, ops->cursor_src, s_pitch,
+			      image->height);
 	image->data = dst;
 	info->fbops->fb_imageblit(info, image);
-	kfree(src);
 	return 0;
 }
 

[PATCH RESEND] fbcon: Use persistent allocation for cursor blinking.

[Antonino A. Daplas]

From: Dave Jones <davej@redhat.com>

Every time the console cursor blinks, we do a kmalloc/kfree pair.
This patch turns that into a single allocation.

This allocation was the most frequent kmalloc I saw on my test box.

[adaplas]
Per Alan's suggestion, move global variables to fbcon's private structure.
This would also avoid resource leaks when fbcon is unloaded.

Signed-off-by: Dave Jones <davej@redhat.com>
Acked-by: Alan Cox <alan@redhat.com>
Signed-off-by: Antonino Daplas <adaplas@pol.net>
---

Oops, I used u8 for cursor_size.

Tony

 drivers/video/console/fbcon.c      |    3 +++
 drivers/video/console/fbcon.h      |    2 ++
 drivers/video/console/softcursor.c |   31 +++++++++++++++++++++----------
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index 390439b..6165fd9 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -3225,7 +3225,10 @@ #endif
 			module_put(info->fbops->owner);
 
 			if (info->fbcon_par) {
+				struct fbcon_ops *ops = info->fbcon_par;
+
 				fbcon_del_cursor_timer(info);
+				kfree(ops->cursor_src);
 				kfree(info->fbcon_par);
 				info->fbcon_par = NULL;
 			}
diff --git a/drivers/video/console/fbcon.h b/drivers/video/console/fbcon.h
index f244ad0..b9386d1 100644
--- a/drivers/video/console/fbcon.h
+++ b/drivers/video/console/fbcon.h
@@ -80,6 +80,8 @@ struct fbcon_ops {
 	char  *cursor_data;
 	u8    *fontbuffer;
 	u8    *fontdata;
+	u8    *cursor_src;
+	u32    cursor_size;
 	u32    fd_size;
 };
     /*
diff --git a/drivers/video/console/softcursor.c b/drivers/video/console/softcursor.c
index 557c563..7d07d83 100644
--- a/drivers/video/console/softcursor.c
+++ b/drivers/video/console/softcursor.c
@@ -20,11 +20,12 @@ #include "fbcon.h"
 
 int soft_cursor(struct fb_info *info, struct fb_cursor *cursor)
 {
+	struct fbcon_ops *ops = info->fbcon_par;
 	unsigned int scan_align = info->pixmap.scan_align - 1;
 	unsigned int buf_align = info->pixmap.buf_align - 1;
 	unsigned int i, size, dsize, s_pitch, d_pitch;
 	struct fb_image *image;
-	u8 *dst, *src;
+	u8 *dst;
 
 	if (info->state != FBINFO_STATE_RUNNING)
 		return 0;
@@ -32,11 +33,19 @@ int soft_cursor(struct fb_info *info, st
 	s_pitch = (cursor->image.width + 7) >> 3;
 	dsize = s_pitch * cursor->image.height;
 
-	src = kmalloc(dsize + sizeof(struct fb_image), GFP_ATOMIC);
-	if (!src)
-		return -ENOMEM;
+	if (dsize + sizeof(struct fb_image) != ops->cursor_size) {
+		if (ops->cursor_src != NULL)
+			kfree(ops->cursor_src);
+		ops->cursor_size = dsize + sizeof(struct fb_image);
 
-	image = (struct fb_image *) (src + dsize);
+		ops->cursor_src = kmalloc(ops->cursor_size, GFP_ATOMIC);
+		if (!ops->cursor_src) {
+			ops->cursor_size = 0;
+			return -ENOMEM;
+		}
+	}
+
+	image = (struct fb_image *) (ops->cursor_src + dsize);
 	*image = cursor->image;
 	d_pitch = (s_pitch + scan_align) & ~scan_align;
 
@@ -48,21 +57,23 @@ int soft_cursor(struct fb_info *info, st
 		switch (cursor->rop) {
 		case ROP_XOR:
 			for (i = 0; i < dsize; i++)
-				src[i] = image->data[i] ^ cursor->mask[i];
+				ops->cursor_src[i] = image->data[i] ^
+					cursor->mask[i];
 			break;
 		case ROP_COPY:
 		default:
 			for (i = 0; i < dsize; i++)
-				src[i] = image->data[i] & cursor->mask[i];
+				ops->cursor_src[i] = image->data[i] &
+					cursor->mask[i];
 			break;
 		}
 	} else
-		memcpy(src, image->data, dsize);
+		memcpy(ops->cursor_src, image->data, dsize);
 
-	fb_pad_aligned_buffer(dst, d_pitch, src, s_pitch, image->height);
+	fb_pad_aligned_buffer(dst, d_pitch, ops->cursor_src, s_pitch,
+			      image->height);
 	image->data = dst;
 	info->fbops->fb_imageblit(info, image);
-	kfree(src);
 	return 0;
 }
 

Re: [PATCH] fbcon: Use persistent allocation for cursor blinking.

[Dave Jones]

On Wed, Aug 02, 2006 at 09:20:49AM +0800, Antonino A. Daplas wrote:

 > >> A naiive question from someone who knows nothing about this subsystem:
 > >> is there any possibility of concurrent calls into this function, for
 > >> example if there are multiple cursors on a multiheaded system?
 > > 
 > > We don't do console multihead so its basically OK. Moving all the
 > > console globals into a struct so we can have multiple instances would be
 > > a good thing [tm] and it would make sense for the variable to end up in
 > > said structure if it was done.
 > > 
 > 
 > Here's an update. Taking Alan's cue, I just moved the global variables
 > to struct fbcon_ops.

Good job.  Have another..

Signed-off-by: Dave Jones <davej@redhat.com>

		Dave

-- 
http://www.codemonkey.org.uk

Re: use persistent allocation for cursor blinking.

[Alan Cox]

Ar Maw, 2006-08-01 am 14:56 -0400, ysgrifennodd Dave Jones:
> +	static u8 *src=NULL;
> +	static int allocsize=0;

s/=/ = /

> +	if (dsize + sizeof(struct fb_image) != allocsize) {
> +		if (src != NULL)
> +			kfree(src);
> +		allocsize = dsize + sizeof(struct fb_image);
> +
> +		src = kmalloc(allocsize, GFP_ATOMIC);
> +		if (!src)
> +			return -ENOMEM;
> +	}

If the allocation fails we have allocsize = "somesize" and src = NULL.
The next time we enter the if is false and we fall through and Oops

Either check src in the if or set allocsize to something impossible (eg
0) on the error path.

NAK

Re: use persistent allocation for cursor blinking.

[Dave Jones]

On Tue, Aug 01, 2006 at 11:17:40PM +0100, Alan Cox wrote:

 > If the allocation fails we have allocsize = "somesize" and src = NULL.
 > The next time we enter the if is false and we fall through and Oops
 > 
 > Either check src in the if or set allocsize to something impossible (eg
 > 0) on the error path.

Good catch.

Signed-off-by: Dave Jones <davej@redhat.com>

--- linux-2.6/drivers/video/console/softcursor.c~	2005-12-28 18:40:08.000000000 -0500
+++ linux-2.6/drivers/video/console/softcursor.c	2005-12-28 18:45:50.000000000 -0500
@@ -23,7 +23,9 @@ int soft_cursor(struct fb_info *info, st
 	unsigned int buf_align = info->pixmap.buf_align - 1;
 	unsigned int i, size, dsize, s_pitch, d_pitch;
 	struct fb_image *image;
-	u8 *dst, *src;
+	u8 *dst;
+	static u8 *src=NULL;
+	static int allocsize = 0;
 
 	if (info->state != FBINFO_STATE_RUNNING)
 		return 0;
@@ -31,9 +33,17 @@ int soft_cursor(struct fb_info *info, st
 	s_pitch = (cursor->image.width + 7) >> 3;
 	dsize = s_pitch * cursor->image.height;
 
-	src = kmalloc(dsize + sizeof(struct fb_image), GFP_ATOMIC);
-	if (!src)
-		return -ENOMEM;
+	if (dsize + sizeof(struct fb_image) != allocsize) {
+		if (src != NULL)
+			kfree(src);
+		allocsize = dsize + sizeof(struct fb_image);
+
+		src = kmalloc(allocsize, GFP_ATOMIC);
+		if (!src) {
+			allocsize = 0;
+			return -ENOMEM;
+		}
+	}
 
 	image = (struct fb_image *) (src + dsize);
 	*image = cursor->image;
@@ -61,7 +69,6 @@ int soft_cursor(struct fb_info *info, st
 	fb_pad_aligned_buffer(dst, d_pitch, src, s_pitch, image->height);
 	image->data = dst;
 	info->fbops->fb_imageblit(info, image);
-	kfree(src);
 	return 0;
 }
 

-- 
http://www.codemonkey.org.uk

Re: use persistent allocation for cursor blinking.

[Alan Cox]

Ar Maw, 2006-08-01 am 18:39 -0400, ysgrifennodd Dave Jones:
> On Tue, Aug 01, 2006 at 11:17:40PM +0100, Alan Cox wrote:
> 
>  > If the allocation fails we have allocsize = "somesize" and src = NULL.
>  > The next time we enter the if is false and we fall through and Oops
>  > 
>  > Either check src in the if or set allocsize to something impossible (eg
>  > 0) on the error path.
> 
> Good catch.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>

Acked-by: Alan Cox <alan@redhat.com>

Re: use persistent allocation for cursor blinking.

[Pavel Machek]

Hi!

> Every time the console cursor blinks, we do a kmalloc/kfree pair.
> This patch turns that into a single allocation.
> 
> This allocation was the most frequent kmalloc I saw on my test box.
> 
> Signed-off-by: Dave Jones <davej@redhat.com>
> 
> 
> --- linux-2.6.14/drivers/video/console/softcursor.c~	2005-12-28 18:40:08.000000000 -0500
> +++ linux-2.6.14/drivers/video/console/softcursor.c	2005-12-28 18:45:50.000000000 -0500
> @@ -23,7 +23,9 @@ int soft_cursor(struct fb_info *info, st
>  	unsigned int buf_align = info->pixmap.buf_align - 1;
>  	unsigned int i, size, dsize, s_pitch, d_pitch;
>  	struct fb_image *image;
> -	u8 *dst, *src;
> +	u8 *dst;
> +	static u8 *src=NULL;
> +	static int allocsize=0;
>  

Spaces around = ? And perhaps it does not need to be initialized when
it is static?

							Pavel
-- 
Thanks for all the (sleeping) penguins.

Re: use persistent allocation for cursor blinking.

[Antonino A. Daplas]

Pavel Machek wrote:
> Hi!
> 
>> Every time the console cursor blinks, we do a kmalloc/kfree pair.
>> This patch turns that into a single allocation.
>>
>> This allocation was the most frequent kmalloc I saw on my test box.
>>
>> Signed-off-by: Dave Jones <davej@redhat.com>
>>
>>
>> --- linux-2.6.14/drivers/video/console/softcursor.c~	2005-12-28 18:40:08.000000000 -0500
>> +++ linux-2.6.14/drivers/video/console/softcursor.c	2005-12-28 18:45:50.000000000 -0500
>> @@ -23,7 +23,9 @@ int soft_cursor(struct fb_info *info, st
>>  	unsigned int buf_align = info->pixmap.buf_align - 1;
>>  	unsigned int i, size, dsize, s_pitch, d_pitch;
>>  	struct fb_image *image;
>> -	u8 *dst, *src;
>> +	u8 *dst;
>> +	static u8 *src=NULL;
>> +	static int allocsize=0;
>>  
> 
> Spaces around = ? And perhaps it does not need to be initialized when
> it is static?
> 

I already sent an updated patch, so your concerns are gone.

Tony