Checking a pointer for NULL before calling kfree() on it is redundant,
kfree() deals with NULL pointers just fine.
This patch removes such checks from files in drivers/video/
Since this is a fairly trivial change (and the same change made
everywhere) I've just made a single patch for all the files and CC all
authors/maintainers of those files I could find for comments. If spliting
this into one patch pr file is prefered, then I can easily do that as
well.
These are the files being modified :
drivers/video/aty/atyfb_base.c
drivers/video/aty/radeon_base.c
drivers/video/aty/radeon_monitor.c
drivers/video/console/bitblit.c
drivers/video/console/sticore.c
drivers/video/fbmem.c
drivers/video/fbmon.c
drivers/video/igafb.c
drivers/video/pxafb.c
drivers/video/riva/fbdev.c
drivers/video/sa1100fb.c
(please CC me on replies to lists other than linux-kernel)
Signed-off-by: Jesper Juhl <[email protected]>
--- linux-2.6.11-mm4-orig/drivers/video/aty/atyfb_base.c 2005-03-16 15:45:26.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/aty/atyfb_base.c 2005-03-19 22:23:47.000000000 +0100
@@ -3435,8 +3435,7 @@ static int __devinit atyfb_pci_probe(str
err_release_io:
#ifdef __sparc__
- if (par->mmap_map)
- kfree(par->mmap_map);
+ kfree(par->mmap_map);
#else
if (par->ati_regbase)
iounmap(par->ati_regbase);
@@ -3444,7 +3443,7 @@ err_release_io:
iounmap(info->screen_base);
#endif
err_release_mem:
- if(par->aux_start)
+ if (par->aux_start)
release_mem_region(par->aux_start, par->aux_size);
release_mem_region(par->res_start, par->res_size);
@@ -3551,8 +3550,7 @@ static void __devexit atyfb_remove(struc
#endif
#endif
#ifdef __sparc__
- if (par->mmap_map)
- kfree(par->mmap_map);
+ kfree(par->mmap_map);
#endif
if (par->aux_start)
release_mem_region(par->aux_start, par->aux_size);
--- linux-2.6.11-mm4-orig/drivers/video/aty/radeon_base.c 2005-03-16 15:45:26.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/aty/radeon_base.c 2005-03-19 22:24:39.000000000 +0100
@@ -2420,10 +2420,8 @@ static int radeonfb_pci_register (struct
err_unmap_fb:
iounmap(rinfo->fb_base);
err_unmap_rom:
- if (rinfo->mon1_EDID)
- kfree(rinfo->mon1_EDID);
- if (rinfo->mon2_EDID)
- kfree(rinfo->mon2_EDID);
+ kfree(rinfo->mon1_EDID);
+ kfree(rinfo->mon2_EDID);
if (rinfo->mon1_modedb)
fb_destroy_modedb(rinfo->mon1_modedb);
fb_dealloc_cmap(&info->cmap);
@@ -2479,10 +2477,8 @@ static void __devexit radeonfb_pci_unreg
pci_release_regions(pdev);
- if (rinfo->mon1_EDID)
- kfree(rinfo->mon1_EDID);
- if (rinfo->mon2_EDID)
- kfree(rinfo->mon2_EDID);
+ kfree(rinfo->mon1_EDID);
+ kfree(rinfo->mon2_EDID);
if (rinfo->mon1_modedb)
fb_destroy_modedb(rinfo->mon1_modedb);
#ifdef CONFIG_FB_RADEON_I2C
--- linux-2.6.11-mm4-orig/drivers/video/aty/radeon_monitor.c 2005-03-16 15:45:26.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/aty/radeon_monitor.c 2005-03-19 22:25:11.000000000 +0100
@@ -618,11 +618,9 @@ void __devinit radeon_probe_screens(stru
}
}
if (ignore_edid) {
- if (rinfo->mon1_EDID)
- kfree(rinfo->mon1_EDID);
+ kfree(rinfo->mon1_EDID);
rinfo->mon1_EDID = NULL;
- if (rinfo->mon2_EDID)
- kfree(rinfo->mon2_EDID);
+ kfree(rinfo->mon2_EDID);
rinfo->mon2_EDID = NULL;
}
--- linux-2.6.11-mm4-orig/drivers/video/console/bitblit.c 2005-03-16 15:45:26.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-19 22:27:39.000000000 +0100
@@ -199,8 +199,7 @@ static void bit_putcs(struct vc_data *vc
count -= cnt;
}
- if (buf)
- kfree(buf);
+ kfree(buf);
}
static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
@@ -273,8 +272,7 @@ static void bit_cursor(struct vc_data *v
dst = kmalloc(w * vc->vc_font.height, GFP_ATOMIC);
if (!dst)
return;
- if (ops->cursor_data)
- kfree(ops->cursor_data);
+ kfree(ops->cursor_data);
ops->cursor_data = dst;
update_attr(dst, src, attribute, vc);
src = dst;
@@ -321,8 +319,7 @@ static void bit_cursor(struct vc_data *v
if (!mask)
return;
- if (ops->cursor_state.mask)
- kfree(ops->cursor_state.mask);
+ kfree(ops->cursor_state.mask);
ops->cursor_state.mask = mask;
p->cursor_shape = vc->vc_cursor_type;
--- linux-2.6.11-mm4-orig/drivers/video/console/sticore.c 2005-03-16 15:45:26.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/console/sticore.c 2005-03-19 22:35:05.000000000 +0100
@@ -798,11 +798,8 @@ sti_read_rom(int wordmode, struct sti_st
return 1;
out_err:
- if(raw)
- kfree(raw);
- if(cooked)
- kfree(cooked);
-
+ kfree(raw);
+ kfree(cooked);
return 0;
}
--- linux-2.6.11-mm4-orig/drivers/video/fbmem.c 2005-03-16 15:45:26.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/fbmem.c 2005-03-19 22:36:26.000000000 +0100
@@ -446,8 +446,7 @@ int fb_show_logo(struct fb_info *info)
logo_new = kmalloc(fb_logo.logo->width * fb_logo.logo->height,
GFP_KERNEL);
if (logo_new == NULL) {
- if (palette)
- kfree(palette);
+ kfree(palette);
if (saved_pseudo_palette)
info->pseudo_palette = saved_pseudo_palette;
return 0;
@@ -466,12 +465,10 @@ int fb_show_logo(struct fb_info *info)
info->fbops->fb_imageblit(info, &image);
}
- if (palette != NULL)
- kfree(palette);
+ kfree(palette);
if (saved_pseudo_palette != NULL)
info->pseudo_palette = saved_pseudo_palette;
- if (logo_new != NULL)
- kfree(logo_new);
+ kfree(logo_new);
return fb_logo.logo->height;
}
#else
--- linux-2.6.11-mm4-orig/drivers/video/fbmon.c 2005-03-16 15:45:26.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/fbmon.c 2005-03-19 22:36:46.000000000 +0100
@@ -588,8 +588,7 @@ static struct fb_videomode *fb_create_mo
*/
void fb_destroy_modedb(struct fb_videomode *modedb)
{
- if (modedb)
- kfree(modedb);
+ kfree(modedb);
}
static int fb_get_monitor_limits(unsigned char *edid, struct fb_monspecs *specs)
--- linux-2.6.11-mm4-orig/drivers/video/igafb.c 2005-03-02 08:38:10.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/igafb.c 2005-03-19 22:37:27.000000000 +0100
@@ -536,8 +536,7 @@ int __init igafb_init(void)
if (!iga_init(info, par)) {
iounmap((void *)par->io_base);
iounmap(info->screen_base);
- if (par->mmap_map)
- kfree(par->mmap_map);
+ kfree(par->mmap_map);
kfree(info);
}
--- linux-2.6.11-mm4-orig/drivers/video/pxafb.c 2005-03-02 08:38:37.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/pxafb.c 2005-03-19 22:41:51.000000000 +0100
@@ -1343,8 +1343,7 @@ int __init pxafb_probe(struct device *de
failed:
dev_set_drvdata(dev, NULL);
- if (fbi)
- kfree(fbi);
+ kfree(fbi);
return ret;
}
--- linux-2.6.11-mm4-orig/drivers/video/riva/fbdev.c 2005-03-16 15:45:26.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/riva/fbdev.c 2005-03-19 22:43:06.000000000 +0100
@@ -2108,8 +2108,7 @@ static void __exit rivafb_remove(struct
#ifdef CONFIG_FB_RIVA_I2C
riva_delete_i2c_busses(par);
- if (par->EDID)
- kfree(par->EDID);
+ kfree(par->EDID);
#endif
unregister_framebuffer(info);
--- linux-2.6.11-mm4-orig/drivers/video/sa1100fb.c 2005-03-02 08:37:50.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/sa1100fb.c 2005-03-19 22:45:22.000000000 +0100
@@ -1507,8 +1507,7 @@ static int __init sa1100fb_probe(struct
failed:
dev_set_drvdata(dev, NULL);
- if (fbi)
- kfree(fbi);
+ kfree(fbi);
release_mem_region(0xb0100000, 0x10000);
return ret;
}
On Sunday 20 March 2005 06:59, Jesper Juhl wrote:
> Checking a pointer for NULL before calling kfree() on it is redundant,
> kfree() deals with NULL pointers just fine.
> This patch removes such checks from files in drivers/video/
>
> Since this is a fairly trivial change (and the same change made
> everywhere) I've just made a single patch for all the files and CC all
> authors/maintainers of those files I could find for comments. If spliting
> this into one patch pr file is prefered, then I can easily do that as
> well.
>
[snip]
> --- linux-2.6.11-mm4-orig/drivers/video/console/bitblit.c 2005-03-16
> 15:45:26.000000000 +0100 +++
> linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-19
> 22:27:39.000000000 +0100 @@ -199,8 +199,7 @@ static void bit_putcs(struct
> vc_data *vc
> count -= cnt;
> }
>
> - if (buf)
> - kfree(buf);
> + kfree(buf);
> }
>
This is performance critical, so I would like the check to remain. A comment
may be added in this section.
> static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> @@ -273,8 +272,7 @@ static void bit_cursor(struct vc_data *v
> dst = kmalloc(w * vc->vc_font.height, GFP_ATOMIC);
> if (!dst)
> return;
> - if (ops->cursor_data)
> - kfree(ops->cursor_data);
> + kfree(ops->cursor_data);
> ops->cursor_data = dst;
> update_attr(dst, src, attribute, vc);
> src = dst;
> @@ -321,8 +319,7 @@ static void bit_cursor(struct vc_data *v
> if (!mask)
> return;
>
> - if (ops->cursor_state.mask)
> - kfree(ops->cursor_state.mask);
> + kfree(ops->cursor_state.mask);
> ops->cursor_state.mask = mask;
Although these are also performance critical, I will agree that the checks
can go. Very rarely will ops->cursor_state.mask and ops->cursor_data be
NULL.
As for the rest, they are acceptable, as long as the maintainers agree.
Tony
On Mon, 21 Mar 2005, Antonino A. Daplas wrote:
> On Sunday 20 March 2005 06:59, Jesper Juhl wrote:
> > Checking a pointer for NULL before calling kfree() on it is redundant,
> > kfree() deals with NULL pointers just fine.
> > This patch removes such checks from files in drivers/video/
> >
> > Since this is a fairly trivial change (and the same change made
> > everywhere) I've just made a single patch for all the files and CC all
> > authors/maintainers of those files I could find for comments. If spliting
> > this into one patch pr file is prefered, then I can easily do that as
> > well.
> >
>
> [snip]
>
> > --- linux-2.6.11-mm4-orig/drivers/video/console/bitblit.c 2005-03-16
> > 15:45:26.000000000 +0100 +++
> > linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-19
> > 22:27:39.000000000 +0100 @@ -199,8 +199,7 @@ static void bit_putcs(struct
> > vc_data *vc
> > count -= cnt;
> > }
> >
> > - if (buf)
> > - kfree(buf);
> > + kfree(buf);
> > }
> >
>
> This is performance critical, so I would like the check to remain. A comment
> may be added in this section.
The first thing kfree() does is check for the NULL pointer. And since kfree()
is used a lot, it's probably already in the cache.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Mon, 21 Mar 2005, Antonino A. Daplas wrote:
> On Sunday 20 March 2005 06:59, Jesper Juhl wrote:
> > Checking a pointer for NULL before calling kfree() on it is redundant,
> > kfree() deals with NULL pointers just fine.
> > This patch removes such checks from files in drivers/video/
> >
> > Since this is a fairly trivial change (and the same change made
> > everywhere) I've just made a single patch for all the files and CC all
> > authors/maintainers of those files I could find for comments. If spliting
> > this into one patch pr file is prefered, then I can easily do that as
> > well.
> >
>
> [snip]
>
> > --- linux-2.6.11-mm4-orig/drivers/video/console/bitblit.c 2005-03-16
> > 15:45:26.000000000 +0100 +++
> > linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-19
> > 22:27:39.000000000 +0100 @@ -199,8 +199,7 @@ static void bit_putcs(struct
> > vc_data *vc
> > count -= cnt;
> > }
> >
> > - if (buf)
> > - kfree(buf);
> > + kfree(buf);
> > }
> >
>
> This is performance critical, so I would like the check to remain. A comment
> may be added in this section.
>
Ok, I believe Andrew already merged the patch into -mm, if you really want
that check back then I'll send him a patch to put it back and add a
comment once he puts out the next -mm.
But, at the risk of exposing my ignorance, I have to ask if it wouldn't
actually perform better /without/ the if(buf) bit? The reason I say that
is that the generated code shrinks quite a bit when it's removed, and also
kfree() itself does the same NULL check as the very first thing, so it
comes down to the bennefit of shorter generated code, one less branch,
against the overhead of a function call - and how often will 'buf' be
NULL? if buff is != NULL the majority of the time, then it should be a
gain to remove the if().
> > static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
> > @@ -273,8 +272,7 @@ static void bit_cursor(struct vc_data *v
> > dst = kmalloc(w * vc->vc_font.height, GFP_ATOMIC);
> > if (!dst)
> > return;
> > - if (ops->cursor_data)
> > - kfree(ops->cursor_data);
> > + kfree(ops->cursor_data);
> > ops->cursor_data = dst;
> > update_attr(dst, src, attribute, vc);
> > src = dst;
> > @@ -321,8 +319,7 @@ static void bit_cursor(struct vc_data *v
> > if (!mask)
> > return;
> >
> > - if (ops->cursor_state.mask)
> > - kfree(ops->cursor_state.mask);
> > + kfree(ops->cursor_state.mask);
> > ops->cursor_state.mask = mask;
>
> Although these are also performance critical, I will agree that the checks
> can go. Very rarely will ops->cursor_state.mask and ops->cursor_data be
> NULL.
>
> As for the rest, they are acceptable, as long as the maintainers agree.
>
Ok, thank you for commenting.
--
Jesper Juhl
>>...
>
>This is performance critical, so I would like the check to remain. A comment
>may be added in this section.
Hm, if we used Yoshifuji's inline kfree(), you'd get both. A check and a clean
code. (Though I would not move kfree to __kfree, but make the inline variant
explicitly different named.)
Jan Engelhardt
--
On Monday 21 March 2005 06:02, Jesper Juhl wrote:
> On Mon, 21 Mar 2005, Antonino A. Daplas wrote:
> > On Sunday 20 March 2005 06:59, Jesper Juhl wrote:
> > > Checking a pointer for NULL before calling kfree() on it is redundant,
> > > kfree() deals with NULL pointers just fine.
> > > This patch removes such checks from files in drivers/video/
> > >
> > > Since this is a fairly trivial change (and the same change made
> > > everywhere) I've just made a single patch for all the files and CC all
> > > authors/maintainers of those files I could find for comments. If
> > > spliting this into one patch pr file is prefered, then I can easily do
> > > that as well.
> >
> > [snip]
> >
> > > --- linux-2.6.11-mm4-orig/drivers/video/console/bitblit.c 2005-03-16
> > > 15:45:26.000000000 +0100 +++
> > > linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-19
> > > 22:27:39.000000000 +0100 @@ -199,8 +199,7 @@ static void
> > > bit_putcs(struct vc_data *vc
> > > count -= cnt;
> > > }
> > >
> > > - if (buf)
> > > - kfree(buf);
> > > + kfree(buf);
> > > }
> >
> > This is performance critical, so I would like the check to remain. A
> > comment may be added in this section.
>
> Ok, I believe Andrew already merged the patch into -mm, if you really want
> that check back then I'll send him a patch to put it back and add a
> comment once he puts out the next -mm.
> But, at the risk of exposing my ignorance, I have to ask if it wouldn't
> actually perform better /without/ the if(buf) bit? The reason I say that
> is that the generated code shrinks quite a bit when it's removed, and also
> kfree() itself does the same NULL check as the very first thing, so it
> comes down to the bennefit of shorter generated code, one less branch,
> against the overhead of a function call - and how often will 'buf' be
> NULL? if buff is != NULL the majority of the time, then it should be a
> gain to remove the if().
You said it, buf is almost always NULL, except when the driver is in
monochrome mode. So a kfree is rarely done.
Anyway, if the patch is already in the tree, let's leave it at that. I would
surmise that the performance loss is negligible.
Tony
On Monday 21 March 2005 05:49, Geert Uytterhoeven wrote:
> On Mon, 21 Mar 2005, Antonino A. Daplas wrote:
> > On Sunday 20 March 2005 06:59, Jesper Juhl wrote:
> > > Checking a pointer for NULL before calling kfree() on it is redundant,
> > > kfree() deals with NULL pointers just fine.
> > > This patch removes such checks from files in drivers/video/
> > >
> > > Since this is a fairly trivial change (and the same change made
> > > everywhere) I've just made a single patch for all the files and CC all
> > > authors/maintainers of those files I could find for comments. If
> > > spliting this into one patch pr file is prefered, then I can easily do
> > > that as well.
> >
> > [snip]
> >
> > > --- linux-2.6.11-mm4-orig/drivers/video/console/bitblit.c 2005-03-16
> > > 15:45:26.000000000 +0100 +++
> > > linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-19
> > > 22:27:39.000000000 +0100 @@ -199,8 +199,7 @@ static void
> > > bit_putcs(struct vc_data *vc
> > > count -= cnt;
> > > }
> > >
> > > - if (buf)
> > > - kfree(buf);
> > > + kfree(buf);
> > > }
> >
> > This is performance critical, so I would like the check to remain. A
> > comment may be added in this section.
>
> The first thing kfree() does is check for the NULL pointer. And since
> kfree() is used a lot, it's probably already in the cache.
It's not the kfree that matters, but the fact that buf is almost always NULL.
Tony
On Mon, 21 Mar 2005, Antonino A. Daplas wrote:
> On Monday 21 March 2005 06:02, Jesper Juhl wrote:
> > On Mon, 21 Mar 2005, Antonino A. Daplas wrote:
> > > On Sunday 20 March 2005 06:59, Jesper Juhl wrote:
> > > > Checking a pointer for NULL before calling kfree() on it is redundant,
> > > > kfree() deals with NULL pointers just fine.
> > > > This patch removes such checks from files in drivers/video/
> > > >
> > > [snip]
> > >
> > > > --- linux-2.6.11-mm4-orig/drivers/video/console/bitblit.c 2005-03-16
> > > > 15:45:26.000000000 +0100 +++
> > > > linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-19
> > > > 22:27:39.000000000 +0100 @@ -199,8 +199,7 @@ static void
> > > > bit_putcs(struct vc_data *vc
> > > > count -= cnt;
> > > > }
> > > >
> > > > - if (buf)
> > > > - kfree(buf);
> > > > + kfree(buf);
> > > > }
> > >
> > > This is performance critical, so I would like the check to remain. A
> > > comment may be added in this section.
> >
> > Ok, I believe Andrew already merged the patch into -mm, if you really want
> > that check back then I'll send him a patch to put it back and add a
> > comment once he puts out the next -mm.
> > But, at the risk of exposing my ignorance, I have to ask if it wouldn't
> > actually perform better /without/ the if(buf) bit? The reason I say that
> > is that the generated code shrinks quite a bit when it's removed, and also
> > kfree() itself does the same NULL check as the very first thing, so it
> > comes down to the bennefit of shorter generated code, one less branch,
> > against the overhead of a function call - and how often will 'buf' be
> > NULL? if buff is != NULL the majority of the time, then it should be a
> > gain to remove the if().
>
> You said it, buf is almost always NULL, except when the driver is in
> monochrome mode. So a kfree is rarely done.
>
I see, then my change in this exact spot woul probably be a loss in the
general case. Thank you for explaining.
> Anyway, if the patch is already in the tree, let's leave it at that. I would
> surmise that the performance loss is negligible.
>
Well, I just spotted two cases I missed in drivers/video/ , so when I send
that patch I might as well include a hunk that puts this one check back
including a comment as to why it should stay.
--
Jesper
On Sun, 20 Mar 2005, Jesper Juhl wrote:
>
> On Mon, 21 Mar 2005, Antonino A. Daplas wrote:
> > On Monday 21 March 2005 06:02, Jesper Juhl wrote:
> > > On Mon, 21 Mar 2005, Antonino A. Daplas wrote:
> > > > On Sunday 20 March 2005 06:59, Jesper Juhl wrote:
> > > > > Checking a pointer for NULL before calling kfree() on it is redundant,
> > > > > kfree() deals with NULL pointers just fine.
> > > > > This patch removes such checks from files in drivers/video/
> > > > >
> > > > [snip]
> > > >
> > > > > --- linux-2.6.11-mm4-orig/drivers/video/console/bitblit.c 2005-03-16
> > > > > 15:45:26.000000000 +0100 +++
> > > > > linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-19
> > > > > 22:27:39.000000000 +0100 @@ -199,8 +199,7 @@ static void
> > > > > bit_putcs(struct vc_data *vc
> > > > > count -= cnt;
> > > > > }
> > > > >
> > > > > - if (buf)
> > > > > - kfree(buf);
> > > > > + kfree(buf);
> > > > > }
> > > >
> > > > This is performance critical, so I would like the check to remain. A
> > > > comment may be added in this section.
> > >
> > > Ok, I believe Andrew already merged the patch into -mm, if you really want
> > > that check back then I'll send him a patch to put it back and add a
> > > comment once he puts out the next -mm.
> > > But, at the risk of exposing my ignorance, I have to ask if it wouldn't
> > > actually perform better /without/ the if(buf) bit? The reason I say that
> > > is that the generated code shrinks quite a bit when it's removed, and also
> > > kfree() itself does the same NULL check as the very first thing, so it
> > > comes down to the bennefit of shorter generated code, one less branch,
> > > against the overhead of a function call - and how often will 'buf' be
> > > NULL? if buff is != NULL the majority of the time, then it should be a
> > > gain to remove the if().
> >
> > You said it, buf is almost always NULL, except when the driver is in
> > monochrome mode. So a kfree is rarely done.
> >
> I see, then my change in this exact spot woul probably be a loss in the
> general case. Thank you for explaining.
>
> > Anyway, if the patch is already in the tree, let's leave it at that. I would
> > surmise that the performance loss is negligible.
> >
> Well, I just spotted two cases I missed in drivers/video/ , so when I send
> that patch I might as well include a hunk that puts this one check back
> including a comment as to why it should stay.
>
One case turned out not to be one when I took a closer look, so I actually
only missed one. Here's a patch to fix that last one and also put the
check in bitblit.c back.
(Andrew: this should apply on top of what you already merged)
Signed-off-by: Jesper Juhl <[email protected]>
--- linux-2.6.11-mm4/drivers/video/console/bitblit.c~ 2005-03-20 23:40:58.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-20 23:40:58.000000000 +0100
@@ -199,7 +199,11 @@ static void bit_putcs(struct vc_data *vc
count -= cnt;
}
- kfree(buf);
+ /* buf is always NULL except when in monochrome mode, so in this case
+ it's a gain to check buf against NULL even though kfree() handles
+ NULL pointers just fine */
+ if (buf)
+ kfree(buf);
}
static void bit_clear_margins(struct vc_data *vc, struct fb_info *info,
--- linux-2.6.11-mm4-orig/drivers/video/w100fb.c 2005-03-16 15:45:26.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/w100fb.c 2005-03-20 23:33:58.000000000 +0100
@@ -537,10 +537,8 @@ static void w100fb_clear_buffer(void)
{
int i;
for (i = 0; i < W100_BUF_NUM; i++) {
- if (gSaveImagePtr[i] != NULL) {
- kfree(gSaveImagePtr[i]);
- gSaveImagePtr[i] = NULL;
- }
+ kfree(gSaveImagePtr[i]);
+ gSaveImagePtr[i] = NULL;
}
}
On Monday 21 March 2005 06:45, Jesper Juhl wrote:
> On Sun, 20 Mar 2005, Jesper Juhl wrote:
> > On Mon, 21 Mar 2005, Antonino A. Daplas wrote:
> > > On Monday 21 March 2005 06:02, Jesper Juhl wrote:
> > > > On Mon, 21 Mar 2005, Antonino A. Daplas wrote:
> > > > > On Sunday 20 March 2005 06:59, Jesper Juhl wrote:
> > > > > > Checking a pointer for NULL before calling kfree() on it is
> > > > > > redundant, kfree() deals with NULL pointers just fine.
> > > > > > This patch removes such checks from files in drivers/video/
> > > > >
> > > > > [snip]
> > > > >
> > > > > > ---
> > > > > > linux-2.6.11-mm4-orig/drivers/video/console/bitblit.c 2005-03-16
> > > > > > 15:45:26.000000000 +0100 +++
> > > > > > linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-19
> > > > > > 22:27:39.000000000 +0100 @@ -199,8 +199,7 @@ static void
> > > > > > bit_putcs(struct vc_data *vc
> > > > > > count -= cnt;
> > > > > > }
> > > > > >
> > > > > > - if (buf)
> > > > > > - kfree(buf);
> > > > > > + kfree(buf);
> > > > > > }
> > > > >
> > > > > This is performance critical, so I would like the check to remain.
> > > > > A comment may be added in this section.
> > > >
> > > > Ok, I believe Andrew already merged the patch into -mm, if you really
> > > > want that check back then I'll send him a patch to put it back and
> > > > add a comment once he puts out the next -mm.
> > > > But, at the risk of exposing my ignorance, I have to ask if it
> > > > wouldn't actually perform better /without/ the if(buf) bit? The
> > > > reason I say that is that the generated code shrinks quite a bit when
> > > > it's removed, and also kfree() itself does the same NULL check as the
> > > > very first thing, so it comes down to the bennefit of shorter
> > > > generated code, one less branch, against the overhead of a function
> > > > call - and how often will 'buf' be NULL? if buff is != NULL the
> > > > majority of the time, then it should be a gain to remove the if().
> > >
> > > You said it, buf is almost always NULL, except when the driver is in
> > > monochrome mode. So a kfree is rarely done.
> >
> > I see, then my change in this exact spot woul probably be a loss in the
> > general case. Thank you for explaining.
> >
> > > Anyway, if the patch is already in the tree, let's leave it at that. I
> > > would surmise that the performance loss is negligible.
> >
> > Well, I just spotted two cases I missed in drivers/video/ , so when I
> > send that patch I might as well include a hunk that puts this one check
> > back including a comment as to why it should stay.
>
> One case turned out not to be one when I took a closer look, so I actually
> only missed one. Here's a patch to fix that last one and also put the
> check in bitblit.c back.
> (Andrew: this should apply on top of what you already merged)
>
>
> Signed-off-by: Jesper Juhl <[email protected]>
>
> --- linux-2.6.11-mm4/drivers/video/console/bitblit.c~ 2005-03-20
> 23:40:58.000000000 +0100 +++
> linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-20
> 23:40:58.000000000 +0100 @@ -199,7 +199,11 @@ static void bit_putcs(struct
> vc_data *vc
> count -= cnt;
> }
>
> - kfree(buf);
> + /* buf is always NULL except when in monochrome mode, so in this case
> + it's a gain to check buf against NULL even though kfree() handles
> + NULL pointers just fine */
> + if (buf)
> + kfree(buf);
> }
>
As Joe Perch suggested, an if (unlikely(buf)) is better.
Tony
--- linux-2.6.11-mm4/drivers/video/console/bitblit.c~ 2005-03-20 23:40:58.000000000 +0100
+++ linux-2.6.11-mm4/drivers/video/console/bitblit.c 2005-03-20 23:40:58.000000000 +0100
@@ -199,7 +199,11 @@ static void bit_putcs(struct vc_data *vc
count -= cnt;
}
- kfree(buf);
+ /* buf is always NULL except when in monochrome mode, so in this case
+ it's a gain to check buf against NULL even though kfree() handles
+ NULL pointers just fine */
+ if (unlikely(buf))
+ kfree(buf);
}