use persistent allocation for cursor blinking.

PATCH Superseded Dave Jones 5 participants 12 messages Aug 1, 2006 – Aug 6, 2006

Subsystems (2)

Framebuffer layer Console

lore

Share & tools

lore

Dave Jones

author

2006-08-01 18:56 UTC #0 lore raw

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

drivers/video/console/softcursor.c renamed from drivers/video/console/softcursor.c~ +12 -5

@@ -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;
 }

Signature · 1 line

http://www.codemonkey.org.uk

Roland Dreier

2006-08-01 19:15 UTC #1 lore raw

Quoted 2 lines

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.

Dave Jones

author

2006-08-01 19:31 UTC #2 lore raw

Quoted 6 lines · Roland Dreier

Quoted 2 lines
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

Signature · 1 line

http://www.codemonkey.org.uk

Alan Cox

NAK

2006-08-01 21:58 UTC #7 lore raw

In reply to Dave Jones

Ar Maw, 2006-08-01 am 14:56 -0400, ysgrifennodd Dave Jones:

drivers/video/console/softcursor.c int soft_cursor(struct fb_info *info, st · line 28

Quoted 2 lines

drivers/video/console/softcursor.c renamed from drivers/video/console/softcursor.c~ +2 -0
+	static u8 *src=NULL;
+	static int allocsize=0;

s/=/ = /

drivers/video/console/softcursor.c int soft_cursor(struct fb_info *info, st · line 44

Quoted 9 lines

drivers/video/console/softcursor.c renamed from drivers/video/console/softcursor.c~ +9 -0

+	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

Alan Cox

2006-08-01 22:01 UTC #3 lore raw

In reply to Roland Dreier

Ar Maw, 2006-08-01 am 12:15 -0700, ysgrifennodd Roland Dreier:

Quoted 6 lines

Quoted 2 lines
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

Dave Jones

author

2006-08-01 22:39 UTC #8 lore raw

In reply to Alan Cox

Quoted 5 lines · Alan Cox

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

drivers/video/console/softcursor.c renamed from drivers/video/console/softcursor.c~ +14 -5

@@ -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;
 }

Signature · 1 line

http://www.codemonkey.org.uk

Alan Cox

Acked-by

2006-08-01 22:53 UTC #9 lore raw

Ar Maw, 2006-08-01 am 18:39 -0400, ysgrifennodd Dave Jones:

Quoted 11 lines · Alan Cox

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

Acked-by Alan Cox <[email protected]>

Antonino A. Daplas

2006-08-02 01:22 UTC #4 lore raw

PATCH 1/1 fbcon: Use persistent allocation for cursor blinking.

In reply to Alan Cox

From: Dave Jones <[email protected]>

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

Acked-by Alan Cox <[email protected]>

Signed-off-by Antonino Daplas <[email protected]>

---

Quoted 3 lines · Alan Cox

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

Thanks for doing this.

Quoted 10 lines

Quoted 4 lines
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(-)

drivers/video/console/fbcon.c +3 -0

index 390439b..6165fd9 100644
@@ -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;
 			}

drivers/video/console/fbcon.h +2 -0

index f244ad0..0b73ae9 100644
@@ -80,6 +80,8 @@ struct fbcon_ops {
 	char  *cursor_data;
 	u8    *fontbuffer;
 	u8    *fontdata;
+	u8    *cursor_src;
+	u8     cursor_size;
 	u32    fd_size;
 };
     /*

drivers/video/console/softcursor.c +21 ~~-10~~

index 557c563..7d07d83 100644
@@ -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;
 }

Antonino A. Daplas

2006-08-02 01:28 UTC #5 lore raw

PATCH RESEND 1/1 fbcon: Use persistent allocation for cursor blinking.

From: Dave Jones <[email protected]>

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

Acked-by Alan Cox <[email protected]>

Signed-off-by Antonino Daplas <[email protected]>

---

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(-)

drivers/video/console/fbcon.c +3 -0

index 390439b..6165fd9 100644
@@ -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;
 			}

drivers/video/console/fbcon.h +2 -0

index f244ad0..b9386d1 100644
@@ -80,6 +80,8 @@ struct fbcon_ops {
 	char  *cursor_data;
 	u8    *fontbuffer;
 	u8    *fontdata;
+	u8    *cursor_src;
+	u32    cursor_size;
 	u32    fd_size;
 };
     /*

drivers/video/console/softcursor.c +21 ~~-10~~

index 557c563..7d07d83 100644
@@ -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;
 }

Dave Jones

author

2006-08-02 01:29 UTC #6 lore raw

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

In reply to Antonino A. Daplas

Quoted 4 lines · Antonino A. Daplas

Quoted 4 lines
Quoted 3 lines
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?

Quoted 8 lines

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

Dave

Signature · 1 line

http://www.codemonkey.org.uk

Pavel Machek

2006-08-06 22:00 UTC #10 lore raw

In reply to Dave Jones

Hi!

Quoted 11 lines

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

drivers/video/console/softcursor.c int soft_cursor(struct fb_info *info, st · line 29

Quoted 8 lines

drivers/video/console/softcursor.c renamed from drivers/video/console/softcursor.c~ +3 -1

@@ -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

Signature · 1 line

Thanks for all the (sleeping) penguins.

Antonino A. Daplas

2006-08-06 23:46 UTC #11 lore raw

Quoted 10 lines · Pavel Machek

Hi!
Quoted 8 lines
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 <[email protected]>

Quoted 15 lines

drivers/video/console/softcursor.c int soft_cursor(struct fb_info *info, st · line 29
Quoted 11 lines
drivers/video/console/softcursor.c renamed from drivers/video/console/softcursor.c~ +3 -1
@@ -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