lkml.org 
[lkml]   [2005]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
SubjectRe: [new patch] Re: [PATCH] remove redundant NULL checks before kfree() in drivers/video/w100fb.c and add if()+comment back in drivers/video/console/bitblit.c
Date
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 <juhl-lkml@dif.dk>
>
> --- 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);
}


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:11    [W:0.035 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site