Re: kfree(0) - ok?

Re: kfree(0) - ok?

[Robert Hancock]

Tim Bird wrote:
> Hi all,
> 
> I have a quick question.
> 
> I'm trying to resurrect a patch from the Linux-tiny patch suite,
> to do accounting of kmalloc memory allocations.  In testing it
> with Linux 2.6.22, I've found a large number of kfrees of
> NULL pointers.
> 
> Is this considered OK?  Or should I examine the offenders
> to see if something is coded badly?

It's perfectly correct to do it - though, if it's done very frequently 
in certain cases, it might be more efficient to check for null before 
the kfree, to avoid the function call overhead into kfree..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

kfree(0) - ok?

[Tim Bird]

Hi all,

I have a quick question.

I'm trying to resurrect a patch from the Linux-tiny patch suite,
to do accounting of kmalloc memory allocations.  In testing it
with Linux 2.6.22, I've found a large number of kfrees of
NULL pointers.

Is this considered OK?  Or should I examine the offenders
to see if something is coded badly?

Thanks,
 -- Tim

=============================
Tim Bird
Architecture Group Chair, CE Linux Forum
Senior Staff Engineer, Sony Corporation of America
=============================

Re: kfree(0) - ok?

[Arjan van de Ven]

On Tue, 2007-08-14 at 15:59 -0700, Tim Bird wrote:
> Hi all,
> 
> I have a quick question.
> 
> I'm trying to resurrect a patch from the Linux-tiny patch suite,
> to do accounting of kmalloc memory allocations.  In testing it
> with Linux 2.6.22, I've found a large number of kfrees of
> NULL pointers.
> 
> Is this considered OK?  Or should I examine the offenders
> to see if something is coded badly?

kfree(NULL) is explicitly ok and it saves code size to not check
anywhere
(the idea is that kfree(kmalloc(...)); is a guaranteed safe nop)

NULL is not 0 though.

Re: kfree(0) - ok?

[Jason Uhlenkott]

On Tue, Aug 14, 2007 at 15:55:48 -0700, Arjan van de Ven wrote:
> NULL is not 0 though.

It is.  Its representation isn't guaranteed to be all-bits-zero, but
the constant value 0 when used in pointer context is always a null
pointer (and in fact the standard requires that NULL be #defined as 0
or a cast thereof).

Re: kfree(0) - ok?

[Jan Engelhardt]

On Aug 14 2007 16:21, Jason Uhlenkott wrote:
>On Tue, Aug 14, 2007 at 15:55:48 -0700, Arjan van de Ven wrote:
>> NULL is not 0 though.
>
>It is.  Its representation isn't guaranteed to be all-bits-zero,

C guarantees that.

>but the constant value 0 when used in pointer context is always a
>null pointer (and in fact the standard requires that NULL be
>#defined as 0 or a cast thereof).
>

	Jan
-- 

Re: kfree(0) - ok?

[Rene Herman]

On 08/15/2007 09:28 AM, Jan Engelhardt wrote:

> On Aug 14 2007 16:21, Jason Uhlenkott wrote:

>> On Tue, Aug 14, 2007 at 15:55:48 -0700, Arjan van de Ven wrote:
>>> NULL is not 0 though.
>> It is.  Its representation isn't guaranteed to be all-bits-zero,
> 
> C guarantees that.

C guarantees what? If you're disagreeing with Jason -- he's right.

>> but the constant value 0 when used in pointer context is always a
>> null pointer (and in fact the standard requires that NULL be
>> #defined as 0 or a cast thereof).

Rene.

Re: kfree(0) - ok?

[Jan Engelhardt]

On Aug 15 2007 10:37, Rene Herman wrote:
> On 08/15/2007 09:28 AM, Jan Engelhardt wrote:
>> On Aug 14 2007 16:21, Jason Uhlenkott wrote:
>
>> > On Tue, Aug 14, 2007 at 15:55:48 -0700, Arjan van de Ven wrote:
>> > > NULL is not 0 though.
>> > It is.  Its representation isn't guaranteed to be all-bits-zero,
>> 
>> C guarantees that.
>
> C guarantees what? If you're disagreeing with Jason -- he's right.

http://coding.derkeiler.com/Archive/C_CPP/comp.lang.c/2003-11/1808.html

>> > but the constant value 0 when used in pointer context is always a
>> > null pointer (and in fact the standard requires that NULL be
>> > #defined as 0 or a cast thereof).


	Jan
-- 

Re: kfree(0) - ok?

[Jason Uhlenkott]

On Wed, Aug 15, 2007 at 11:20:33 +0200, Jan Engelhardt wrote:
> 
> On Aug 15 2007 10:37, Rene Herman wrote:
> > On 08/15/2007 09:28 AM, Jan Engelhardt wrote:
> >> On Aug 14 2007 16:21, Jason Uhlenkott wrote:
> >
> >> > On Tue, Aug 14, 2007 at 15:55:48 -0700, Arjan van de Ven wrote:
> >> > > NULL is not 0 though.
> >> > It is.  Its representation isn't guaranteed to be all-bits-zero,
> >> 
> >> C guarantees that.
> >
> > C guarantees what? If you're disagreeing with Jason -- he's right.
> 
> http://coding.derkeiler.com/Archive/C_CPP/comp.lang.c/2003-11/1808.html

That's about representation of integers types.  Pointer types are
another matter.

C99 sections 6.2.5 and 6.2.6 cover this.

This is all just academic language lawyering, of course.  Any machine
on which a pointer isn't NULL after being memset to 0 has serious
quality of implementation issues.

Re: kfree(0) - ok?

[Rene Herman]

On 08/15/2007 11:20 AM, Jan Engelhardt wrote:

> On Aug 15 2007 10:37, Rene Herman wrote:
>> On 08/15/2007 09:28 AM, Jan Engelhardt wrote:
>>> On Aug 14 2007 16:21, Jason Uhlenkott wrote:
>>>> On Tue, Aug 14, 2007 at 15:55:48 -0700, Arjan van de Ven wrote:

>>>>> NULL is not 0 though.
>>>> 
>>>> It is.  Its representation isn't guaranteed to be all-bits-zero,
>>> 
>>> C guarantees that.
>> 
>> C guarantees what? If you're disagreeing with Jason -- he's right.
> 
> http://coding.derkeiler.com/Archive/C_CPP/comp.lang.c/2003-11/1808.html

He said the null _pointer_ isn't guaranteed to be all-bits zero. And it 
isn't. Read the standard or the faq.

>>>> but the constant value 0 when used in pointer context is always a
>>>> null pointer (and in fact the standard requires that NULL be
>>>> #defined as 0 or a cast thereof).

Rene.

Re: kfree(0) - ok?

[Jan Engelhardt]

On Aug 15 2007 11:58, Rene Herman wrote:
>> > > > > NULL is not 0 though.
>> > > > It is.  Its representation isn't guaranteed to be all-bits-zero,
>> > > 
>
> He said the null _pointer_ isn't guaranteed to be all-bits zero. And it
> isn't. Read the standard or the faq.

0 is all-bits-zero.
NULL is 0. ("It is.", above)

Transitively, this would make NULL all-bits-zero.
I might have missed something, though, perhaps that the cast to void* makes it
intransitive.
But leave it at whatever the standard says.

>> > > > but the constant value 0 when used in pointer context is always a
>> > > > null pointer (and in fact the standard requires that NULL be
>> > > > #defined as 0 or a cast thereof).

	Jan
-- 

Re: kfree(0) - ok?

[Rene Herman]

On 08/15/2007 12:20 PM, Jan Engelhardt wrote:

> On Aug 15 2007 11:58, Rene Herman wrote:
>>>>>>> NULL is not 0 though.
>>>>>> It is.  Its representation isn't guaranteed to be all-bits-zero,

>> He said the null _pointer_ isn't guaranteed to be all-bits zero. And it
>> isn't. Read the standard or the faq.
> 
> 0 is all-bits-zero.
> NULL is 0. ("It is.", above)

NULL is a define. It does not have a binary presentation. He was talking 
about the representation of the null pointer, not of 0.

Retarded language lawyering over in comp.lang.c please where all the other 
people who think they just have to be brilliant for having been able to read 
a standards documents go to wank off.

Rene.

Re: kfree(0) - ok?

[Kyle Moffett]

On Aug 15, 2007, at 06:20:27, Jan Engelhardt wrote:
> On Aug 15 2007 11:58, Rene Herman wrote:
>>>> NULL is not 0 though.
>>> It is.  Its representation isn't guaranteed to be all-bits-zero,
>>
>> He said the null _pointer_ isn't guaranteed to be all-bits zero.  
>> And it isn't. Read the standard or the faq.
>
> 0 is all-bits-zero.
> NULL is 0. ("It is.", above)
>
> Transitively, this would make NULL all-bits-zero.  I might have  
> missed something, though, perhaps that the cast to void* makes it  
> intransitive.  But leave it at whatever the standard says.
>
>> but the constant value 0 when used in pointer context is always a  
>> null pointer (and in fact the standard requires that NULL be  
>> #defined as 0 or a cast thereof).

Irrespective of whatever the standard says, EVERY platform and  
compiler anybody makes nowadays has a NULL pointer value with all  
bits clear.  Theoretically the standard allows otherwise, but such a  
decision would break so much code.  Linux especially, we rely on the  
uninitialized data to have all bits clear and we depend on that  
producing NULL pointers; if a NULL pointer was not bitwise exactly 0  
then the test "if (some_ptr != NULL)" would fail and we would start  
dereferencing garbage.

Cheers,
Kyle Moffett

Re: kfree(0) - ok?

[Jan Engelhardt]

On Aug 15 2007 09:58, Kyle Moffett wrote:
>
> Irrespective of whatever the standard says, EVERY platform and
> compiler anybody makes nowadays has a NULL pointer value with all
> bits clear.  Theoretically the standard allows otherwise, but such
> a decision would break so much code.  Linux especially, we rely on
> the uninitialized data to have all bits clear and we depend on that
> producing NULL pointers; if a NULL pointer was not bitwise exactly
> 0 then the test "if (some_ptr != NULL)" would fail and we would
> start dereferencing garbage.

But if kmalloc returns NULL on failure, then testing for NULL
(irrespective of being 0 or 0xDEADBEEF) is ok.
What would actually concern me then is what "if (!some_ptr)" would do.
Probably not the right thing.


	Jan
-- 

Re: kfree(0) - ok?

[Kyle Moffett]

On Aug 15, 2007, at 10:06:49, Jan Engelhardt wrote:
> On Aug 15 2007 09:58, Kyle Moffett wrote:
>> Irrespective of whatever the standard says, EVERY platform and  
>> compiler anybody makes nowadays has a NULL pointer value with all  
>> bits clear.  Theoretically the standard allows otherwise, but such  
>> a decision would break so much code.  Linux especially, we rely on  
>> the uninitialized data to have all bits clear and we depend on  
>> that producing NULL pointers; if a NULL pointer was not bitwise  
>> exactly 0 then the test "if (some_ptr != NULL)" would fail and we  
>> would start dereferencing garbage.
>
> But if kmalloc returns NULL on failure, then testing for NULL  
> (irrespective of being 0 or 0xDEADBEEF) is ok.  What would actually  
> concern me then is what "if (!some_ptr)" would do. Probably not the  
> right thing.

Well, what I was referring to is:

static struct foo *some_ptr;

/* Assumes that $SOME_LOCK is held */
int initialize_foo_module()
{
	if (!some_ptr) {
		some_ptr = kmalloc(sizeof(*some_ptr));
		if (!some_ptr)
			return -ENOMEM;

		/* ... */
	}

	/* ... */
}


We initialize all of the static data to all-bits-clear zeros during  
kernel init.  Any platform on which the binary representations of  
"(unsigned long)0" and "(void *)0" are different (even in length, due  
to other issues) will not run the Linux kernel as it stands today.

And as to the sizeof(unsigned long) == sizeof(void *) issue, please  
remember that every Linux compiler is either ILP32 (int, long, and  
pointer are 32-bit) or LP64 (int is 32-bit and long/pointer are 64- 
bit).  We sort of fundamentally rely on these properties in code all  
over the place.

So yes the Linux kernel "breaks the standard" in a bunch of places,  
but on the other hand we're not your average software and we don't  
have to worry about building on an LLP64 compiler (Windows 64-bit and  
some UNIXes) or other strangeness.

Cheers,
Kyle Moffett

Re: kfree(0) - ok?

[H. Peter Anvin]

Jan Engelhardt wrote:
> 0 is all-bits-zero.
> NULL is 0. ("It is.", above)
> 
> Transitively, this would make NULL all-bits-zero.
> I might have missed something, though, perhaps that the cast to void* makes it
> intransitive.

It does -- a cast from integer to pointer isn't required to be a bitwise
noop.  Machines on which NULL isn't bitwise zero do exist, and the C
standard allows them.  What is almost certainly more common than "all
bits zero is not a NULL pointer" is "any NULL pointer is not necessarily
all bits zero"; there are quite a few machines on which there are
nonzero bitpatterns that are still valid NULL pointers, but an all-zero
memset() will still produce NULL pointers.

However, the particular supersets of the C standard that gcc provides
and which the kernel are written in (the latter being a proper subset of
the former) does not.

	-hpa

Re: kfree(0) - ok?

[Jason Uhlenkott]

On Wed, Aug 15, 2007 at 09:28:54 +0200, Jan Engelhardt wrote:
> 
> On Aug 14 2007 16:21, Jason Uhlenkott wrote:
> >On Tue, Aug 14, 2007 at 15:55:48 -0700, Arjan van de Ven wrote:
> >> NULL is not 0 though.
> >
> >It is.  Its representation isn't guaranteed to be all-bits-zero,
> 
> C guarantees that.

Equality with the expression 0 is guaranteed.  Representation isn't.

http://www.c-faq.com/null/varieties.html

Re: kfree(0) - ok?

[Andreas Schwab]

Jan Engelhardt <jengelh@computergmbh.de> writes:

> On Aug 14 2007 16:21, Jason Uhlenkott wrote:
>>On Tue, Aug 14, 2007 at 15:55:48 -0700, Arjan van de Ven wrote:
>>> NULL is not 0 though.
>>
>>It is.  Its representation isn't guaranteed to be all-bits-zero,
>
> C guarantees that.

Linux C does it.  But not Standard C.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: kfree(0) - ok?

[Giacomo A. Catenazzi]

Jan Engelhardt wrote:
> On Aug 14 2007 16:21, Jason Uhlenkott wrote:
>> On Tue, Aug 14, 2007 at 15:55:48 -0700, Arjan van de Ven wrote:
>>> NULL is not 0 though.
>> It is.  Its representation isn't guaranteed to be all-bits-zero,
> 
> C guarantees that.

Hmm. It depends on your interpretation of "representation".
On memory a null pointer can have some bit set.

No, see a very recent discussion on austin group list
(which list also few machines that don't have all 0-bits null pointer)

To clarify, from Rationale of C99, section 6.7.8 "Initialization":

: An implementation might conceivably have codes for floating zero
: and/or null pointer other than all bits zero. In such a case,
: the implementation must fill out an incomplete initializer with
: the various appropriate representations of zero; it may not just
: fill the area with zero bytes. As far as the committee knows,
: all machines treat all bits zero as a representation of
: floating-point zero. But, all bits zero might not be the
: canonical representation of zero.

Anyway, I think for kernel it is safe to assume all-zero bit
null pointer.

ciao
	cate

Re: kfree(0) - ok?

[Satyam Sharma]

On Tue, 14 Aug 2007, Arjan van de Ven wrote:

> 
> On Tue, 2007-08-14 at 15:59 -0700, Tim Bird wrote:
> > Hi all,
> > 
> > I have a quick question.
> > 
> > I'm trying to resurrect a patch from the Linux-tiny patch suite,
> > to do accounting of kmalloc memory allocations.  In testing it
> > with Linux 2.6.22, I've found a large number of kfrees of
> > NULL pointers.
> > 
> > Is this considered OK?  Or should I examine the offenders
> > to see if something is coded badly?
> 
> kfree(NULL) is explicitly ok and it saves code size to not check
> anywhere

But that doesn't come free of cost, does it, seeing we've now pushed
the conditional inside kfree() itself. kfree() isn't inlined so we do
save on space but lose out on the extra time overhead for the common
case. Speaking of which ...

[PATCH] {slub, slob}: use unlikely() for kfree(ZERO_OR_NULL_PTR) check

Considering kfree(NULL) would normally occur only in error paths and
kfree(ZERO_SIZE_PTR) is uncommon as well, so let's use unlikely() for
the condition check in SLUB's and SLOB's kfree() to optimize for the
common case. SLAB has this already.

Signed-off-by: Satyam Sharma <satyam@infradead.org>

---

 mm/slob.c |    2 +-
 mm/slub.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/slob.c b/mm/slob.c
index ec33fcd..37a8b9a 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -466,7 +466,7 @@ void kfree(const void *block)
 {
 	struct slob_page *sp;
 
-	if (ZERO_OR_NULL_PTR(block))
+	if (unlikely(ZERO_OR_NULL_PTR(block)))
 		return;
 
 	sp = (struct slob_page *)virt_to_page(block);
diff --git a/mm/slub.c b/mm/slub.c
index 69d02e3..3788537 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2467,7 +2467,7 @@ void kfree(const void *x)
 	 * this comparison would be true for all "negative" pointers
 	 * (which would cover the whole upper half of the address space).
 	 */
-	if (ZERO_OR_NULL_PTR(x))
+	if (unlikely(ZERO_OR_NULL_PTR(x)))
 		return;
 
 	page = virt_to_head_page(x);

Re: kfree(0) - ok?

[Christoph Lameter]

On Wed, 15 Aug 2007, Satyam Sharma wrote:

> [PATCH] {slub, slob}: use unlikely() for kfree(ZERO_OR_NULL_PTR) check

Good that actually has a code impact.

Re: kfree(0) - ok?

[Andrew Morton]

On Wed, 15 Aug 2007 05:12:41 +0530 (IST)
Satyam Sharma <satyam@infradead.org> wrote:

> [PATCH] {slub, slob}: use unlikely() for kfree(ZERO_OR_NULL_PTR) check
> 
> Considering kfree(NULL) would normally occur only in error paths and
> kfree(ZERO_SIZE_PTR) is uncommon as well, so let's use unlikely() for
> the condition check in SLUB's and SLOB's kfree() to optimize for the
> common case. SLAB has this already.

I went through my current versions of slab/slub/slub and came up with this:

diff -puN mm/slob.c~slub-slob-use-unlikely-for-kfreezero_or_null_ptr-check mm/slob.c
--- a/mm/slob.c~slub-slob-use-unlikely-for-kfreezero_or_null_ptr-check
+++ a/mm/slob.c
@@ -360,7 +360,7 @@ static void slob_free(void *block, int s
 	slobidx_t units;
 	unsigned long flags;
 
-	if (ZERO_OR_NULL_PTR(block))
+	if (unlikely(ZERO_OR_NULL_PTR(block)))
 		return;
 	BUG_ON(!size);
 
@@ -466,7 +466,7 @@ void kfree(const void *block)
 {
 	struct slob_page *sp;
 
-	if (ZERO_OR_NULL_PTR(block))
+	if (unlikely(ZERO_OR_NULL_PTR(block)))
 		return;
 
 	sp = (struct slob_page *)virt_to_page(block);
@@ -484,7 +484,7 @@ size_t ksize(const void *block)
 {
 	struct slob_page *sp;
 
-	if (ZERO_OR_NULL_PTR(block))
+	if (unlikely(ZERO_OR_NULL_PTR(block)))
 		return 0;
 
 	sp = (struct slob_page *)virt_to_page(block);
diff -puN mm/slub.c~slub-slob-use-unlikely-for-kfreezero_or_null_ptr-check mm/slub.c
--- a/mm/slub.c~slub-slob-use-unlikely-for-kfreezero_or_null_ptr-check
+++ a/mm/slub.c
@@ -2434,7 +2434,7 @@ size_t ksize(const void *object)
 	struct page *page;
 	struct kmem_cache *s;
 
-	if (ZERO_OR_NULL_PTR(object))
+	if (unlikely(ZERO_OR_NULL_PTR(object)))
 		return 0;
 
 	page = get_object_page(object);
@@ -2468,7 +2468,7 @@ void kfree(const void *x)
 {
 	struct page *page;
 
-	if (ZERO_OR_NULL_PTR(x))
+	if (unlikely(ZERO_OR_NULL_PTR(x)))
 		return;
 
 	page = virt_to_head_page(x);
@@ -2785,7 +2785,7 @@ void *__kmalloc_track_caller(size_t size
 							get_order(size));
 	s = get_slab(size, gfpflags);
 
-	if (ZERO_OR_NULL_PTR(s))
+	if (unlikely(ZERO_OR_NULL_PTR(s)))
 		return s;
 
 	return slab_alloc(s, gfpflags, -1, caller);
@@ -2801,7 +2801,7 @@ void *__kmalloc_node_track_caller(size_t
 							get_order(size));
 	s = get_slab(size, gfpflags);
 
-	if (ZERO_OR_NULL_PTR(s))
+	if (unlikely(ZERO_OR_NULL_PTR(s)))
 		return s;
 
 	return slab_alloc(s, gfpflags, node, caller);
_

Which is getting pretty idiotic:

akpm:/usr/src/25> grep ZERO_OR_NULL_PTR */*.c
mm/slab.c:              BUG_ON(ZERO_OR_NULL_PTR(cachep->slabp_cache));
mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(cachep)))
mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(cachep)))
mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(objp)))
mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(objp)))
mm/slob.c:      if (unlikely(ZERO_OR_NULL_PTR(block)))
mm/slob.c:      if (unlikely(ZERO_OR_NULL_PTR(block)))
mm/slob.c:      if (unlikely(ZERO_OR_NULL_PTR(block)))
mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(object)))
mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(x)))
mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))

are we seeing a pattern here?  We could stick the unlikely inside
ZERO_OR_NULL_PTR() itself.  That's a little bit sleazy though - there might
be future callsites at which it is likely, who knows?

I guess we can stick with the idiotic patch ;)

Re: kfree(0) - ok?

[Arjan van de Ven]

On Fri, 2007-08-17 at 11:22 -0700, Andrew Morton wrote:
> On Wed, 15 Aug 2007 05:12:41 +0530 (IST)
> Satyam Sharma <satyam@infradead.org> wrote:
> 
> > [PATCH] {slub, slob}: use unlikely() for kfree(ZERO_OR_NULL_PTR) check
> > 
> > Considering kfree(NULL) would normally occur only in error paths and
> > kfree(ZERO_SIZE_PTR) is uncommon as well, so let's use unlikely() for
> > the condition check in SLUB's and SLOB's kfree() to optimize for the
> > common case. SLAB has this already.
> 
> I went through my current versions of slab/slub/slub and came up with this:
> 
> diff -puN mm/slob.c~slub-slob-use-unlikely-for-kfreezero_or_null_ptr-check mm/slob.c
> --- a/mm/slob.c~slub-slob-use-unlikely-for-kfreezero_or_null_ptr-check
> +++ a/mm/slob.c
> @@ -360,7 +360,7 @@ static void slob_free(void *block, int s
>  	slobidx_t units;
>  	unsigned long flags;
>  
> -	if (ZERO_OR_NULL_PTR(block))
> +	if (unlikely(ZERO_OR_NULL_PTR(block)))



btw this makes NO sense at all; gcc already defaults to assuming
unlikely if you check a pointer for NULL....


-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org

Re: kfree(0) - ok?

[Satyam Sharma]

On Fri, 17 Aug 2007, Arjan van de Ven wrote:

> 
> On Fri, 2007-08-17 at 11:22 -0700, Andrew Morton wrote:
> > On Wed, 15 Aug 2007 05:12:41 +0530 (IST)
> > Satyam Sharma <satyam@infradead.org> wrote:
> > 
> > > [PATCH] {slub, slob}: use unlikely() for kfree(ZERO_OR_NULL_PTR) check
> > > 
> > > Considering kfree(NULL) would normally occur only in error paths and
> > > kfree(ZERO_SIZE_PTR) is uncommon as well, so let's use unlikely() for
> > > the condition check in SLUB's and SLOB's kfree() to optimize for the
> > > common case. SLAB has this already.
> > 
> > I went through my current versions of slab/slub/slub and came up with this:
> > 
> > diff -puN mm/slob.c~slub-slob-use-unlikely-for-kfreezero_or_null_ptr-check mm/slob.c
> > --- a/mm/slob.c~slub-slob-use-unlikely-for-kfreezero_or_null_ptr-check
> > +++ a/mm/slob.c
> > @@ -360,7 +360,7 @@ static void slob_free(void *block, int s
> >  	slobidx_t units;
> >  	unsigned long flags;
> >  
> > -	if (ZERO_OR_NULL_PTR(block))
> > +	if (unlikely(ZERO_OR_NULL_PTR(block)))
> 
> 
> 
> btw this makes NO sense at all; gcc already defaults to assuming
> unlikely if you check a pointer for NULL....

ZERO_OR_NULL_PTR() is not a check for NULL.

Re: kfree(0) - ok?

[Jan Engelhardt]

On Aug 17 2007 11:22, Andrew Morton wrote:

>Which is getting pretty idiotic:
>
>akpm:/usr/src/25> grep ZERO_OR_NULL_PTR */*.c
>mm/slab.c:              BUG_ON(ZERO_OR_NULL_PTR(cachep->slabp_cache));
>mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(cachep)))
>mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(cachep)))
>mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(objp)))
>mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(objp)))
>mm/slob.c:      if (unlikely(ZERO_OR_NULL_PTR(block)))
>mm/slob.c:      if (unlikely(ZERO_OR_NULL_PTR(block)))
>mm/slob.c:      if (unlikely(ZERO_OR_NULL_PTR(block)))
>mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
>mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
>mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(object)))
>mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(x)))
>mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
>mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
>
>are we seeing a pattern here?  We could stick the unlikely inside
>ZERO_OR_NULL_PTR() itself.

Yeah,

	BUG_ON(unlikely(ZERO_OR_NULL_PTR(..)))

that will really help the bug - "hm, it's Friday, let's BUG" ;-)



	Jan
-- 

Re: kfree(0) - ok?

[Christoph Lameter]

On Fri, 17 Aug 2007, Andrew Morton wrote:

> are we seeing a pattern here?  We could stick the unlikely inside
> ZERO_OR_NULL_PTR() itself.  That's a little bit sleazy though - there might
> be future callsites at which it is likely, who knows?

Thought about that myself but then there would be a weird side effect to 
ZERO_OR_NULL_PTR(). But since your thinking along the same lines: Lets do 
it. I will fix up the patch to do just that.

Re: kfree(0) - ok?

[Satyam Sharma]

On Fri, 17 Aug 2007, Christoph Lameter wrote:

> On Fri, 17 Aug 2007, Andrew Morton wrote:
> 
> > are we seeing a pattern here?  We could stick the unlikely inside
> > ZERO_OR_NULL_PTR() itself.  That's a little bit sleazy though - there might
> > be future callsites at which it is likely, who knows?
> 
> Thought about that myself but then there would be a weird side effect to 
> ZERO_OR_NULL_PTR().

True, but I suspect such a side-effect to actually matter only for the
BUG_ON case, where introducing the unlikely() would mean the output from
the show_registers() dump during the BUG() would show a not-useful-at-all
%%eax == 0x0000001 value, but only if CONFIG_PROFILE_LIKELY=y, admittedly.

> But since your thinking along the same lines: Lets do 
> it. I will fix up the patch to do just that.

Ok, thanks.


Satyam

Re: kfree(0) - ok?

[Satyam Sharma]

On Sat, 18 Aug 2007, Satyam Sharma wrote:

> On Fri, 17 Aug 2007, Christoph Lameter wrote:
> 
> > On Fri, 17 Aug 2007, Andrew Morton wrote:
> > 
> > > are we seeing a pattern here?  We could stick the unlikely inside
> > > ZERO_OR_NULL_PTR() itself.  That's a little bit sleazy though - there might
> > > be future callsites at which it is likely, who knows?
> > 
> > Thought about that myself but then there would be a weird side effect to 
> > ZERO_OR_NULL_PTR().
> 
> True, but I suspect such a side-effect to actually matter only for the
> BUG_ON case, where introducing the unlikely() would mean the output from
> the show_registers() dump during the BUG() would show a not-useful-at-all
> %%eax == 0x0000001 value, but only if CONFIG_PROFILE_LIKELY=y, admittedly.

Hang on, BUG_ON() already uses unlikely anyway. And I've just verified
from a testcase that gcc doesn't get confused by unlikely(unlikely(...))
kind of code, so we're in the clear, I think.

Re: kfree(0) - ok?

[Satyam Sharma]

On Fri, 17 Aug 2007, Andrew Morton wrote:

> On Wed, 15 Aug 2007 05:12:41 +0530 (IST)
> Satyam Sharma <satyam@infradead.org> wrote:
> 
> > [PATCH] {slub, slob}: use unlikely() for kfree(ZERO_OR_NULL_PTR) check
> > 
> > Considering kfree(NULL) would normally occur only in error paths and
> > kfree(ZERO_SIZE_PTR) is uncommon as well, so let's use unlikely() for
> > the condition check in SLUB's and SLOB's kfree() to optimize for the
> > common case. SLAB has this already.
> 
> I went through my current versions of slab/slub/slub and came up with this:
> 
> diff -puN mm/slob.c~slub-slob-use-unlikely-for-kfreezero_or_null_ptr-check mm/slob.c
> [...]
> @@ -484,7 +484,7 @@ size_t ksize(const void *block)
>  {
>  	struct slob_page *sp;
>  
> -	if (ZERO_OR_NULL_PTR(block))
> +	if (unlikely(ZERO_OR_NULL_PTR(block)))
>  		return 0;
>  
>  	sp = (struct slob_page *)virt_to_page(block);
> diff -puN mm/slub.c~slub-slob-use-unlikely-for-kfreezero_or_null_ptr-check mm/slub.c
> [...]
> @@ -2434,7 +2434,7 @@ size_t ksize(const void *object)
>  	struct page *page;
>  	struct kmem_cache *s;
>  
> -	if (ZERO_OR_NULL_PTR(object))
> +	if (unlikely(ZERO_OR_NULL_PTR(object)))
>  		return 0;
>  
>  	page = get_object_page(object);

Hmm, I didn't know ksize(NULL) was also allowed to succeed (and
return 0).

<checking around>

Oh yes, of course. We want krealloc(NULL) cases to behave
consistently as expected, and letting ksize(NULL) return 0 means
the code for krealloc() can lose an extra "if (!p)" check that
would otherwise have been required. Cool.


> Which is getting pretty idiotic:
> 
> akpm:/usr/src/25> grep ZERO_OR_NULL_PTR */*.c
> mm/slab.c:              BUG_ON(ZERO_OR_NULL_PTR(cachep->slabp_cache));
> mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(cachep)))
> mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(cachep)))
> mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(objp)))
> mm/slab.c:      if (unlikely(ZERO_OR_NULL_PTR(objp)))
> mm/slob.c:      if (unlikely(ZERO_OR_NULL_PTR(block)))
> mm/slob.c:      if (unlikely(ZERO_OR_NULL_PTR(block)))
> mm/slob.c:      if (unlikely(ZERO_OR_NULL_PTR(block)))
> mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
> mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
> mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(object)))
> mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(x)))
> mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
> mm/slub.c:      if (unlikely(ZERO_OR_NULL_PTR(s)))
> 
> are we seeing a pattern here?  We could stick the unlikely inside
> ZERO_OR_NULL_PTR() itself.  That's a little bit sleazy though - there might
> be future callsites at which it is likely, who knows?

Well, all the above callsites genuinely appear to benefit from unlikely.
And it's unlikely (English word, this here :-) ZERO_OR_NULL_PTR would grow
callsites outside of mm/ especially considering the implementation
(or even the knowledge) of the ZERO_SIZE_PTR concept is something we'd
ideally want to abstract away from other generic callsites, I imagine.


Satyam

Re: kfree(0) - ok?

[Christoph Lameter]

On Sat, 18 Aug 2007, Satyam Sharma wrote:

> >  	page = get_object_page(object);
> 
> Hmm, I didn't know ksize(NULL) was also allowed to succeed (and
> return 0).

That was merged over my objections. IMHO ksize(NULL) should fail since we 
are determining the size of an unallocated object.

> Oh yes, of course. We want krealloc(NULL) cases to behave consistently 
> as expected, and letting ksize(NULL) return 0 means the code for 
> krealloc() can lose an extra "if (!p)" check that would otherwise have 
> been required. Cool.

krealloc should check for that.

Re: kfree(0) - ok?

[Pekka Enberg]

On 8/18/07, Christoph Lameter <clameter@sgi.com> wrote:
> That was merged over my objections. IMHO ksize(NULL) should fail since we
> are determining the size of an unallocated object.

Agreed, especially as we have real zero-sized objects returned from
kmalloc() et al now.

Re: kfree(0) - ok?

[Christoph Lameter]

On Sat, 18 Aug 2007, Pekka Enberg wrote:

> Agreed, especially as we have real zero-sized objects returned from
> kmalloc() et al now.



Slab allocators: Fail if ksize is called with a NULL parameter

A NULL pointer means that the object was not allocated. One cannot
determine the size of an object that has not been allocated. Currently
we return 0 but we really should BUG() on attempts to determine the size
of something nonexistent.

krealloc() interprets NULL to mean a zero sized object. Handle that
separately in krealloc().

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6/mm/slab.c
===================================================================
--- linux-2.6.orig/mm/slab.c	2007-08-17 16:17:41.000000000 -0700
+++ linux-2.6/mm/slab.c	2007-08-17 16:18:15.000000000 -0700
@@ -4436,7 +4436,8 @@ const struct seq_operations slabstats_op
  */
 size_t ksize(const void *objp)
 {
-	if (unlikely(ZERO_OR_NULL_PTR(objp)))
+	BUG_ON(!objp);
+	if (unlikely(objp == ZERO_SIZE_PTR))
 		return 0;
 
 	return obj_size(virt_to_cache(objp));
Index: linux-2.6/mm/slob.c
===================================================================
--- linux-2.6.orig/mm/slob.c	2007-08-17 16:18:19.000000000 -0700
+++ linux-2.6/mm/slob.c	2007-08-17 16:18:40.000000000 -0700
@@ -484,7 +484,8 @@ size_t ksize(const void *block)
 {
 	struct slob_page *sp;
 
-	if (ZERO_OR_NULL_PTR(block))
+	BUG_ON(!block);
+	if (block == ZERO_SIZE_PTR)
 		return 0;
 
 	sp = (struct slob_page *)virt_to_page(block);
Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2007-08-17 16:16:36.000000000 -0700
+++ linux-2.6/mm/slub.c	2007-08-17 16:17:36.000000000 -0700
@@ -2426,7 +2426,8 @@ size_t ksize(const void *object)
 	struct page *page;
 	struct kmem_cache *s;
 
-	if (ZERO_OR_NULL_PTR(object))
+	BUG_ON(!object);
+	if (object == ZERO_SIZE_PTR)
 		return 0;
 
 	page = get_object_page(object);
Index: linux-2.6/mm/util.c
===================================================================
--- linux-2.6.orig/mm/util.c	2007-08-17 16:16:29.000000000 -0700
+++ linux-2.6/mm/util.c	2007-08-17 16:16:32.000000000 -0700
@@ -81,14 +81,16 @@ EXPORT_SYMBOL(kmemdup);
 void *krealloc(const void *p, size_t new_size, gfp_t flags)
 {
 	void *ret;
-	size_t ks;
+	size_t ks = 0;
 
 	if (unlikely(!new_size)) {
 		kfree(p);
 		return ZERO_SIZE_PTR;
 	}
 
-	ks = ksize(p);
+	if (p)
+		ks = ksize(p);
+
 	if (ks >= new_size)
 		return (void *)p;
 
 

Re: kfree(0) - ok?

[Thomas Gleixner]

On Sat, 2007-08-18 at 00:42 +0300, Pekka Enberg wrote:
> On 8/18/07, Christoph Lameter <clameter@sgi.com> wrote:
> > That was merged over my objections. IMHO ksize(NULL) should fail since we
> > are determining the size of an unallocated object.
> 
> Agreed, especially as we have real zero-sized objects returned from
> kmalloc() et al now.

Do we really ? 

If yes, who invented this 1980s reminiscence, where you got valid
pointers for malloc(0) ?

This is completely stupid. You do not go into a bar and order an empty
glass, just because you might eventually become thirsty later.

	tglx

Re: kfree(0) - ok?

[Satyam Sharma]

On Sat, 18 Aug 2007, Thomas Gleixner wrote:

> On Sat, 2007-08-18 at 00:42 +0300, Pekka Enberg wrote:
> > On 8/18/07, Christoph Lameter <clameter@sgi.com> wrote:
> > > That was merged over my objections. IMHO ksize(NULL) should fail since we
> > > are determining the size of an unallocated object.
> > 
> > Agreed, especially as we have real zero-sized objects returned from
> > kmalloc() et al now.
> 
> Do we really ? 
> 
> If yes, who invented this 1980s reminiscence, where you got valid
> pointers for malloc(0) ?

No, not valid. Dereferencing it will oops. See ZERO_SIZE_PTR.

> This is completely stupid. You do not go into a bar and order an empty
> glass, just because you might eventually become thirsty later.

What we're doing presently is at least better than what SLAB did
previously (did return a valid pointer!), all this time :-)

I do agree with you in principle, of course. But it's not for the kind of
cases that you describe -- "kmalloc(0) just because I'd eventually want
to krealloc() it to something meaningful later". This was done because
there's a lot of lazy callsites that "don't want to write code for corner
cases explicitly". Sad, very sad, I say :-)

[ The krealloc() discussion on this thread came about when I noticed that
  it's the only callsite of ksize() that would reasonably / meaningfully
  want to deal with NULL ptrs, for whom I noticed (from Andrew's initial
  mail) that ksize(NULL) returned 0. As you know from the canonical
  semantics of realloc(), it _is_ supposed to deal with NULL ptrs, hence
  the discussion. ]


Satyam

Re: kfree(0) - ok?

[Christoph Lameter]

On Sat, 18 Aug 2007, Thomas Gleixner wrote:

> If yes, who invented this 1980s reminiscence, where you got valid
> pointers for malloc(0) ?

I believe his first name started with an L and ended with an s ;-)

Seriously the kmalloc(0) pointer allowed us to detect cases in which 
people tried to store into objects allocated with kmalloc(0).

If we would just return NULL then we would not be able to distinguish it 
from a failure (that is what I initially had).

Re: kfree(0) - ok?

[Pekka Enberg]

On 8/18/07, Thomas Gleixner <tglx@linutronix.de> wrote:
> If yes, who invented this 1980s reminiscence, where you got valid
> pointers for malloc(0) ?

Well, kmalloc(0) has always been legal and traditionally returned a
pointer to a smallest non-zero sized object. We did try to make
kmalloc(0) illegal for a while but ended up fixing up a bunch of
call-sites for little or no gain. I did propose that kmalloc(0) should
return NULL but Linus and others pointed out that we can do better and
not mix up out-of-memory and zero-sized allocations.

Re: kfree(0) - ok?

[Jan Engelhardt]

On Aug 18 2007 01:40, Thomas Gleixner wrote:
>
>Do we really ? 
>
>If yes, who invented this 1980s reminiscence, where you got valid
>pointers for malloc(0) ?
>
>This is completely stupid. You do not go into a bar and order an empty
>glass, just because you might eventually become thirsty later.

Actually, this is how all-you-can-drink offers work. ;-)


	Jan
-- 

Re: kfree(0) - ok?

[Satyam Sharma]

On Fri, 17 Aug 2007, Christoph Lameter wrote:

> On Sat, 18 Aug 2007, Satyam Sharma wrote:
> 
> > Hmm, I didn't know ksize(NULL) was also allowed to succeed (and
> > return 0).
> 
> That was merged over my objections. IMHO ksize(NULL) should fail since we 
> are determining the size of an unallocated object.

Agreed, I'd have implemented ksize() that oops'ed on NULL, myself.
For that matter, I'd wish that kfree() oops'ed on NULL too (and have
duly participated in such a flamewar once), but not many (if any) on
this list seem to sympathize with such an opinion :-)

> > Oh yes, of course. We want krealloc(NULL) cases to behave consistently 
> > as expected, and letting ksize(NULL) return 0 means the code for 
> > krealloc() can lose an extra "if (!p)" check that would otherwise have 
> > been required. Cool.
> 
> krealloc should check for that.

Agreed again, explicitly checking for that only sounds fair to me.

Re: kfree(0) - ok?

[Satyam Sharma]

On Tue, 14 Aug 2007, Tim Bird wrote:

> Hi all,
> 
> I have a quick question.
> 
> I'm trying to resurrect a patch from the Linux-tiny patch suite,
> to do accounting of kmalloc memory allocations.  In testing it
> with Linux 2.6.22, I've found a large number of kfrees of
> NULL pointers.
> 
> Is this considered OK?

kfree(NULL) is allowed -- for programmers' convenience.