linux-kernel - Re: [slubllv6 12/17] slub: Avoid disabling interrupts in free slowpath

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.00.1105261354130.26952@chino.kir.corp.google.com>
Date:	Thu, 26 May 2011 15:16:04 -0700 (PDT)
From:	David Rientjes <rientjes@...gle.com>
To:	Christoph Lameter <cl@...ux.com>
cc:	Pekka Enberg <penberg@...helsinki.fi>,
	Eric Dumazet <eric.dumazet@...il.com>,
	"H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [slubllv6 12/17] slub: Avoid disabling interrupts in free
 slowpath

On Thu, 26 May 2011, Christoph Lameter wrote:

> Disabling interrupts can be avoided now. However, list operation still require
> disabling interrupts since allocations can occur from interrupt
> contexts and there is no way to perform atomic list operations.
> 
> The acquition of the list_lock therefore has to disable interrupts as well.
> 
> Dropping interrupt handling significantly simplifies the slowpath.
> 
> Signed-off-by: Christoph Lameter <cl@...ux.com>
> 
> ---
>  mm/slub.c |   16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
> 
> Index: linux-2.6/mm/slub.c
> ===================================================================
> --- linux-2.6.orig/mm/slub.c	2011-05-24 09:40:57.434875035 -0500
> +++ linux-2.6/mm/slub.c	2011-05-24 09:41:00.194875015 -0500
> @@ -2183,11 +2183,10 @@ static void __slab_free(struct kmem_cach
>  	struct kmem_cache_node *n = NULL;
>  	unsigned long flags;
>  
> -	local_irq_save(flags);
>  	stat(s, FREE_SLOWPATH);
>  
>  	if (kmem_cache_debug(s) && !free_debug_processing(s, page, x, addr))
> -		goto out_unlock;
> +		return;
>  
>  	do {
>  		prior = page->freelist;
> @@ -2206,7 +2205,7 @@ static void __slab_free(struct kmem_cach
>  			 * Otherwise the list_lock will synchronize with
>  			 * other processors updating the list of slabs.
>  			 */
> -                        spin_lock(&n->list_lock);
> +                        spin_lock_irqsave(&n->list_lock, flags);
>  		}
>  		inuse = new.inuse;
>  
> @@ -2222,7 +2221,7 @@ static void __slab_free(struct kmem_cach
>  		 */
>                  if (was_frozen)
>                          stat(s, FREE_FROZEN);
> -                goto out_unlock;
> +                return;
>          }
>  
>  	/*
> @@ -2245,11 +2244,7 @@ static void __slab_free(struct kmem_cach
>  			stat(s, FREE_ADD_PARTIAL);
>  		}
>  	}
> -
> -	spin_unlock(&n->list_lock);
> -
> -out_unlock:
> -	local_irq_restore(flags);
> +	spin_unlock_irqrestore(&n->list_lock, flags);

We know that flags will be initialized at this point in the code, but the 
compiler doesn't and it emits this:

	mm/slub.c: In function ‘__slab_free’:
	mm/slub.c:2198: warning: ‘flags’ may be used uninitialized in this function

so it'll need uninitialized_var().

>  	return;
>  
>  slab_empty:
> @@ -2261,8 +2256,7 @@ slab_empty:
>  		stat(s, FREE_REMOVE_PARTIAL);
>  	}
>  
> -	spin_unlock(&n->list_lock);
> -	local_irq_restore(flags);
> +	spin_unlock_irqrestore(&n->list_lock, flags);
>  	stat(s, FREE_SLAB);
>  	discard_slab(s, page);
>  }
> 
>