Re: [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

[Antonino A. Daplas]

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@xxxxxx>
>
> --- 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/