Skip to contentSkip to conversation

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
#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
#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.

author
#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
#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

#4 lore raw

PATCH 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]>

---

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;
 }
#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;
 }
author
#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
NAK
#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

author
#8 lore raw
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
Acked-by
#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]>
#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.
#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