LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* mm/slub.c: inconsequent NULL checking
@ 2008-02-19 22:49 Adrian Bunk
  2008-02-20 13:52 ` Pekka Enberg
  0 siblings, 1 reply; 5+ messages in thread
From: Adrian Bunk @ 2008-02-19 22:49 UTC (permalink / raw)
  To: Christoph Lameter, penberg, mpm; +Cc: linux-mm, linux-kernel

The Coverity checker spotted the following inconsequent NULL checking 
introduced by commit 8ff12cfc009a2a38d87fa7058226fe197bb2696f:

<--  snip  -->

...
static inline int is_end(void *addr)
{
        return (unsigned long)addr & PAGE_MAPPING_ANON;
}
...
static void deactivate_slab(struct kmem_cache *s, struct kmem_cache_cpu *c)
{
...
        if (c->freelist)    <----------------------------------------
                stat(c, DEACTIVATE_REMOTE_FREES);
        /*
         * Merge cpu freelist into freelist. Typically we get here
         * because both freelists are empty. So this is unlikely
         * to occur.
         *
         * We need to use _is_end here because deactivate slab may
         * be called for a debug slab. Then c->freelist may contain
         * a dummy pointer.
         */
        while (unlikely(!is_end(c->freelist))) {
                void **object;

                tail = 0;       /* Hot objects. Put the slab first */

                /* Retrieve object from cpu_freelist */
                object = c->freelist;
                c->freelist = c->freelist[c->offset];
...                           ^^^^^^^^^^^^^^^^^^^^^^

<--  snip  -->

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: mm/slub.c: inconsequent NULL checking
  2008-02-19 22:49 mm/slub.c: inconsequent NULL checking Adrian Bunk
@ 2008-02-20 13:52 ` Pekka Enberg
  2008-02-22 19:59   ` [2.6 patch] mm/slub.c: remove unneeded NULL check Adrian Bunk
  0 siblings, 1 reply; 5+ messages in thread
From: Pekka Enberg @ 2008-02-20 13:52 UTC (permalink / raw)
  To: bunk, clameter, penberg, mpm; +Cc: linux-mm, linux-kernel


Hi Adrian,

On 2/20/2008, "Adrian Bunk" <bunk@kernel.org> wrote:
> The Coverity checker spotted the following inconsequent NULL checking
> introduced by commit 8ff12cfc009a2a38d87fa7058226fe197bb2696f:
> 
> <--  snip  -->
> 
> ...
> static inline int is_end(void *addr)
> {
>         return (unsigned long)addr & PAGE_MAPPING_ANON;
> }
> ...
> static void deactivate_slab(struct kmem_cache *s, struct kmem_cache_cpu *c)
> {
> ...
>         if (c->freelist)    <----------------------------------------
>                 stat(c, DEACTIVATE_REMOTE_FREES);

I spotted this too. c->freelist should never be NULL so why not send a
patch to Christoph?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [2.6 patch] mm/slub.c: remove unneeded NULL check
  2008-02-20 13:52 ` Pekka Enberg
@ 2008-02-22 19:59   ` Adrian Bunk
  2008-02-22 20:11     ` Pekka Enberg
  2008-02-27 19:55     ` Christoph Lameter
  0 siblings, 2 replies; 5+ messages in thread
From: Adrian Bunk @ 2008-02-22 19:59 UTC (permalink / raw)
  To: Pekka Enberg; +Cc: clameter, mpm, linux-mm, linux-kernel

On Wed, Feb 20, 2008 at 03:52:44PM +0200, Pekka Enberg wrote:
> 
> Hi Adrian,
> 
> On 2/20/2008, "Adrian Bunk" <bunk@kernel.org> wrote:
> > The Coverity checker spotted the following inconsequent NULL checking
> > introduced by commit 8ff12cfc009a2a38d87fa7058226fe197bb2696f:
> > 
> > <--  snip  -->
> > 
> > ...
> > static inline int is_end(void *addr)
> > {
> >         return (unsigned long)addr & PAGE_MAPPING_ANON;
> > }
> > ...
> > static void deactivate_slab(struct kmem_cache *s, struct kmem_cache_cpu *c)
> > {
> > ...
> >         if (c->freelist)    <----------------------------------------
> >                 stat(c, DEACTIVATE_REMOTE_FREES);
> 
> I spotted this too.

I missed that.

> c->freelist should never be NULL so why not send a
> patch to Christoph?

Patch below.

cu
Adrian


<--  snip  -->


There's no reason for checking c->freelist for being NULL here (and we'd 
anyway Oops below if it was).

Signed-off-by: Adrian Bunk <bunk@kernel.org>

---
dae2a3c60f258f3ad2522b85d79b735a89d702f0 diff --git a/mm/slub.c b/mm/slub.c
index 74c65af..072e0a6 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1404,8 +1404,7 @@ static void deactivate_slab(struct kmem_cache *s, struct kmem_cache_cpu *c)
 	struct page *page = c->page;
 	int tail = 1;
 
-	if (c->freelist)
-		stat(c, DEACTIVATE_REMOTE_FREES);
+	stat(c, DEACTIVATE_REMOTE_FREES);
 	/*
 	 * Merge cpu freelist into freelist. Typically we get here
 	 * because both freelists are empty. So this is unlikely

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [2.6 patch] mm/slub.c: remove unneeded NULL check
  2008-02-22 19:59   ` [2.6 patch] mm/slub.c: remove unneeded NULL check Adrian Bunk
@ 2008-02-22 20:11     ` Pekka Enberg
  2008-02-27 19:55     ` Christoph Lameter
  1 sibling, 0 replies; 5+ messages in thread
From: Pekka Enberg @ 2008-02-22 20:11 UTC (permalink / raw)
  To: Adrian Bunk; +Cc: clameter, mpm, linux-mm, linux-kernel

Adrian Bunk wrote:
> There's no reason for checking c->freelist for being NULL here (and we'd 
> anyway Oops below if it was).
> 
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
> 
> ---
> dae2a3c60f258f3ad2522b85d79b735a89d702f0 diff --git a/mm/slub.c b/mm/slub.c
> index 74c65af..072e0a6 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1404,8 +1404,7 @@ static void deactivate_slab(struct kmem_cache *s, struct kmem_cache_cpu *c)
>  	struct page *page = c->page;
>  	int tail = 1;
>  
> -	if (c->freelist)
> -		stat(c, DEACTIVATE_REMOTE_FREES);
> +	stat(c, DEACTIVATE_REMOTE_FREES);
>  	/*
>  	 * Merge cpu freelist into freelist. Typically we get here
>  	 * because both freelists are empty. So this is unlikely

Christoph, please apply.

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [2.6 patch] mm/slub.c: remove unneeded NULL check
  2008-02-22 19:59   ` [2.6 patch] mm/slub.c: remove unneeded NULL check Adrian Bunk
  2008-02-22 20:11     ` Pekka Enberg
@ 2008-02-27 19:55     ` Christoph Lameter
  1 sibling, 0 replies; 5+ messages in thread
From: Christoph Lameter @ 2008-02-27 19:55 UTC (permalink / raw)
  To: Adrian Bunk; +Cc: Pekka Enberg, mpm, linux-mm, linux-kernel

On Fri, 22 Feb 2008, Adrian Bunk wrote:

> There's no reason for checking c->freelist for being NULL here (and we'd 
> anyway Oops below if it was).

Well we still need to check for the freelist being empty otherwise the 
counter for remote frees does not work as intended. The check was 
introduced at the time when page->end did not yet exist. At that time the 
NULL check made sense.


From: Christoph Lameter <clameter@sgi.com>
Subject: Fix check for remote frees

The check for remote frees must check is_end() instead of != NULL.

We execute the !is_end() section rarely so move the check in there. Just do it
once by relying on tail being 1 only the first time we enter the loop.

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

---
dae2a3c60f258f3ad2522b85d79b735a89d702f0 diff --git a/mm/slub.c b/mm/slub.c
index 74c65af..072e0a6 100644
---
 mm/slub.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/mm/slub.c
===================================================================
--- linux-2.6.orig/mm/slub.c	2008-02-27 11:48:11.000000000 -0800
+++ linux-2.6/mm/slub.c	2008-02-27 11:51:07.000000000 -0800
@@ -1404,8 +1404,6 @@ static void deactivate_slab(struct kmem_
 	struct page *page = c->page;
 	int tail = 1;
 
-	if (c->freelist)
-		stat(c, DEACTIVATE_REMOTE_FREES);
 	/*
 	 * Merge cpu freelist into freelist. Typically we get here
 	 * because both freelists are empty. So this is unlikely
@@ -1418,6 +1416,8 @@ static void deactivate_slab(struct kmem_
 	while (unlikely(!is_end(c->freelist))) {
 		void **object;
 
+		if (unlikely(tail))
+			stat(c, DEACTIVATE_REMOTE_FREES);
 		tail = 0;	/* Hot objects. Put the slab first */
 
 		/* Retrieve object from cpu_freelist */

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2008-02-27 19:55 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-19 22:49 mm/slub.c: inconsequent NULL checking Adrian Bunk
2008-02-20 13:52 ` Pekka Enberg
2008-02-22 19:59   ` [2.6 patch] mm/slub.c: remove unneeded NULL check Adrian Bunk
2008-02-22 20:11     ` Pekka Enberg
2008-02-27 19:55     ` Christoph Lameter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).