Re: no need to check for NULL before calling kfree() -fs/ext2/

Re: no need to check for NULL before calling kfree() -fs/ext2/

[Pekka J Enberg]

Hi, 

Jan Engelhardt writes:
> "[...]In general, you should prefer to use actual profile feedback for this 
> (`-fprofile-arcs'), as programmers are NOTORIOUSLY BAD AT PREDICTING how 
> their programs actually perform." --gcc info pages.

Indeed. 

I wrote:
> > The optimization does not help if you are releasing actual memory.

Jan Engelhardt writes:
> It does not turn the real case (releasing memory) worse, but just improves the 
> unreal case (releasing NULL).

You don't know that until you profile! Please note that it _can_ turn the 
real case worse as the generated code will be bigger (assuming we inline 
kfree() to optimize the special case). To summarize: 

 (1) The optimization only helps when the passed pointer is NULL.
 (2) Most of the time, kfree() _should_ be given a real pointer.
     Anything else but sounds quite broken.
 (3) We don't know if inlining kfree() hurts the common case.
 (4) The cleanups Jesper and others are doing are to remove the
     _redundant_ NULL checks (i.e. it is now checked twice). 

Therefore please keep merging the cleanup patches and don't inline kfree() 
unless someone can show a _globally visible_ performance regression (i.e. p% 
slowdown in XYZ benchmark). 

               Pekka 

Re: no need to check for NULL before calling kfree() -fs/ext2/

[Paul Jackson]

Pekka wrote:
>  (4) The cleanups Jesper and others are doing are to remove the
>      _redundant_ NULL checks (i.e. it is now checked twice). 

Even such obvious changes as removing redundant checks doesn't
seem to ensure a performance improvement.  Jesper Juhl posted
performance data for such changes in his microbenchmark a couple
of days ago.

As I posted then, I could swear that his numbers show:

> Just looking at the third run, it seems to me that "if (likely(p))
> kfree(p);" beats a naked "kfree(p);" everytime, whether p is half
> NULL's, or very few NULL's, or almost all NULL's.

Twice now I have asked Jesper to explain this strange result.

I have heard no explanation (not even a terse "you idiot ;)"),
nor anyone else comment on these numbers.

Maybe we should be following your good advice:

> You don't know that until you profile! 

instead of continuing to make these code changes.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: no need to check for NULL before calling kfree() -fs/ext2/

[Pekka J Enberg]

Hi, 

Paul Jackson writes:
> Even such obvious changes as removing redundant checks doesn't
> seem to ensure a performance improvement.  Jesper Juhl posted
> performance data for such changes in his microbenchmark a couple
> of days ago.

It is not a performance issue, it's an API issue. Please note that kfree() 
is analogous libc free() in terms of NULL checking. People are checking NULL 
twice now because they're confused whether kfree() deals it or not. 

Paul Jackson writes:
> Maybe we should be following your good advice: 
> 
> > You don't know that until you profile!  
> 
> instead of continuing to make these code changes.

I am all for profiling but it should not stop us from merging the patches 
because we can restore the generated code with the included (totally 
untested) patch. 

            Pekka 

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
 --- 

Index: 2.6/include/linux/slab.h
===================================================================
 --- 2.6.orig/include/linux/slab.h       2005-03-22 14:31:30.000000000 +0200
+++ 2.6/include/linux/slab.h    2005-03-30 09:08:13.000000000 +0300
@@ -105,8 +105,14 @@
       return __kmalloc(size, flags);
} 

+static inline void kfree(const void * p)
+{
+       if (!p)
+               return;
+       __kfree(p);
+}
+
extern void *kcalloc(size_t, size_t, int);
 -extern void kfree(const void *);
extern unsigned int ksize(const void *); 

extern int FASTCALL(kmem_cache_reap(int));
Index: 2.6/mm/slab.c
===================================================================
 --- 2.6.orig/mm/slab.c  2005-03-22 14:31:31.000000000 +0200
+++ 2.6/mm/slab.c       2005-03-30 09:08:45.000000000 +0300
@@ -2567,13 +2567,11 @@
 * Don't free memory not originally allocated by kmalloc()
 * or you will run into trouble.
 */
 -void kfree (const void *objp)
+void __kfree (const void *objp)
{
       kmem_cache_t *c;
       unsigned long flags; 

 -       if (!objp)
 -               return;
       local_irq_save(flags);
       kfree_debugcheck(objp);
       c = GET_PAGE_CACHE(virt_to_page(objp));
@@ -2581,7 +2579,7 @@
       local_irq_restore(flags);
} 

 -EXPORT_SYMBOL(kfree);
+EXPORT_SYMBOL(__kfree); 

#ifdef CONFIG_SMP
/** 

Re: no need to check for NULL before calling kfree() -fs/ext2/

[Paul Jackson]

Pekka writes:
> It is not a performance issue, it's an API issue.
> ...
> I am all for profiling but it should not stop us from merging the patches 

Ok - sounds right.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@engr.sgi.com> 1.650.933.1373, 1.925.600.0401

Re: no need to check for NULL before calling kfree() -fs/ext2/

[P Lavin]

Hi,
In my wlan driver module, i allocated some memory using kmalloc in 
interrupt context, this one failed but its not returning NULL , so i was 
proceeding further everything was going wrong... & finally the kernel 
crahed. Can any one of you tell me why this is happening ? i cannot use 
GFP_KERNEL because i'm calling this function from interrupt context & it 
may block. Any other solution for this ?? I'm concerned abt why kmalloc 
is not returning null if its not a success ??

Is it not necessary to check for NULL before calling kfree() ??
Regards,
Lavin

Pekka J Enberg wrote:

> Hi,
> Paul Jackson writes:
>
>> Even such obvious changes as removing redundant checks doesn't
>> seem to ensure a performance improvement.  Jesper Juhl posted
>> performance data for such changes in his microbenchmark a couple
>> of days ago.
>
>
> It is not a performance issue, it's an API issue. Please note that 
> kfree() is analogous libc free() in terms of NULL checking. People are 
> checking NULL twice now because they're confused whether kfree() deals 
> it or not.
> Paul Jackson writes:
>
>> Maybe we should be following your good advice:
>> > You don't know that until you profile! 
>> instead of continuing to make these code changes.
>
>
> I am all for profiling but it should not stop us from merging the 
> patches because we can restore the generated code with the included 
> (totally untested) patch.
>            Pekka
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
> Index: 2.6/include/linux/slab.h
> ===================================================================
> --- 2.6.orig/include/linux/slab.h       2005-03-22 14:31:30.000000000 
> +0200
> +++ 2.6/include/linux/slab.h    2005-03-30 09:08:13.000000000 +0300
> @@ -105,8 +105,14 @@
>       return __kmalloc(size, flags);
> }
> +static inline void kfree(const void * p)
> +{
> +       if (!p)
> +               return;
> +       __kfree(p);
> +}
> +
> extern void *kcalloc(size_t, size_t, int);
> -extern void kfree(const void *);
> extern unsigned int ksize(const void *);
> extern int FASTCALL(kmem_cache_reap(int));
> Index: 2.6/mm/slab.c
> ===================================================================
> --- 2.6.orig/mm/slab.c  2005-03-22 14:31:31.000000000 +0200
> +++ 2.6/mm/slab.c       2005-03-30 09:08:45.000000000 +0300
> @@ -2567,13 +2567,11 @@
> * Don't free memory not originally allocated by kmalloc()
> * or you will run into trouble.
> */
> -void kfree (const void *objp)
> +void __kfree (const void *objp)
> {
>       kmem_cache_t *c;
>       unsigned long flags;
> -       if (!objp)
> -               return;
>       local_irq_save(flags);
>       kfree_debugcheck(objp);
>       c = GET_PAGE_CACHE(virt_to_page(objp));
> @@ -2581,7 +2579,7 @@
>       local_irq_restore(flags);
> }
> -EXPORT_SYMBOL(kfree);
> +EXPORT_SYMBOL(__kfree);
> #ifdef CONFIG_SMP
> /**
> -
> 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/

Re: no need to check for NULL before calling kfree() -fs/ext2/

[Jesper Juhl]

On Wed, 30 Mar 2005, P Lavin wrote:

> Date: Wed, 30 Mar 2005 12:45:01 +0530
> From: P Lavin <lavin.p@redpinesignals.com>
> To: linux-kernel@vger.kernel.org
> Subject: Re: no need to check for NULL before calling kfree() -fs/ext2/
> 
> Hi,
> In my wlan driver module, i allocated some memory using kmalloc in interrupt
> context, this one failed but its not returning NULL , 

kmalloc() should always return NULL if the allocation failed.


>so i was proceeding
> further everything was going wrong... & finally the kernel crahed. Can any one
> of you tell me why this is happening ? i cannot use GFP_KERNEL because i'm
> calling this function from interrupt context & it may block. Any other

If you need to allocate memory from interrupt context you should be using 
GFP_ATOMIC (or, if possible, do the allocation earlier in a different 
context).


> solution for this ?? I'm concerned abt why kmalloc is not returning null if
> its not a success ??
> 
I have no explanation for that, are you sure that's really what's 
happening?


> Is it not necessary to check for NULL before calling kfree() ??

No, it is not nessesary to check for NULL before calling kfree() since 
kfree() does    

void kfree (const void *objp)
{
	... 
        if (!objp)
                return;
	...
}

So, if you pass kfree() a NULL pointer it deals with it itself, you don't 
need to check that explicitly before calling kfree() - that's redundant.


-- 
Jesper Juhl

Re: no need to check for NULL before calling kfree() -fs/ext2/

[Jesper Juhl]

On Tue, 29 Mar 2005, Paul Jackson wrote:

> Pekka wrote:
> >  (4) The cleanups Jesper and others are doing are to remove the
> >      _redundant_ NULL checks (i.e. it is now checked twice). 
> 
> Even such obvious changes as removing redundant checks doesn't
> seem to ensure a performance improvement.  Jesper Juhl posted
> performance data for such changes in his microbenchmark a couple
> of days ago.
> 
> As I posted then, I could swear that his numbers show:
> 
> > Just looking at the third run, it seems to me that "if (likely(p))
> > kfree(p);" beats a naked "kfree(p);" everytime, whether p is half
> > NULL's, or very few NULL's, or almost all NULL's.
> 
> Twice now I have asked Jesper to explain this strange result.
> 
I've been kept busy with other things for a while and haven't had the time 
to reply to your emails, sorry.   As I just said in another post I don't 
know how valid my numbers are, but I'll try and craft a few more tests to 
see if I can get some more solid results.

> 
> Maybe we should be following your good advice:
> 
> > You don't know that until you profile! 
> 
> instead of continuing to make these code changes.
> 
I'll gather some more numbers and post them along with any conclusions I 
believe can be drawn from them within a day or two, untill then I'll hold 
back on the patches...


-- 
Jesper Juhl

Re: no need to check for NULL before calling kfree() -fs/ext2/

[P Lavin]

Hi Jesper,
I'm sending this mail to mailing list coz in my company we have some
restrictions on o/g mails, Sorry for that...
Lemme ask u smthing, herez the code
     199     sndpkt = (RSI_sndpkt_t *) RSI_MALLOC(sizeof(RSI_sndpkt_t));
     200     sndpkt->buf_list = (RSI_buf_t *) RSI_MALLOC(sizeof(RSI_buf_t));
Here if malloc fails sndpkt->buf_list should be null right ?? & if i
proceed further ..

     201     sndpkt->buf_list->start_addr = buf;
     202     sndpkt->buf_list->length     = length;
Here itself this should crash right ?? But its not crashing here !!! Wt
was happening was

201 sndpkt->buf_list->start_addr = buf; was not getting initailised & wn
we try to access this variable latter
this was crashing.

Actally i'm not checking for return value from kmalloc thatz a mistake,
I'll fix this but why is it not crashing in line # 201 ??

Jesper Juhl wrote:

>On Wed, 30 Mar 2005, P Lavin wrote:
>
>  
>
>>Date: Wed, 30 Mar 2005 12:45:01 +0530
>>From: P Lavin <lavin.p@redpinesignals.com>
>>To: linux-kernel@vger.kernel.org
>>Subject: Re: no need to check for NULL before calling kfree() -fs/ext2/
>>
>>Hi,
>>In my wlan driver module, i allocated some memory using kmalloc in interrupt
>>context, this one failed but its not returning NULL , 
>>    
>>
>
>kmalloc() should always return NULL if the allocation failed.
>
>
>  
>
>>so i was proceeding
>>further everything was going wrong... & finally the kernel crahed. Can any one
>>of you tell me why this is happening ? i cannot use GFP_KERNEL because i'm
>>calling this function from interrupt context & it may block. Any other
>>    
>>
>
>If you need to allocate memory from interrupt context you should be using 
>GFP_ATOMIC (or, if possible, do the allocation earlier in a different 
>context).
>
>
>  
>
I'm using this flag only, this flag does not guarentee mem allocation,
right ??

>>solution for this ?? I'm concerned abt why kmalloc is not returning null if
>>its not a success ??
>>
>>    
>>
>I have no explanation for that, are you sure that's really what's 
>happening?
>
>
>  
>
I'm not checking this , but my explanation is given above.

>>Is it not necessary to check for NULL before calling kfree() ??
>>    
>>
>
>No, it is not nessesary to check for NULL before calling kfree() since 
>kfree() does    
>
>void kfree (const void *objp)
>{
>	... 
>        if (!objp)
>                return;
>	...
>}
>
>So, if you pass kfree() a NULL pointer it deals with it itself, you don't 
>need to check that explicitly before calling kfree() - that's redundant.
>
>
>  
>

Regs,
Lavin

Re: no need to check for NULL before calling kfree() -fs/ext2/

[Jesper Juhl]

On Wed, 30 Mar 2005, Jesper Juhl wrote:

> On Tue, 29 Mar 2005, Paul Jackson wrote:
> 
> > Pekka wrote:
> > >  (4) The cleanups Jesper and others are doing are to remove the
> > >      _redundant_ NULL checks (i.e. it is now checked twice). 
> > 
> > Even such obvious changes as removing redundant checks doesn't
> > seem to ensure a performance improvement.  Jesper Juhl posted
> > performance data for such changes in his microbenchmark a couple
> > of days ago.
> > 
> > As I posted then, I could swear that his numbers show:
> > 
> > > Just looking at the third run, it seems to me that "if (likely(p))
> > > kfree(p);" beats a naked "kfree(p);" everytime, whether p is half
> > > NULL's, or very few NULL's, or almost all NULL's.
> > 
> > Twice now I have asked Jesper to explain this strange result.
> > 
> I've been kept busy with other things for a while and haven't had the time 
> to reply to your emails, sorry.   As I just said in another post I don't 
> know how valid my numbers are, but I'll try and craft a few more tests to 
> see if I can get some more solid results.
> 
> > 
> > Maybe we should be following your good advice:
> > 
> > > You don't know that until you profile! 
> > 
> > instead of continuing to make these code changes.
> > 
> I'll gather some more numbers and post them along with any conclusions I 
> believe can be drawn from them within a day or two, untill then I'll hold 
> back on the patches...
> 
Ok, I never got around to doing some more benchmarks, mainly since it 
seems that people converged on the oppinion that the kfree() cleanups are 
OK and we can fix up any regressions by inlining kfree if needed (the 
difference these changes make to performance seem to be small and in the 
noice anyway).
If anyone would /like/ me to do more benchmarks, then speak up and I will 
do them - I guess I could also build a kernel with an inline kfree() as a 
comparison.. but, unless someone speaks up I'll just carry on making these 
kfree() cleanups and not bother with benchmarks...


-- 
Jesper