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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
