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
=============================
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.
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.
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 [email protected]
Home Page: http://www.roberthancock.com/
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).
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 <[email protected]>
---
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, Satyam Sharma wrote:
> [PATCH] {slub, slob}: use unlikely() for kfree(ZERO_OR_NULL_PTR) check
Good that actually has a code impact.
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
--
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.
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
Jan Engelhardt <[email protected]> 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, [email protected]
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."
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
--
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
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.
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.
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
--
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.
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
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
--
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
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
On Wed, 15 Aug 2007 05:12:41 +0530 (IST)
Satyam Sharma <[email protected]> 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 ;)
On Fri, 2007-08-17 at 11:22 -0700, Andrew Morton wrote:
> On Wed, 15 Aug 2007 05:12:41 +0530 (IST)
> Satyam Sharma <[email protected]> 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
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
--
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 <[email protected]> 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.
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.
On Fri, 17 Aug 2007, Andrew Morton wrote:
> On Wed, 15 Aug 2007 05:12:41 +0530 (IST)
> Satyam Sharma <[email protected]> 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
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
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.
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.
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.
On 8/18/07, Christoph Lameter <[email protected]> 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.
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 <[email protected]>
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;
On Sat, 2007-08-18 at 00:42 +0300, Pekka Enberg wrote:
> On 8/18/07, Christoph Lameter <[email protected]> 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
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 <[email protected]> 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
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).
On 8/18/07, Thomas Gleixner <[email protected]> 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.
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
--