[PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka J Enberg]

From: Pekka Enberg <penberg@cs.helsinki.fi>

This changes kmem_cache_free() to deal with NULL objects passed to it. The 
current behavior is inconsistent with kfree() so there are callers 
passing NULL to kmem_cache_free().

Andreas, can you please confirm this fixes the oops you reported on 
linux-scsi?

Cc: Andreas Steinmetz <ast@domdv.de>
Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 mm/slab.c |    5 +++++
 1 file changed, 5 insertions(+)

Index: 2.6/mm/slab.c
===================================================================
--- 2.6.orig/mm/slab.c	2007-03-19 10:18:52.000000000 +0200
+++ 2.6/mm/slab.c	2007-03-19 10:19:42.000000000 +0200
@@ -3741,6 +3741,8 @@ EXPORT_SYMBOL(__kmalloc);
  * @cachep: The cache the allocation was from.
  * @objp: The previously allocated object.
  *
+ * If @objp is NULL, no operation is performed.
+ *
  * Free an object which was previously allocated from this
  * cache.
  */
@@ -3748,6 +3750,9 @@ void kmem_cache_free(struct kmem_cache *
 {
 	unsigned long flags;
 
+	if (unlikely(!objp))
+		return;
+
 	BUG_ON(virt_to_cache(objp) != cachep);
 
 	local_irq_save(flags);

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Andrew Morton]

On Mon, 19 Mar 2007 10:27:18 +0200 (EET) Pekka J Enberg <penberg@cs.helsinki.fi> wrote:

> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> This changes kmem_cache_free() to deal with NULL objects passed to it. The 
> current behavior is inconsistent with kfree() so there are callers 
> passing NULL to kmem_cache_free().

err, we don't want to do this, do we?  It adds overhead for something which
we've carefully taught all our programmers to not do.  The only known code
which will benefit from this is buggy.

> Andreas, can you please confirm this fixes the oops you reported on 
> linux-scsi?

s/fix/hide/

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka J Enberg]

Hi,

On Mon, 19 Mar 2007, Andrew Morton wrote:
> err, we don't want to do this, do we?  It adds overhead for something which
> we've carefully taught all our programmers to not do.  The only known code
> which will benefit from this is buggy.

Well, I actually disagree with that. It makes little sense for 
kmem_cache_free() to behave differently from kfree() especially since the 
overhead is minimal. But anyway, if you're unhappy with the patch, then we 
should make it explicit that you're not allowed to pass NULL to 
kmem_cache_free(), mempool_free() and perhaps others as well in which case 
kfree() comes even more special...

I know this has been discussed in the past but I think you should be able 
to blindly pass whetever pointer the allocator fuction gives you to the 
corresponding release function which is why I wanted to fix 
kmem_cache_free() in the first place.

On Mon, 19 Mar 2007, Andrew Morton wrote:
> s/fix/hide/

No, even though there is clearly some problem with either the scsi or 
block layer not allocating any pages for the iovec, I do think slab should 
deal with NULL pointers properly.

				Pekka

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Christoph Lameter]

On Mon, 19 Mar 2007, Pekka J Enberg wrote:

> This changes kmem_cache_free() to deal with NULL objects passed to it. The 
> current behavior is inconsistent with kfree() so there are callers 
> passing NULL to kmem_cache_free().

Hmmm.. kmem_cache_free is significantly different. One also needs to 
specify the slab cache.
 
> Andreas, can you please confirm this fixes the oops you reported on 
> linux-scsi?

Could we fix the BUG instead?

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka J Enberg]

On Mon, 19 Mar 2007, Christoph Lameter wrote:
> Hmmm.. kmem_cache_free is significantly different. One also needs to 
> specify the slab cache.

No, it really isn't. Why would we want kfree() to be special? It's only 
going to confuse people which results in bugs.

On Mon, 19 Mar 2007, Christoph Lameter wrote:
> Could we fix the BUG instead?

Yes, it should be fixed. But we still have a problem with block layer 
(and probably others) passing NULL to mempool_free. But where should we 
fix it if not slab? Is the problem ih bio_alloc_bioset() in fs/bio.c as 
it's leaving a ->bi_io_vec NULL? Or is it in bio_free() forgetting to 
check for NULL? Or maybe in mempool_free() in mm/mempool.c? It gets messy 
real quick because you do need to be able to say "this data is optional." 
Furthemore, the NULL sematics of kfree() are also useful for error 
handling.

It's much safer to deal with this at slab level so why leave it out?

			Pekka

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Matt Mackall]

On Mon, Mar 19, 2007 at 10:08:03AM -0700, Christoph Lameter wrote:
> On Mon, 19 Mar 2007, Pekka J Enberg wrote:
> 
> > This changes kmem_cache_free() to deal with NULL objects passed to it. The 
> > current behavior is inconsistent with kfree() so there are callers 
> > passing NULL to kmem_cache_free().
> 
> Hmmm.. kmem_cache_free is significantly different. One also needs to 
> specify the slab cache.

I think this sort of thing should work:

a = kmalloc(...)
b = kmem_cache_alloc(..)
c = allocate_some_id(...)
if (!a || !b || !c) {
   free_some_id(c)
   kmem_cache_free(c)
   kfree(a);
   return -ENOMEM;
}

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Andrew Morton]

On Mon, 19 Mar 2007 15:49:46 -0500
Matt Mackall <mpm@selenic.com> wrote:

> On Mon, Mar 19, 2007 at 10:08:03AM -0700, Christoph Lameter wrote:
> > On Mon, 19 Mar 2007, Pekka J Enberg wrote:
> > 
> > > This changes kmem_cache_free() to deal with NULL objects passed to it. The 
> > > current behavior is inconsistent with kfree() so there are callers 
> > > passing NULL to kmem_cache_free().
> > 
> > Hmmm.. kmem_cache_free is significantly different. One also needs to 
> > specify the slab cache.
> 
> I think this sort of thing should work:
> 
> a = kmalloc(...)
> b = kmem_cache_alloc(..)
> c = allocate_some_id(...)
> if (!a || !b || !c) {
>    free_some_id(c)
>    kmem_cache_free(c)
>    kfree(a);
>    return -ENOMEM;
> }

Would prefer to do:

static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
						void *objp)
{
	if (objp)
		kmem_cache_free(cachep, objp);
}

so that we don't add extra overhead to all the thousands of existing,
well-behaved callsites.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka Enberg]

On 3/19/2007, "Andrew Morton" <akpm@linux-foundation.org> wrote:
> Would prefer to do:
> 
> static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
> 						void *objp)
> {
> 	if (objp)
> 		kmem_cache_free(cachep, objp);
> }
> 
> so that we don't add extra overhead to all the thousands of existing,
> well-behaved callsites.

That bloats kernel text all the same so it's much cleaner to just make
the callers explicitly check for NULL then. That said, I'm sorry but I
just don't buy the "overhead" part of your argument since it's one
branch and no extra data cache pressure especially as we're already
doing the BUG_ON and page flag checking.

But, since you're NAKing my patch, we need to get the mempool for from
the original thread in to fix the oops.

                                     Pekka

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Andrew Morton]

On Mon, 19 Mar 2007 23:25:36 +0200 (EET)
"Pekka Enberg" <penberg@cs.helsinki.fi> wrote:

> 
> On 3/19/2007, "Andrew Morton" <akpm@linux-foundation.org> wrote:
> > Would prefer to do:
> > 
> > static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
> > 						void *objp)
> > {
> > 	if (objp)
> > 		kmem_cache_free(cachep, objp);
> > }
> > 
> > so that we don't add extra overhead to all the thousands of existing,
> > well-behaved callsites.
> 
> That bloats kernel text all the same

But only for those callsites which choose to use it!  We avoid adding a
test-and-branch to those thousands of callsite which don't need it.

This is a super-hot path.

> so it's much cleaner to just make
> the callers explicitly check for NULL then. That said, I'm sorry but I
> just don't buy the "overhead" part of your argument since it's one
> branch and no extra data cache pressure especially as we're already
> doing the BUG_ON and page flag checking.

The BUG_ON (at least) should probably be moved into CONFIG_DEBUG_SLAB.

> But, since you're NAKing my patch, we need to get the mempool for from
> the original thread in to fix the oops.

We need to fix scsi rather than working around it in slab or in mempool -
it appears that it's getting its sg lists tangled up, and the problem has
been known since November (at least).

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Matt Mackall]

On Mon, Mar 19, 2007 at 02:41:00PM -0700, Andrew Morton wrote:
> On Mon, 19 Mar 2007 23:25:36 +0200 (EET)
> "Pekka Enberg" <penberg@cs.helsinki.fi> wrote:
> 
> > 
> > On 3/19/2007, "Andrew Morton" <akpm@linux-foundation.org> wrote:
> > > Would prefer to do:
> > > 
> > > static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
> > > 						void *objp)
> > > {
> > > 	if (objp)
> > > 		kmem_cache_free(cachep, objp);
> > > }
> > > 
> > > so that we don't add extra overhead to all the thousands of existing,
> > > well-behaved callsites.
> > 
> > That bloats kernel text all the same
> 
> But only for those callsites which choose to use it!  We avoid adding a
> test-and-branch to those thousands of callsite which don't need it.
> 
> This is a super-hot path.

You're right about that. Perhaps there's some clever exception
handling thing we can do to eliminate the hot-path overhead.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka Enberg]

On 3/19/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> The BUG_ON (at least) should probably be moved into CONFIG_DEBUG_SLAB.

No it shouldn't. Letting non-slab pages pass through causes nasty and
hard to debug problems which is why we have the BUG_ONs in the first
place:

http://lkml.org/lkml/2006/5/8/101

                                           Pekka

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Christoph Lameter]

On Tue, 20 Mar 2007, Pekka Enberg wrote:

> On 3/19/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > The BUG_ON (at least) should probably be moved into CONFIG_DEBUG_SLAB.
> 
> No it shouldn't. Letting non-slab pages pass through causes nasty and
> hard to debug problems which is why we have the BUG_ONs in the first
> place:

CONFIG_DEBUG_SLAB is there in order to handle these nasty and hard to 
debug problems. Usually non-slab pages are not passed to kmem_cache_free.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka J Enberg]

On Tue, 20 Mar 2007, Christoph Lameter wrote:
> CONFIG_DEBUG_SLAB is there in order to handle these nasty and hard to 
> debug problems. Usually non-slab pages are not passed to kmem_cache_free.

Yeah, it's probably not a big deal for kmem_cache_free() but if we make 
the BUG_ON CONFIG_DEBUG_SLAB, kfree() loses it as well.

		Pekka

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka Enberg]

On 3/19/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> This is a super-hot path.

Super-hot exactly where?

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Eric Dumazet]

Pekka Enberg a écrit :
> On 3/19/07, Andrew Morton <akpm@linux-foundation.org> wrote:
>> This is a super-hot path.
> 
> Super-hot exactly where?

Don't be silly Pekka ... We have plenty oprofiles results if you dont trust 
Andrew.

CPU: AMD64 processors, speed 1992.52 MHz (estimated)
Counted CPU_CLK_UNHALTED events (Cycles outside of halt state) with a unit 
mask of 0x00 (No unit mask) count 100000
samples  %        symbol name
1861563   4.7882  tg3_start_xmit_dma_bug
1375727   3.5386  memcpy_c
1166438   3.0002  tcp_v4_rcv
1157334   2.9768  kmem_cache_free

In this workload (real server), you can see kmem_cache_free() is number four.

Adding one test and conditional branch in this super-hot function just to 
correct a bug in a SCSI driver (or whatever) is not *SANE*.


Numbers talk.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka J Enberg]

On 3/19/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > > This is a super-hot path.

At some point in time, I wrote:
> > Super-hot exactly where?

On Tue, 20 Mar 2007, Eric Dumazet wrote:
> Don't be silly Pekka ... We have plenty oprofiles results if you dont trust
> Andrew.

Oh, don't get me wrong, this has certainly nothing to do with "not 
trusting" Andrew. It's just that "this is a super-hot path" doesn't really 
help me understand where kmem_cache_free() is so performance sensitive at 
all.

On Tue, 20 Mar 2007, Eric Dumazet wrote:
> CPU: AMD64 processors, speed 1992.52 MHz (estimated)
> Counted CPU_CLK_UNHALTED events (Cycles outside of halt state) with a unit
> mask of 0x00 (No unit mask) count 100000
> samples  %        symbol name
> 1861563   4.7882  tg3_start_xmit_dma_bug
> 1375727   3.5386  memcpy_c
> 1166438   3.0002  tcp_v4_rcv
> 1157334   2.9768  kmem_cache_free
> 
> In this workload (real server), you can see kmem_cache_free() is number four.

Thanks for the profile. I still wonder where exactly thouse super-hot 
call-sites are...

On Tue, 20 Mar 2007, Eric Dumazet wrote:
> Adding one test and conditional branch in this super-hot function just to
> correct a bug in a SCSI driver (or whatever) is not *SANE*.

Agreed. Unless we can get kmem_cache_free() out of those hot paths, of 
course =).

				Pekka

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Eric Dumazet]

Pekka J Enberg a écrit :
> Thanks for the profile. I still wonder where exactly thouse super-hot 
> call-sites are...
> 

In this case, it's a typical network server

Each time a packet is sent to or received from network, network stack has to 
allocate/free a skb (kmem_cache_alloc()/kmem_cache_free() and its data 
(kmalloc/kfree)

Other paths are for example dentries allocations, file allocations, ... really 
  many spots for some workloads.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Jarek Poplawski]

On 20-03-2007 08:47, Pekka J Enberg wrote:
> On 3/19/07, Andrew Morton <akpm@linux-foundation.org> wrote:
...
> On Tue, 20 Mar 2007, Eric Dumazet wrote:
>> CPU: AMD64 processors, speed 1992.52 MHz (estimated)
>> Counted CPU_CLK_UNHALTED events (Cycles outside of halt state) with a unit
>> mask of 0x00 (No unit mask) count 100000
>> samples  %        symbol name
>> 1861563   4.7882  tg3_start_xmit_dma_bug
>> 1375727   3.5386  memcpy_c
>> 1166438   3.0002  tcp_v4_rcv
>> 1157334   2.9768  kmem_cache_free
>>
>> In this workload (real server), you can see kmem_cache_free() is number four.
> 
> Thanks for the profile. I still wonder where exactly thouse super-hot 
> call-sites are...
> 
> On Tue, 20 Mar 2007, Eric Dumazet wrote:
>> Adding one test and conditional branch in this super-hot function just to
>> correct a bug in a SCSI driver (or whatever) is not *SANE*.

Sure! My proposal is to remove a few more - so we could
transmit this data from mostly not buggy SCSI drivers
really fast!

> 
> Agreed. Unless we can get kmem_cache_free() out of those hot paths, of 
> course =).
> 
> 				Pekka

IMHO admins care far more about infallibility than speed. Every
such "saving" causes the bugs are harder to follow, it takes more 
time and people to find bug's paths and more bugs goes to stable.

I'm sure this admin with SCSI - st problem will next time think
3 times before upgrading to the next "stable" - and it'll be only
because of this one buggy driver. (Of course then we don't get his
next bug report, either - so the next SCSI bug will be diagnosed at
least one kernel version later.)

I think Pekka was right (it looks he changed his mind now) something
should be done here. I think something like this should be a minimum:

BUG_ON(!objp || virt_to_cache(objp) != cachep);

to show distinctly what's going on. (BTW, kfree isn't exceptional 
with NULL treatment - neighbouring slob.c works alike.)

Regards,
Jarek P.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka Enberg]

On 3/21/07, Jarek Poplawski <jarkao2@o2.pl> wrote:
> I think Pekka was right (it looks he changed his mind now) something
> should be done here. I think something like this should be a minimum:
>
> BUG_ON(!objp || virt_to_cache(objp) != cachep);
>
> to show distinctly what's going on.

No, if we were to add a NULL check in kmem_cache_free(), it should
behave like kfree() does. Anyway, if you feel about this strongly I
suspect the best solution is to add a __kmem_cache_free which does
_not_ have the NULL check and convert those super-hot paths to use it.
Sort of what Andrew suggested already.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Jarek Poplawski]

On Wed, Mar 21, 2007 at 02:13:52PM +0200, Pekka Enberg wrote:
> On 3/21/07, Jarek Poplawski <jarkao2@o2.pl> wrote:
> >I think Pekka was right (it looks he changed his mind now) something
> >should be done here. I think something like this should be a minimum:
> >
> >BUG_ON(!objp || virt_to_cache(objp) != cachep);
> >
> >to show distinctly what's going on.
> 
> No, if we were to add a NULL check in kmem_cache_free(), it should
> behave like kfree() does. Anyway, if you feel about this strongly I
> suspect the best solution is to add a __kmem_cache_free which does
> _not_ have the NULL check and convert those super-hot paths to use it.
> Sort of what Andrew suggested already.
> 

Are you sure there is no difference? Would this message
below be written? Would you waste youre time to write
the patch in this thread? Maybe even repostal of this
bug would be unnecessary - because somebody would have
seen in a minute something you analyzed at least 0,5h.

I don't say it's the best proposal - but at least:

1. we know the rules,
2. we save the diagnosing time for the real problem.

With  __kmem_cache_free you would set #1 I hope, but if
nobody would use this - debugging time wouldn't change.
This could be acceptable, if there were no problems
with fixing the errors. But there are problems - bugs
like this aren't fixed on time - maybe because people
waste too much time per bug?

If this path is so hot, there is other possibility:
- to write a comment about NULLs here,
- to require such checks were inserted earlier.

Why after this all there is no change in the bio_free?
This bio_free still is waiting to pass NULL bi_io_vecs
without any warning!
Why still no "nr_pages > 0" check in scsi_req_map_sg?
Was this patch so obvious - authors weren't so sure
(not talking about time)?

I think optimizations are good and possible: if there
is no bug in some place for 2 or 3 years - then OK.
But until there are such bugs - let from 1 driver only -
checks should definitely be added, even at a cost of
speed.

Cheers,
Jarek P. 
 

On 19-03-2007 09:00, Pekka Enberg wrote:
> On 3/19/07, Andrew Morton <akpm@linux-foundation.org> wrote:
>>         BUG_ON(!PageSlab(page));
>>
>> that's seriously screwed up.  Do you have CONFIG_DEBUG_SLAB enabled?  If
>> not, please enable it and retest.
> 
> This is scary. Looking at disassembly of the OOPS:
> 
> Disassembly of section .text:
> 
> 00000000 <.text>:
>   0:   5f                      pop    %edi
>   1:   c3                      ret
>   2:   57                      push   %edi
>   3:   89 c1                   mov    %eax,%ecx
>   5:   89 d7                   mov    %edx,%edi
>   7:   8d 92 00 00 00 40       lea    0x40000000(%edx),%edx
>   d:   56                      push   %esi
>   e:   c1 ea 0c                shr    $0xc,%edx
>  11:   53                      push   %ebx
>  12:   c1 e2 05                shl    $0x5,%edx
>  15:   03 15 40 5d 5a c0       add    0xc05a5d40,%edx
> 
> At this point, edx has the result of virt_to_page().
> 
>  1b:   8b 02                   mov    (%edx),%eax
>  1d:   f6 c4 40                test   $0x40,%ah
>  20:   74 03                   je     0x25
> 
> If it's a compound page, look up the real page from ->private.
> 
>  22:   8b 52 0c                mov    0xc(%edx),%edx
> 
> Now, reload page flags.
> 
>  25:   8b 02                   mov    (%edx),%eax
> 
> And test...
> 
>  27:   a8 80                   test   $0x80,%al
>  29:   75 04                   jne    0x2f
>  2b:   0f 0b                   ud2a
>  2d:   eb fe                   jmp    0x2d
>  2f:   39 4a 18                cmp    %ecx,0x18(%edx)
> 
> [snip, snip]
> 
> EIP is at kmem_cache_free+0x29/0x5a
> eax: c1800000   ebx: f0ae12c0   ecx: c18f73c0   edx: c1800000
> esi: c1919de0   edi: 00000000   ebp: 00001000   esp: f1fe7e14
> ds: 007b   es: 007b   ss: 0068
> 
> But somehow eax and edx have the same value 0xc1800000 here. Hmm?
> 
>                                   Pekka

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka J Enberg]

On Wed, 21 Mar 2007, Jarek Poplawski wrote:
> With  __kmem_cache_free you would set #1 I hope, but if
> nobody would use this - debugging time wouldn't change.

I think you got it backwards. I suggested making the _current_ 
kmem_cache_free() deal with NULL (so everyone will get it) and add a new 
optimized __kmem_cache_free() for those call-sites that really need it.

On Wed, 21 Mar 2007, Jarek Poplawski wrote:
> This could be acceptable, if there were no problems
> with fixing the errors. But there are problems - bugs
> like this aren't fixed on time - maybe because people
> waste too much time per bug?

You're barking up the wrong tree here, Jarek. I strongly feel that we 
should be more defensive in the slab for the exact reasons you outlined. 
There's bunch of bug reports people seem to dismiss as slab errors where 
in fact it's caused by a buggy caller.

That said, Eric and Andrew make a good point about kmem_cache_free() being 
in super-hot paths which clearly must be addressed. The only reason 
holding me back is the fact that I don't know what those super-hot 
call-sites are (with the exception of network skb allocation) so I am 
really in no position to make that patch.

			Pekka

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Rafael J. Wysocki]

On Wednesday, 21 March 2007 14:36, Pekka J Enberg wrote:
> On Wed, 21 Mar 2007, Jarek Poplawski wrote:
> > With  __kmem_cache_free you would set #1 I hope, but if
> > nobody would use this - debugging time wouldn't change.
> 
> I think you got it backwards. I suggested making the _current_ 
> kmem_cache_free() deal with NULL (so everyone will get it) and add a new 
> optimized __kmem_cache_free() for those call-sites that really need it.
> 
> On Wed, 21 Mar 2007, Jarek Poplawski wrote:
> > This could be acceptable, if there were no problems
> > with fixing the errors. But there are problems - bugs
> > like this aren't fixed on time - maybe because people
> > waste too much time per bug?
> 
> You're barking up the wrong tree here, Jarek. I strongly feel that we 
> should be more defensive in the slab for the exact reasons you outlined. 
> There's bunch of bug reports people seem to dismiss as slab errors where 
> in fact it's caused by a buggy caller.
> 
> That said, Eric and Andrew make a good point about kmem_cache_free() being 
> in super-hot paths which clearly must be addressed. The only reason 
> holding me back is the fact that I don't know what those super-hot 
> call-sites are (with the exception of network skb allocation) so I am 
> really in no position to make that patch.

IMHO one way to find them is to actually slow down kmem_cache_free() and see
where the performance is hurt.

Greetings,
Rafael

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka Enberg]

On 3/21/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> IMHO one way to find them is to actually slow down kmem_cache_free() and see
> where the performance is hurt.

Yeah, I'll try to sneak a patch past Andrew.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Andrew Morton]

On Wed, 21 Mar 2007 16:41:19 +0200 "Pekka Enberg" <penberg@cs.helsinki.fi> wrote:

> On 3/21/07, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > IMHO one way to find them is to actually slow down kmem_cache_free() and see
> > where the performance is hurt.
> 
> Yeah, I'll try to sneak a patch past Andrew.

That would be sneaky.

Thing is, such a patch would amount to adding a test-for-NULL to codepaths
which we *know* do not need it.  There is no point in doing that.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Jörn Engel]

On Wed, 21 March 2007 08:30:27 -0800, Andrew Morton wrote:
> On Wed, 21 Mar 2007 16:41:19 +0200 "Pekka Enberg" <penberg@cs.helsinki.fi> wrote:
> 
> > Yeah, I'll try to sneak a patch past Andrew.
> 
> That would be sneaky.
> 
> Thing is, such a patch would amount to adding a test-for-NULL to codepaths
> which we *know* do not need it.  There is no point in doing that.

How about two patches, one renaming kmem_cache_free to
kmem_cache_free_fast or __kmem_cache_free or whatever pleases you most,
the second adding kmem_cache_free with a NULL check.

The point is that the easiest way to use kmem_cache_free should be the
safest, but not necessarily the fastest.  Existing well-tuned and
NULL-aware code paths can remain fast, random new code will be safe.

Jörn

-- 
Joern's library part 14:
http://www.sandpile.org/

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Pekka Enberg]

On 3/21/2007, "Andrew Morton" <akpm@linux-foundation.org> wrote:
> Thing is, such a patch would amount to adding a test-for-NULL to codepaths
> which we *know* do not need it.  There is no point in doing that.

Now you're just being stubborn, Andrew ;-).

The extra check does not matter much at all for most cases. What it buys
us is consistent API with kfree(), more robust slab, and less bugs, no?

All the super-hot paths can be fixed with __kmem_cache_free(). As an
added bonus, we can remove all extra debugging checks when
CONFIG_SLAB_DEBUG is disabled from __kmem_cache_free() as it will be
only used in tested, known good code paths so performance for those
cases will be even better!

So I'll whoop up a patch soonish and send it to you. Perhaps your evil
twin will apply it.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Jarek Poplawski]

On Wed, Mar 21, 2007 at 03:36:34PM +0200, Pekka J Enberg wrote:
> On Wed, 21 Mar 2007, Jarek Poplawski wrote:
> > With  __kmem_cache_free you would set #1 I hope, but if
> > nobody would use this - debugging time wouldn't change.
> 
> I think you got it backwards. I suggested making the _current_ 
> kmem_cache_free() deal with NULL (so everyone will get it) and add a new 
> optimized __kmem_cache_free() for those call-sites that really need it.

If you could assure optimized version will be used only with
buggy-free code, so you don't waste time for debugging it,
then I really got it backwards, sorry!

> 
> On Wed, 21 Mar 2007, Jarek Poplawski wrote:
> > This could be acceptable, if there were no problems
> > with fixing the errors. But there are problems - bugs
> > like this aren't fixed on time - maybe because people
> > waste too much time per bug?
> 
> You're barking up the wrong tree here, Jarek. I strongly feel that we 
> should be more defensive in the slab for the exact reasons you outlined. 
> There's bunch of bug reports people seem to dismiss as slab errors where 
> in fact it's caused by a buggy caller.

But I can see only one tree here. And I seem to agree with all the rest.
So, I probably really got it backwards...

Must be going to find the right tree,
Jarek P.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Jörn Engel]

On Mon, 19 March 2007 14:10:38 -0700, Andrew Morton wrote:
> 
> Would prefer to do:
> 
> static inline void kmem_cache_free_if_not_null(struct kmem_cache *cachep,
> 						void *objp)
> {
> 	if (objp)
> 		kmem_cache_free(cachep, objp);
> }
> 
> so that we don't add extra overhead to all the thousands of existing,
> well-behaved callsites.

In principle, this would work.  But two things need changing, imho:
1. Don't inline the function.  kmem_cache_free() has only about 34 NULL
   callers, if my grep is reliable, so this case is arguable.  But in
   general, out-of-line functions are better than many extra
   conditionals pulled in through the inline one.
2. Switch the names.  According to Rusty's benchmark, the easiest way to
   use and interface should be the correct one.  Every new driver
   written by a rookie will call kmem_cache_free(), simply because the
   name seems simpler.

void kmem_cache_free_fast(struct kmem_cache *cachep, void *objp)
{
	/* old kmem_cache_free() */
}

void kmem_cache_free(struct kmem_cache *cachep, void *objp)
{
	if (likely(objp))
		kmem_cache_free_fast(cachep, objp);
}

Jörn

-- 
Correctness comes second.
Features come third.
Performance comes last.
Maintainability is easily forgotten.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Christoph Lameter]

On Mon, 19 Mar 2007, Matt Mackall wrote:

> I think this sort of thing should work:
> 
> a = kmalloc(...)
> b = kmem_cache_alloc(..)
> c = allocate_some_id(...)
> if (!a || !b || !c) {
>    free_some_id(c)
>    kmem_cache_free(c)

^^^^ this requires the specification of a kmem_cache structure and the 
object must be allocated by that cache.

>    kfree(a);

Here we dynamically determine the slab cache and do not verify even which 
slab it came from.

So you can always use kfree if you do not care. kmem_cache_free verifies
correctness.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Matt Mackall]

On Mon, Mar 19, 2007 at 02:16:01PM -0700, Christoph Lameter wrote:
> On Mon, 19 Mar 2007, Matt Mackall wrote:
> 
> > I think this sort of thing should work:
> > 
> > a = kmalloc(...)
> > b = kmem_cache_alloc(..)
> > c = allocate_some_id(...)
> > if (!a || !b || !c) {
> >    free_some_id(c)
> >    kmem_cache_free(c)
> 
> ^^^^ this requires the specification of a kmem_cache structure and the 
> object must be allocated by that cache.

Yes, omitted for brevity.

> >    kfree(a);
> 
> Here we dynamically determine the slab cache and do not verify even which 
> slab it came from.

That's an implementation detail that you shouldn't rely on and that
SLOB in fact breaks. kfree(kmem_cache_alloc(...)) is bad style to the
point of being a bug.

In my opinion:

xxxfree(xxxalloc(...)); /* should always work, even if allocation fails */
yyyfree(xxxalloc(...)); /* should never be expected to work */

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] slab: deal with NULL pointers passed to kmem_cache_free

[Andreas Steinmetz]

Pekka J Enberg wrote:
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> 
> This changes kmem_cache_free() to deal with NULL objects passed to it. The 
> current behavior is inconsistent with kfree() so there are callers 
> passing NULL to kmem_cache_free().
> 
> Andreas, can you please confirm this fixes the oops you reported on 
> linux-scsi?
> 

Didn't test this as Mike Christie pointed me to a working fix for the st
driver.

> Cc: Andreas Steinmetz <ast@domdv.de>
> Cc: Christoph Lameter <clameter@sgi.com>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
>  mm/slab.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> Index: 2.6/mm/slab.c
> ===================================================================
> --- 2.6.orig/mm/slab.c	2007-03-19 10:18:52.000000000 +0200
> +++ 2.6/mm/slab.c	2007-03-19 10:19:42.000000000 +0200
> @@ -3741,6 +3741,8 @@ EXPORT_SYMBOL(__kmalloc);
>   * @cachep: The cache the allocation was from.
>   * @objp: The previously allocated object.
>   *
> + * If @objp is NULL, no operation is performed.
> + *
>   * Free an object which was previously allocated from this
>   * cache.
>   */
> @@ -3748,6 +3750,9 @@ void kmem_cache_free(struct kmem_cache *
>  {
>  	unsigned long flags;
>  
> +	if (unlikely(!objp))
> +		return;
> +
>  	BUG_ON(virt_to_cache(objp) != cachep);
>  
>  	local_irq_save(flags);


-- 
Andreas Steinmetz                       SPAMmers use robotrap@domdv.de