All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Christoph Lameter <cl@linux.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Sasha Levin <sasha.levin@oracle.com>,
	Pekka Enberg <penberg@kernel.org>, Matt Mackall <mpm@selenic.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Dave Jones <davej@redhat.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: slub/debugobjects: lockup when freeing memory
Date: Tue, 19 Aug 2014 19:31:21 -0700	[thread overview]
Message-ID: <20140820023121.GS4752@linux.vnet.ibm.com> (raw)
In-Reply-To: <alpine.DEB.2.11.1408192057200.32428@gentwo.org>

On Tue, Aug 19, 2014 at 09:00:05PM -0500, Christoph Lameter wrote:
> On Mon, 18 Aug 2014, Paul E. McKenney wrote:
> 
> > > +#ifdef CONFIG_RCU_DEBUG_XYZ
> >
> > If you make CONFIG_RCU_DEBUG_XYZ instead be CONFIG_DEBUG_OBJECTS_RCU_HEAD,
> > then it will automatically show up when it needs to.
> 
> Ok.
> 
> > The rest looks plausible, for whatever that is worth.
> 
> We talked in the hallway about init_rcu_head not touching
> the contents of the rcu_head. If that is the case then we can simplify
> the patch.

That is correct -- the debug-objects code uses separate storage to track
states, and does not touch the memory to which the state applies.

> We could also remove the #ifdefs if init_rcu_head and destroy_rcu_head
> are no ops if CONFIG_DEBUG_RCU_HEAD is not defined.

And indeed they are, good point!  It appears to me that both sets of
#ifdefs can go away.

							Thanx, Paul

> Index: linux/mm/slub.c
> ===================================================================
> --- linux.orig/mm/slub.c
> +++ linux/mm/slub.c
> @@ -1308,6 +1308,25 @@ static inline struct page *alloc_slab_pa
>  	return page;
>  }
> 
> +#define need_reserve_slab_rcu						\
> +	(sizeof(((struct page *)NULL)->lru) < sizeof(struct rcu_head))
> +
> +static struct rcu_head *get_rcu_head(struct kmem_cache *s, struct page *page)
> +{
> +	if (need_reserve_slab_rcu) {
> +		int order = compound_order(page);
> +		int offset = (PAGE_SIZE << order) - s->reserved;
> +
> +		VM_BUG_ON(s->reserved != sizeof(struct rcu_head));
> +		return page_address(page) + offset;
> +	} else {
> +		/*
> +		 * RCU free overloads the RCU head over the LRU
> +		 */
> +		return (void *)&page->lru;
> +	}
> +}
> +
>  static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
>  {
>  	struct page *page;
> @@ -1357,6 +1376,22 @@ static struct page *allocate_slab(struct
>  			kmemcheck_mark_unallocated_pages(page, pages);
>  	}
> 
> +#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
> +	if (unlikely(s->flags & SLAB_DESTROY_BY_RCU) && page)
> +		/*
> +		 * Initialize various things. However, this init is
> +	 	 * not allowed to modify the contents of the rcu head.
> +		 * Allocations are permitted. However, the use of
> +		 * the same cache or another cache with SLAB_RCU_DESTROY
> +		 * set may cause additional recursions.
> +		 *
> +		 * So in order to be safe the slab caches used
> +		 * in init_rcu_head should be restricted to be of the
> +		 * non rcu kind only.
> +		 */
> +		init_rcu_head(get_rcu_head(s, page));
> +#endif
> +
>  	if (flags & __GFP_WAIT)
>  		local_irq_disable();
>  	if (!page)
> @@ -1452,13 +1487,13 @@ static void __free_slab(struct kmem_cach
>  	memcg_uncharge_slab(s, order);
>  }
> 
> -#define need_reserve_slab_rcu						\
> -	(sizeof(((struct page *)NULL)->lru) < sizeof(struct rcu_head))
> -
>  static void rcu_free_slab(struct rcu_head *h)
>  {
>  	struct page *page;
> 
> +#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
> +	destroy_rcu_head(h);
> +#endif
>  	if (need_reserve_slab_rcu)
>  		page = virt_to_head_page(h);
>  	else
> @@ -1469,24 +1504,9 @@ static void rcu_free_slab(struct rcu_hea
> 
>  static void free_slab(struct kmem_cache *s, struct page *page)
>  {
> -	if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) {
> -		struct rcu_head *head;
> -
> -		if (need_reserve_slab_rcu) {
> -			int order = compound_order(page);
> -			int offset = (PAGE_SIZE << order) - s->reserved;
> -
> -			VM_BUG_ON(s->reserved != sizeof(*head));
> -			head = page_address(page) + offset;
> -		} else {
> -			/*
> -			 * RCU free overloads the RCU head over the LRU
> -			 */
> -			head = (void *)&page->lru;
> -		}
> -
> -		call_rcu(head, rcu_free_slab);
> -	} else
> +	if (unlikely(s->flags & SLAB_DESTROY_BY_RCU))
> +		call_rcu(get_rcu_head(s, page), rcu_free_slab);
> +	else
>  		__free_slab(s, page);
>  }
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: Christoph Lameter <cl@linux.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Sasha Levin <sasha.levin@oracle.com>,
	Pekka Enberg <penberg@kernel.org>, Matt Mackall <mpm@selenic.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Dave Jones <davej@redhat.com>,
	"linux-mm@kvack.org" <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: slub/debugobjects: lockup when freeing memory
Date: Tue, 19 Aug 2014 19:31:21 -0700	[thread overview]
Message-ID: <20140820023121.GS4752@linux.vnet.ibm.com> (raw)
In-Reply-To: <alpine.DEB.2.11.1408192057200.32428@gentwo.org>

On Tue, Aug 19, 2014 at 09:00:05PM -0500, Christoph Lameter wrote:
> On Mon, 18 Aug 2014, Paul E. McKenney wrote:
> 
> > > +#ifdef CONFIG_RCU_DEBUG_XYZ
> >
> > If you make CONFIG_RCU_DEBUG_XYZ instead be CONFIG_DEBUG_OBJECTS_RCU_HEAD,
> > then it will automatically show up when it needs to.
> 
> Ok.
> 
> > The rest looks plausible, for whatever that is worth.
> 
> We talked in the hallway about init_rcu_head not touching
> the contents of the rcu_head. If that is the case then we can simplify
> the patch.

That is correct -- the debug-objects code uses separate storage to track
states, and does not touch the memory to which the state applies.

> We could also remove the #ifdefs if init_rcu_head and destroy_rcu_head
> are no ops if CONFIG_DEBUG_RCU_HEAD is not defined.

And indeed they are, good point!  It appears to me that both sets of
#ifdefs can go away.

							Thanx, Paul

> Index: linux/mm/slub.c
> ===================================================================
> --- linux.orig/mm/slub.c
> +++ linux/mm/slub.c
> @@ -1308,6 +1308,25 @@ static inline struct page *alloc_slab_pa
>  	return page;
>  }
> 
> +#define need_reserve_slab_rcu						\
> +	(sizeof(((struct page *)NULL)->lru) < sizeof(struct rcu_head))
> +
> +static struct rcu_head *get_rcu_head(struct kmem_cache *s, struct page *page)
> +{
> +	if (need_reserve_slab_rcu) {
> +		int order = compound_order(page);
> +		int offset = (PAGE_SIZE << order) - s->reserved;
> +
> +		VM_BUG_ON(s->reserved != sizeof(struct rcu_head));
> +		return page_address(page) + offset;
> +	} else {
> +		/*
> +		 * RCU free overloads the RCU head over the LRU
> +		 */
> +		return (void *)&page->lru;
> +	}
> +}
> +
>  static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
>  {
>  	struct page *page;
> @@ -1357,6 +1376,22 @@ static struct page *allocate_slab(struct
>  			kmemcheck_mark_unallocated_pages(page, pages);
>  	}
> 
> +#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
> +	if (unlikely(s->flags & SLAB_DESTROY_BY_RCU) && page)
> +		/*
> +		 * Initialize various things. However, this init is
> +	 	 * not allowed to modify the contents of the rcu head.
> +		 * Allocations are permitted. However, the use of
> +		 * the same cache or another cache with SLAB_RCU_DESTROY
> +		 * set may cause additional recursions.
> +		 *
> +		 * So in order to be safe the slab caches used
> +		 * in init_rcu_head should be restricted to be of the
> +		 * non rcu kind only.
> +		 */
> +		init_rcu_head(get_rcu_head(s, page));
> +#endif
> +
>  	if (flags & __GFP_WAIT)
>  		local_irq_disable();
>  	if (!page)
> @@ -1452,13 +1487,13 @@ static void __free_slab(struct kmem_cach
>  	memcg_uncharge_slab(s, order);
>  }
> 
> -#define need_reserve_slab_rcu						\
> -	(sizeof(((struct page *)NULL)->lru) < sizeof(struct rcu_head))
> -
>  static void rcu_free_slab(struct rcu_head *h)
>  {
>  	struct page *page;
> 
> +#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
> +	destroy_rcu_head(h);
> +#endif
>  	if (need_reserve_slab_rcu)
>  		page = virt_to_head_page(h);
>  	else
> @@ -1469,24 +1504,9 @@ static void rcu_free_slab(struct rcu_hea
> 
>  static void free_slab(struct kmem_cache *s, struct page *page)
>  {
> -	if (unlikely(s->flags & SLAB_DESTROY_BY_RCU)) {
> -		struct rcu_head *head;
> -
> -		if (need_reserve_slab_rcu) {
> -			int order = compound_order(page);
> -			int offset = (PAGE_SIZE << order) - s->reserved;
> -
> -			VM_BUG_ON(s->reserved != sizeof(*head));
> -			head = page_address(page) + offset;
> -		} else {
> -			/*
> -			 * RCU free overloads the RCU head over the LRU
> -			 */
> -			head = (void *)&page->lru;
> -		}
> -
> -		call_rcu(head, rcu_free_slab);
> -	} else
> +	if (unlikely(s->flags & SLAB_DESTROY_BY_RCU))
> +		call_rcu(get_rcu_head(s, page), rcu_free_slab);
> +	else
>  		__free_slab(s, page);
>  }
> 


  reply	other threads:[~2014-08-20  2:31 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-06-19 14:30 slub/debugobjects: lockup when freeing memory Sasha Levin
2014-06-19 14:30 ` Sasha Levin
2014-06-19 15:03 ` Christoph Lameter
2014-06-19 15:03   ` Christoph Lameter
2014-06-19 16:52   ` Paul E. McKenney
2014-06-19 16:52     ` Paul E. McKenney
2014-06-19 19:29     ` Thomas Gleixner
2014-06-19 19:29       ` Thomas Gleixner
2014-06-19 20:19       ` Christoph Lameter
2014-06-19 20:19         ` Christoph Lameter
2014-06-19 20:28         ` Thomas Gleixner
2014-06-19 20:28           ` Thomas Gleixner
2014-06-19 20:36         ` Paul E. McKenney
2014-06-19 20:36           ` Paul E. McKenney
2014-08-18 16:37         ` Paul E. McKenney
2014-08-18 16:37           ` Paul E. McKenney
2014-08-19  3:44           ` Christoph Lameter
2014-08-19  3:44             ` Christoph Lameter
2014-08-19  3:58             ` Paul E. McKenney
2014-08-19  3:58               ` Paul E. McKenney
2014-08-20  2:00               ` Christoph Lameter
2014-08-20  2:00                 ` Christoph Lameter
2014-08-20  2:31                 ` Paul E. McKenney [this message]
2014-08-20  2:31                   ` Paul E. McKenney
2014-08-20  6:01                   ` Christoph Lameter
2014-08-20  6:01                     ` Christoph Lameter
2014-08-20 12:19                     ` Paul E. McKenney
2014-08-20 12:19                       ` Paul E. McKenney
2014-06-19 20:29       ` Paul E. McKenney
2014-06-19 20:29         ` Paul E. McKenney
2014-06-19 20:32         ` Sasha Levin
2014-06-19 20:32           ` Sasha Levin
2014-06-19 20:39           ` Paul E. McKenney
2014-06-19 20:39             ` Paul E. McKenney
2014-06-19 20:37         ` Thomas Gleixner
2014-06-19 20:37           ` Thomas Gleixner
2014-06-19 20:53           ` Paul E. McKenney
2014-06-19 20:53             ` Paul E. McKenney
2014-06-19 21:32             ` Thomas Gleixner
2014-06-19 21:32               ` Thomas Gleixner
2014-06-19 22:04               ` Paul E. McKenney
2014-06-19 22:04                 ` Paul E. McKenney
2014-06-20  8:17                 ` Thomas Gleixner
2014-06-20  8:17                   ` Thomas Gleixner
2014-06-20 15:40                   ` Paul E. McKenney
2014-06-20 15:40                     ` Paul E. McKenney
2014-07-12 18:03                     ` Sasha Levin
2014-07-12 18:03                       ` Sasha Levin
2014-07-12 19:33                       ` Paul E. McKenney
2014-07-12 19:33                         ` Paul E. McKenney
2014-06-20 14:30                 ` Christoph Lameter
2014-06-20 14:30                   ` Christoph Lameter
2014-06-19 20:42         ` Sasha Levin
2014-06-19 20:42           ` Sasha Levin
2014-06-19 20:53           ` Paul E. McKenney
2014-06-19 20:53             ` Paul E. McKenney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140820023121.GS4752@linux.vnet.ibm.com \
    --to=paulmck@linux.vnet.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=cl@linux.com \
    --cc=davej@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=mpm@selenic.com \
    --cc=penberg@kernel.org \
    --cc=sasha.levin@oracle.com \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.