Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S261323AbVCTXFA (ORCPT ); Sun, 20 Mar 2005 18:05:00 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S261326AbVCTXFA (ORCPT ); Sun, 20 Mar 2005 18:05:00 -0500 Received: from smtp-out.hotpop.com ([38.113.3.61]:11491 "EHLO smtp-out.hotpop.com") by vger.kernel.org with ESMTP id S261323AbVCTXEy (ORCPT ); Sun, 20 Mar 2005 18:04:54 -0500 From: "Antonino A. Daplas" Reply-To: adaplas@pol.net To: Jesper Juhl Subject: 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 Date: Mon, 21 Mar 2005 07:04:50 +0800 User-Agent: KMail/1.5.4 Cc: Antonino Daplas , linux-kernel , linux-fbdev-devel@lists.sourceforge.net, Alex Kern , "Ben. Herrenschmidt" , Thomas Bogendoerfer , Helge Deller , Philipp Rumpf , James Simmons , Geert Uytterhoeven , "Eddie C. Dost" , Nicolas Pitre , linux-arm-kernel@lists.arm.linux.org.uk, Andrew Morton , Richard Purdie References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200503210704.50357.adaplas@hotpop.com> X-HotPOP: ----------------------------------------------- Sent By HotPOP.com FREE Email Get your FREE POP email at www.HotPOP.com ----------------------------------------------- Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4266 Lines: 104 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 > > --- 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/