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

=============================
-

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

-

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.
-

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);

-

On Wed, 15 Aug 2007 05:12:41 +0530 (IST)
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...
[ message continues ]

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
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

-

That was merged over my objections. IMHO ksize(NULL) should fail since we 
krealloc should check for that.

-

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
Agreed again, explicitly checking for that only sounds fair to me.

-

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
-

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

--

-

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.

-

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).
-

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

-

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

...
[ message continues ]

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.
-

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
Ok, thanks.
Satyam

-

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.

-

Yeah,
	BUG_ON(unlikely(ZERO_OR_NULL_PTR(..)))
that will really help the bug - "hm, it's Friday, let's BUG" ;-)
	Jan

--

-

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
-

ZERO_OR_NULL_PTR() is not a check for NULL.

-

Good that actually has a code impact.
-

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).

-

Jan

--

-

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)
Anyway, I think for kernel it is safe to assume all-zero bit

null pointer.
ciao

	cate

-

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."

-

Equality with the expression 0 is guaranteed.  Representation isn't.
http://www.c-faq.com/null/varieties.html

-

Rene.

-

Jan

--

-

He said the null _pointer_ isn't guaranteed to be all-bits zero. And it 
Rene.

-

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.
	Jan

--

-

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

-

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
-

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

--

-

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
-

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.

-

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.

-