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]>
--- 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
> 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.
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
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
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
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 <[email protected]>
--- 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
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 <[email protected]>
Acked-by: Alan Cox <[email protected]>
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]>
---
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;
}
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(-)
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;
}
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 <[email protected]>
Dave
--
http://www.codemonkey.org.uk
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 <[email protected]>
>
>
> --- 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.
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 <[email protected]>
>>
>>
>> --- 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