[PATCH tip/core/urgent 0/2] RCU lockdep fix and shrink RCU API

[PATCH tip/core/urgent 0/2] RCU lockdep fix and shrink RCU API

[Paul E. McKenney]

Hello!

This set has two patches, one from Lai Jiangshan that fixes RCU lockdep's
rcu_read_lock_sched_held() to correctly interpret things like
local_irq_disable() as starting a valid RCU-sched read-side critical
section, and one from Alexey Dobriyan that removes several useless
members of the RCU API.

							Thanx, Paul

------------------------------------------------------------------------

 b/Documentation/DocBook/kernel-locking.tmpl   |    8 --------
 b/arch/powerpc/mm/pgtable.c                   |    1 -
 b/block/cfq-iosched.c                         |    1 -
 b/block/genhd.c                               |    1 -
 b/drivers/staging/batman-adv/hard-interface.c |    1 -
 b/fs/file.c                                   |    3 ---
 b/fs/fs-writeback.c                           |    1 -
 b/fs/partitions/check.c                       |    1 -
 b/include/linux/init_task.h                   |    1 -
 b/include/linux/rcupdate.h                    |    4 ++--
 b/mm/backing-dev.c                            |    1 -
 b/mm/slob.c                                   |    1 -
 b/security/selinux/avc.c                      |    1 -
 b/security/selinux/netnode.c                  |    2 --
 include/linux/rcupdate.h                      |    6 ------
 15 files changed, 2 insertions(+), 31 deletions(-)

[PATCH tip/core/urgent 1/2] rcu: local_irq_disable() also delimits RCU_SCHED read-site critical sections

[Paul E. McKenney]

From: Lai Jiangshan <laijs@cn.fujitsu.com>

It is documented that local_irq_disable() also delimits
RCU_SCHED read-site critical sections.
See the document of synchronize_sched() or
Documentation/RCU/whatisRCU.txt.

So we have to test irqs_disabled() in rcu_read_lock_sched_held().
Otherwise rcu-lockdep brings incorrect complaint.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 include/linux/rcupdate.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index e1bdc4b..872a98e 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -149,7 +149,7 @@ static inline int rcu_read_lock_sched_held(void)
 		return 1;
 	if (debug_locks)
 		lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
-	return lockdep_opinion || preempt_count() != 0;
+	return lockdep_opinion || preempt_count() != 0 || irqs_disabled();
 }
 #else /* #ifdef CONFIG_PREEMPT */
 static inline int rcu_read_lock_sched_held(void)
@@ -180,7 +180,7 @@ static inline int rcu_read_lock_bh_held(void)
 #ifdef CONFIG_PREEMPT
 static inline int rcu_read_lock_sched_held(void)
 {
-	return !rcu_scheduler_active || preempt_count() != 0;
+	return !rcu_scheduler_active || preempt_count() != 0 || irqs_disabled();
 }
 #else /* #ifdef CONFIG_PREEMPT */
 static inline int rcu_read_lock_sched_held(void)
-- 
1.7.0

[PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD

[Paul E. McKenney]

From: Alexey Dobriyan <adobriyan@gmail.com>

call_rcu() will unconditionally reinitialize RCU head anyway.  New users
of these macros constantly appear, so remove them.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Acked-by: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---
 Documentation/DocBook/kernel-locking.tmpl   |    8 --------
 arch/powerpc/mm/pgtable.c                   |    1 -
 block/cfq-iosched.c                         |    1 -
 block/genhd.c                               |    1 -
 drivers/staging/batman-adv/hard-interface.c |    1 -
 fs/file.c                                   |    3 ---
 fs/fs-writeback.c                           |    1 -
 fs/partitions/check.c                       |    1 -
 include/linux/init_task.h                   |    1 -
 include/linux/rcupdate.h                    |    6 ------
 mm/backing-dev.c                            |    1 -
 mm/slob.c                                   |    1 -
 security/selinux/avc.c                      |    1 -
 security/selinux/netnode.c                  |    2 --
 14 files changed, 0 insertions(+), 29 deletions(-)

diff --git a/Documentation/DocBook/kernel-locking.tmpl b/Documentation/DocBook/kernel-locking.tmpl
index 084f6ad..e6cc574 100644
--- a/Documentation/DocBook/kernel-locking.tmpl
+++ b/Documentation/DocBook/kernel-locking.tmpl
@@ -1725,14 +1725,6 @@ the amount of locking which needs to be done.
          if (++cache_num > MAX_CACHE_SIZE) {
                  struct object *i, *outcast = NULL;
                  list_for_each_entry(i, &amp;cache, list) {
-@@ -85,6 +94,7 @@
-         obj-&gt;popularity = 0;
-         atomic_set(&amp;obj-&gt;refcnt, 1); /* The cache holds a reference */
-         spin_lock_init(&amp;obj-&gt;lock);
-+        INIT_RCU_HEAD(&amp;obj-&gt;rcu);
-
-         spin_lock_irqsave(&amp;cache_lock, flags);
-         __cache_add(obj);
 @@ -104,12 +114,11 @@
  struct object *cache_find(int id)
  {
diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
index 99df697..82e38d1 100644
--- a/arch/powerpc/mm/pgtable.c
+++ b/arch/powerpc/mm/pgtable.c
@@ -91,7 +91,6 @@ static void pte_free_rcu_callback(struct rcu_head *head)
 
 static void pte_free_submit(struct pte_freelist_batch *batch)
 {
-	INIT_RCU_HEAD(&batch->rcu);
 	call_rcu(&batch->rcu, pte_free_rcu_callback);
 }
 
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index dee9d93..caa2100 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -3719,7 +3719,6 @@ static void *cfq_init_queue(struct request_queue *q)
 	 * second, in order to have larger depth for async operations.
 	 */
 	cfqd->last_delayed_sync = jiffies - HZ;
-	INIT_RCU_HEAD(&cfqd->rcu);
 	return cfqd;
 }
 
diff --git a/block/genhd.c b/block/genhd.c
index d13ba76..27e85a2 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -987,7 +987,6 @@ int disk_expand_part_tbl(struct gendisk *disk, int partno)
 	if (!new_ptbl)
 		return -ENOMEM;
 
-	INIT_RCU_HEAD(&new_ptbl->rcu_head);
 	new_ptbl->len = target;
 
 	for (i = 0; i < len; i++)
diff --git a/drivers/staging/batman-adv/hard-interface.c b/drivers/staging/batman-adv/hard-interface.c
index befd488..96ea0e5 100644
--- a/drivers/staging/batman-adv/hard-interface.c
+++ b/drivers/staging/batman-adv/hard-interface.c
@@ -301,7 +301,6 @@ int hardif_add_interface(char *dev, int if_num)
 	batman_if->if_num = if_num;
 	batman_if->dev = dev;
 	batman_if->if_active = IF_INACTIVE;
-	INIT_RCU_HEAD(&batman_if->rcu);
 
 	printk(KERN_INFO "batman-adv:Adding interface: %s\n", dev);
 	avail_ifs++;
diff --git a/fs/file.c b/fs/file.c
index 34bb7f7..cccaead 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -178,7 +178,6 @@ static struct fdtable * alloc_fdtable(unsigned int nr)
 	fdt->open_fds = (fd_set *)data;
 	data += nr / BITS_PER_BYTE;
 	fdt->close_on_exec = (fd_set *)data;
-	INIT_RCU_HEAD(&fdt->rcu);
 	fdt->next = NULL;
 
 	return fdt;
@@ -312,7 +311,6 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
 	new_fdt->close_on_exec = (fd_set *)&newf->close_on_exec_init;
 	new_fdt->open_fds = (fd_set *)&newf->open_fds_init;
 	new_fdt->fd = &newf->fd_array[0];
-	INIT_RCU_HEAD(&new_fdt->rcu);
 	new_fdt->next = NULL;
 
 	spin_lock(&oldf->file_lock);
@@ -430,7 +428,6 @@ struct files_struct init_files = {
 		.fd		= &init_files.fd_array[0],
 		.close_on_exec	= (fd_set *)&init_files.close_on_exec_init,
 		.open_fds	= (fd_set *)&init_files.open_fds_init,
-		.rcu		= RCU_HEAD_INIT,
 	},
 	.file_lock	= __SPIN_LOCK_UNLOCKED(init_task.file_lock),
 };
diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 76fc4d5..a79d6c3 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -77,7 +77,6 @@ static inline bool bdi_work_on_stack(struct bdi_work *work)
 static inline void bdi_work_init(struct bdi_work *work,
 				 struct wb_writeback_args *args)
 {
-	INIT_RCU_HEAD(&work->rcu_head);
 	work->args = *args;
 	work->state = WS_USED;
 }
diff --git a/fs/partitions/check.c b/fs/partitions/check.c
index e8865c1..03608a4 100644
--- a/fs/partitions/check.c
+++ b/fs/partitions/check.c
@@ -455,7 +455,6 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno,
 	}
 
 	/* everything is up and running, commence */
-	INIT_RCU_HEAD(&p->rcu_head);
 	rcu_assign_pointer(ptbl->part[partno], p);
 
 	/* suppress uevent if the disk supresses it */
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index b1ed1cd..7996fc2 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -49,7 +49,6 @@ extern struct group_info init_groups;
 		{ .first = &init_task.pids[PIDTYPE_PGID].node },	\
 		{ .first = &init_task.pids[PIDTYPE_SID].node },		\
 	},								\
-	.rcu		= RCU_HEAD_INIT,				\
 	.level		= 0,						\
 	.numbers	= { {						\
 		.nr		= 0,					\
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 872a98e..7dd951e 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -77,12 +77,6 @@ extern void rcu_scheduler_starting(void);
 #error "Unknown RCU implementation specified to kernel configuration"
 #endif
 
-#define RCU_HEAD_INIT	{ .next = NULL, .func = NULL }
-#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
-#define INIT_RCU_HEAD(ptr) do { \
-       (ptr)->next = NULL; (ptr)->func = NULL; \
-} while (0)
-
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 
 extern struct lockdep_map rcu_lock_map;
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 0e8ca03..4745647 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -653,7 +653,6 @@ int bdi_init(struct backing_dev_info *bdi)
 	bdi->max_ratio = 100;
 	bdi->max_prop_frac = PROP_FRAC_BASE;
 	spin_lock_init(&bdi->wb_lock);
-	INIT_RCU_HEAD(&bdi->rcu_head);
 	INIT_LIST_HEAD(&bdi->bdi_list);
 	INIT_LIST_HEAD(&bdi->wb_list);
 	INIT_LIST_HEAD(&bdi->work_list);
diff --git a/mm/slob.c b/mm/slob.c
index 837ebd6..6de238d 100644
--- a/mm/slob.c
+++ b/mm/slob.c
@@ -647,7 +647,6 @@ void kmem_cache_free(struct kmem_cache *c, void *b)
 	if (unlikely(c->flags & SLAB_DESTROY_BY_RCU)) {
 		struct slob_rcu *slob_rcu;
 		slob_rcu = b + (c->size - sizeof(struct slob_rcu));
-		INIT_RCU_HEAD(&slob_rcu->head);
 		slob_rcu->size = c->size;
 		call_rcu(&slob_rcu->head, kmem_rcu_free);
 	} else {
diff --git a/security/selinux/avc.c b/security/selinux/avc.c
index 989fef8..bf4e3bc 100644
--- a/security/selinux/avc.c
+++ b/security/selinux/avc.c
@@ -288,7 +288,6 @@ static struct avc_node *avc_alloc_node(void)
 	if (!node)
 		goto out;
 
-	INIT_RCU_HEAD(&node->rhead);
 	INIT_HLIST_NODE(&node->list);
 	avc_cache_stats_incr(allocations);
 
diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
index 7100072..3dae9ef 100644
--- a/security/selinux/netnode.c
+++ b/security/selinux/netnode.c
@@ -182,8 +182,6 @@ static void sel_netnode_insert(struct sel_netnode *node)
 		BUG();
 	}
 
-	INIT_RCU_HEAD(&node->rcu);
-
 	/* we need to impose a limit on the growth of the hash table so check
 	 * this bucket to make sure it is within the specified bounds */
 	list_add_rcu(&node->list, &sel_netnode_hash[idx].list);
-- 
1.7.0

Re: [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD

[Mathieu Desnoyers]

* Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> From: Alexey Dobriyan <adobriyan@gmail.com>
> 
> call_rcu() will unconditionally reinitialize RCU head anyway.  New users
> of these macros constantly appear, so remove them.

Hrm. So do we have something that checks for double-use of a RCU head at
the moment ? (using call_rcu() twice on the same head without being
certain that the first callback have finished its execution).

I think that hiding rcu head initialization into call_rcu() is one more
step towards misuses that will silently corrupt rcu head lists. So I
think we should first add the double-use debugging option before we
remove the RCU head initializations.

Thanks,

Mathieu

> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Acked-by: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> ---
>  Documentation/DocBook/kernel-locking.tmpl   |    8 --------
>  arch/powerpc/mm/pgtable.c                   |    1 -
>  block/cfq-iosched.c                         |    1 -
>  block/genhd.c                               |    1 -
>  drivers/staging/batman-adv/hard-interface.c |    1 -
>  fs/file.c                                   |    3 ---
>  fs/fs-writeback.c                           |    1 -
>  fs/partitions/check.c                       |    1 -
>  include/linux/init_task.h                   |    1 -
>  include/linux/rcupdate.h                    |    6 ------
>  mm/backing-dev.c                            |    1 -
>  mm/slob.c                                   |    1 -
>  security/selinux/avc.c                      |    1 -
>  security/selinux/netnode.c                  |    2 --
>  14 files changed, 0 insertions(+), 29 deletions(-)
> 
> diff --git a/Documentation/DocBook/kernel-locking.tmpl b/Documentation/DocBook/kernel-locking.tmpl
> index 084f6ad..e6cc574 100644
> --- a/Documentation/DocBook/kernel-locking.tmpl
> +++ b/Documentation/DocBook/kernel-locking.tmpl
> @@ -1725,14 +1725,6 @@ the amount of locking which needs to be done.
>           if (++cache_num > MAX_CACHE_SIZE) {
>                   struct object *i, *outcast = NULL;
>                   list_for_each_entry(i, &amp;cache, list) {
> -@@ -85,6 +94,7 @@
> -         obj-&gt;popularity = 0;
> -         atomic_set(&amp;obj-&gt;refcnt, 1); /* The cache holds a reference */
> -         spin_lock_init(&amp;obj-&gt;lock);
> -+        INIT_RCU_HEAD(&amp;obj-&gt;rcu);
> -
> -         spin_lock_irqsave(&amp;cache_lock, flags);
> -         __cache_add(obj);
>  @@ -104,12 +114,11 @@
>   struct object *cache_find(int id)
>   {
> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
> index 99df697..82e38d1 100644
> --- a/arch/powerpc/mm/pgtable.c
> +++ b/arch/powerpc/mm/pgtable.c
> @@ -91,7 +91,6 @@ static void pte_free_rcu_callback(struct rcu_head *head)
>  
>  static void pte_free_submit(struct pte_freelist_batch *batch)
>  {
> -	INIT_RCU_HEAD(&batch->rcu);
>  	call_rcu(&batch->rcu, pte_free_rcu_callback);
>  }
>  
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index dee9d93..caa2100 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -3719,7 +3719,6 @@ static void *cfq_init_queue(struct request_queue *q)
>  	 * second, in order to have larger depth for async operations.
>  	 */
>  	cfqd->last_delayed_sync = jiffies - HZ;
> -	INIT_RCU_HEAD(&cfqd->rcu);
>  	return cfqd;
>  }
>  
> diff --git a/block/genhd.c b/block/genhd.c
> index d13ba76..27e85a2 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -987,7 +987,6 @@ int disk_expand_part_tbl(struct gendisk *disk, int partno)
>  	if (!new_ptbl)
>  		return -ENOMEM;
>  
> -	INIT_RCU_HEAD(&new_ptbl->rcu_head);
>  	new_ptbl->len = target;
>  
>  	for (i = 0; i < len; i++)
> diff --git a/drivers/staging/batman-adv/hard-interface.c b/drivers/staging/batman-adv/hard-interface.c
> index befd488..96ea0e5 100644
> --- a/drivers/staging/batman-adv/hard-interface.c
> +++ b/drivers/staging/batman-adv/hard-interface.c
> @@ -301,7 +301,6 @@ int hardif_add_interface(char *dev, int if_num)
>  	batman_if->if_num = if_num;
>  	batman_if->dev = dev;
>  	batman_if->if_active = IF_INACTIVE;
> -	INIT_RCU_HEAD(&batman_if->rcu);
>  
>  	printk(KERN_INFO "batman-adv:Adding interface: %s\n", dev);
>  	avail_ifs++;
> diff --git a/fs/file.c b/fs/file.c
> index 34bb7f7..cccaead 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -178,7 +178,6 @@ static struct fdtable * alloc_fdtable(unsigned int nr)
>  	fdt->open_fds = (fd_set *)data;
>  	data += nr / BITS_PER_BYTE;
>  	fdt->close_on_exec = (fd_set *)data;
> -	INIT_RCU_HEAD(&fdt->rcu);
>  	fdt->next = NULL;
>  
>  	return fdt;
> @@ -312,7 +311,6 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
>  	new_fdt->close_on_exec = (fd_set *)&newf->close_on_exec_init;
>  	new_fdt->open_fds = (fd_set *)&newf->open_fds_init;
>  	new_fdt->fd = &newf->fd_array[0];
> -	INIT_RCU_HEAD(&new_fdt->rcu);
>  	new_fdt->next = NULL;
>  
>  	spin_lock(&oldf->file_lock);
> @@ -430,7 +428,6 @@ struct files_struct init_files = {
>  		.fd		= &init_files.fd_array[0],
>  		.close_on_exec	= (fd_set *)&init_files.close_on_exec_init,
>  		.open_fds	= (fd_set *)&init_files.open_fds_init,
> -		.rcu		= RCU_HEAD_INIT,
>  	},
>  	.file_lock	= __SPIN_LOCK_UNLOCKED(init_task.file_lock),
>  };
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 76fc4d5..a79d6c3 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -77,7 +77,6 @@ static inline bool bdi_work_on_stack(struct bdi_work *work)
>  static inline void bdi_work_init(struct bdi_work *work,
>  				 struct wb_writeback_args *args)
>  {
> -	INIT_RCU_HEAD(&work->rcu_head);
>  	work->args = *args;
>  	work->state = WS_USED;
>  }
> diff --git a/fs/partitions/check.c b/fs/partitions/check.c
> index e8865c1..03608a4 100644
> --- a/fs/partitions/check.c
> +++ b/fs/partitions/check.c
> @@ -455,7 +455,6 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno,
>  	}
>  
>  	/* everything is up and running, commence */
> -	INIT_RCU_HEAD(&p->rcu_head);
>  	rcu_assign_pointer(ptbl->part[partno], p);
>  
>  	/* suppress uevent if the disk supresses it */
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index b1ed1cd..7996fc2 100644
> --- a/include/linux/init_task.h
> +++ b/include/linux/init_task.h
> @@ -49,7 +49,6 @@ extern struct group_info init_groups;
>  		{ .first = &init_task.pids[PIDTYPE_PGID].node },	\
>  		{ .first = &init_task.pids[PIDTYPE_SID].node },		\
>  	},								\
> -	.rcu		= RCU_HEAD_INIT,				\
>  	.level		= 0,						\
>  	.numbers	= { {						\
>  		.nr		= 0,					\
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 872a98e..7dd951e 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -77,12 +77,6 @@ extern void rcu_scheduler_starting(void);
>  #error "Unknown RCU implementation specified to kernel configuration"
>  #endif
>  
> -#define RCU_HEAD_INIT	{ .next = NULL, .func = NULL }
> -#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
> -#define INIT_RCU_HEAD(ptr) do { \
> -       (ptr)->next = NULL; (ptr)->func = NULL; \
> -} while (0)
> -
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  
>  extern struct lockdep_map rcu_lock_map;
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 0e8ca03..4745647 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -653,7 +653,6 @@ int bdi_init(struct backing_dev_info *bdi)
>  	bdi->max_ratio = 100;
>  	bdi->max_prop_frac = PROP_FRAC_BASE;
>  	spin_lock_init(&bdi->wb_lock);
> -	INIT_RCU_HEAD(&bdi->rcu_head);
>  	INIT_LIST_HEAD(&bdi->bdi_list);
>  	INIT_LIST_HEAD(&bdi->wb_list);
>  	INIT_LIST_HEAD(&bdi->work_list);
> diff --git a/mm/slob.c b/mm/slob.c
> index 837ebd6..6de238d 100644
> --- a/mm/slob.c
> +++ b/mm/slob.c
> @@ -647,7 +647,6 @@ void kmem_cache_free(struct kmem_cache *c, void *b)
>  	if (unlikely(c->flags & SLAB_DESTROY_BY_RCU)) {
>  		struct slob_rcu *slob_rcu;
>  		slob_rcu = b + (c->size - sizeof(struct slob_rcu));
> -		INIT_RCU_HEAD(&slob_rcu->head);
>  		slob_rcu->size = c->size;
>  		call_rcu(&slob_rcu->head, kmem_rcu_free);
>  	} else {
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 989fef8..bf4e3bc 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -288,7 +288,6 @@ static struct avc_node *avc_alloc_node(void)
>  	if (!node)
>  		goto out;
>  
> -	INIT_RCU_HEAD(&node->rhead);
>  	INIT_HLIST_NODE(&node->list);
>  	avc_cache_stats_incr(allocations);
>  
> diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
> index 7100072..3dae9ef 100644
> --- a/security/selinux/netnode.c
> +++ b/security/selinux/netnode.c
> @@ -182,8 +182,6 @@ static void sel_netnode_insert(struct sel_netnode *node)
>  		BUG();
>  	}
>  
> -	INIT_RCU_HEAD(&node->rcu);
> -
>  	/* we need to impose a limit on the growth of the hash table so check
>  	 * this bucket to make sure it is within the specified bounds */
>  	list_add_rcu(&node->list, &sel_netnode_hash[idx].list);
> -- 
> 1.7.0
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD

[Paul E. McKenney]

On Thu, Mar 18, 2010 at 03:35:20PM -0400, Mathieu Desnoyers wrote:
> * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> > From: Alexey Dobriyan <adobriyan@gmail.com>
> > 
> > call_rcu() will unconditionally reinitialize RCU head anyway.  New users
> > of these macros constantly appear, so remove them.
> 
> Hrm. So do we have something that checks for double-use of a RCU head at
> the moment ? (using call_rcu() twice on the same head without being
> certain that the first callback have finished its execution).
> 
> I think that hiding rcu head initialization into call_rcu() is one more
> step towards misuses that will silently corrupt rcu head lists. So I
> think we should first add the double-use debugging option before we
> remove the RCU head initializations.

So your thought is to have rcu_do_batch() do something like the
following?

	...

	next = list->next;
	prefetch(next);
	list->next = RCU_HEAD_INIT_PTR;
	func = list->func;
	list->func = RCU_HEAD_INIT_PTR;
	func(list);
	... /* touching anything referenced by "list" is use-after-free. */

Then have __call_rcu() do something like the following before initializing
the ->func and ->next pointers:

	WARN_ON_ONCE(head->next != RCU_HEAD_INIT_PTR ||
		     head->func != RCU_HEAD_INIT_PTR);

And then require that all users of call_rcu() and friends use one of the
RCU_INIT() macros?

Or did you have something else in mind?

							Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> > 
> > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > Acked-by: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > ---
> >  Documentation/DocBook/kernel-locking.tmpl   |    8 --------
> >  arch/powerpc/mm/pgtable.c                   |    1 -
> >  block/cfq-iosched.c                         |    1 -
> >  block/genhd.c                               |    1 -
> >  drivers/staging/batman-adv/hard-interface.c |    1 -
> >  fs/file.c                                   |    3 ---
> >  fs/fs-writeback.c                           |    1 -
> >  fs/partitions/check.c                       |    1 -
> >  include/linux/init_task.h                   |    1 -
> >  include/linux/rcupdate.h                    |    6 ------
> >  mm/backing-dev.c                            |    1 -
> >  mm/slob.c                                   |    1 -
> >  security/selinux/avc.c                      |    1 -
> >  security/selinux/netnode.c                  |    2 --
> >  14 files changed, 0 insertions(+), 29 deletions(-)
> > 
> > diff --git a/Documentation/DocBook/kernel-locking.tmpl b/Documentation/DocBook/kernel-locking.tmpl
> > index 084f6ad..e6cc574 100644
> > --- a/Documentation/DocBook/kernel-locking.tmpl
> > +++ b/Documentation/DocBook/kernel-locking.tmpl
> > @@ -1725,14 +1725,6 @@ the amount of locking which needs to be done.
> >           if (++cache_num > MAX_CACHE_SIZE) {
> >                   struct object *i, *outcast = NULL;
> >                   list_for_each_entry(i, &amp;cache, list) {
> > -@@ -85,6 +94,7 @@
> > -         obj-&gt;popularity = 0;
> > -         atomic_set(&amp;obj-&gt;refcnt, 1); /* The cache holds a reference */
> > -         spin_lock_init(&amp;obj-&gt;lock);
> > -+        INIT_RCU_HEAD(&amp;obj-&gt;rcu);
> > -
> > -         spin_lock_irqsave(&amp;cache_lock, flags);
> > -         __cache_add(obj);
> >  @@ -104,12 +114,11 @@
> >   struct object *cache_find(int id)
> >   {
> > diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
> > index 99df697..82e38d1 100644
> > --- a/arch/powerpc/mm/pgtable.c
> > +++ b/arch/powerpc/mm/pgtable.c
> > @@ -91,7 +91,6 @@ static void pte_free_rcu_callback(struct rcu_head *head)
> >  
> >  static void pte_free_submit(struct pte_freelist_batch *batch)
> >  {
> > -	INIT_RCU_HEAD(&batch->rcu);
> >  	call_rcu(&batch->rcu, pte_free_rcu_callback);
> >  }
> >  
> > diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> > index dee9d93..caa2100 100644
> > --- a/block/cfq-iosched.c
> > +++ b/block/cfq-iosched.c
> > @@ -3719,7 +3719,6 @@ static void *cfq_init_queue(struct request_queue *q)
> >  	 * second, in order to have larger depth for async operations.
> >  	 */
> >  	cfqd->last_delayed_sync = jiffies - HZ;
> > -	INIT_RCU_HEAD(&cfqd->rcu);
> >  	return cfqd;
> >  }
> >  
> > diff --git a/block/genhd.c b/block/genhd.c
> > index d13ba76..27e85a2 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -987,7 +987,6 @@ int disk_expand_part_tbl(struct gendisk *disk, int partno)
> >  	if (!new_ptbl)
> >  		return -ENOMEM;
> >  
> > -	INIT_RCU_HEAD(&new_ptbl->rcu_head);
> >  	new_ptbl->len = target;
> >  
> >  	for (i = 0; i < len; i++)
> > diff --git a/drivers/staging/batman-adv/hard-interface.c b/drivers/staging/batman-adv/hard-interface.c
> > index befd488..96ea0e5 100644
> > --- a/drivers/staging/batman-adv/hard-interface.c
> > +++ b/drivers/staging/batman-adv/hard-interface.c
> > @@ -301,7 +301,6 @@ int hardif_add_interface(char *dev, int if_num)
> >  	batman_if->if_num = if_num;
> >  	batman_if->dev = dev;
> >  	batman_if->if_active = IF_INACTIVE;
> > -	INIT_RCU_HEAD(&batman_if->rcu);
> >  
> >  	printk(KERN_INFO "batman-adv:Adding interface: %s\n", dev);
> >  	avail_ifs++;
> > diff --git a/fs/file.c b/fs/file.c
> > index 34bb7f7..cccaead 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -178,7 +178,6 @@ static struct fdtable * alloc_fdtable(unsigned int nr)
> >  	fdt->open_fds = (fd_set *)data;
> >  	data += nr / BITS_PER_BYTE;
> >  	fdt->close_on_exec = (fd_set *)data;
> > -	INIT_RCU_HEAD(&fdt->rcu);
> >  	fdt->next = NULL;
> >  
> >  	return fdt;
> > @@ -312,7 +311,6 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
> >  	new_fdt->close_on_exec = (fd_set *)&newf->close_on_exec_init;
> >  	new_fdt->open_fds = (fd_set *)&newf->open_fds_init;
> >  	new_fdt->fd = &newf->fd_array[0];
> > -	INIT_RCU_HEAD(&new_fdt->rcu);
> >  	new_fdt->next = NULL;
> >  
> >  	spin_lock(&oldf->file_lock);
> > @@ -430,7 +428,6 @@ struct files_struct init_files = {
> >  		.fd		= &init_files.fd_array[0],
> >  		.close_on_exec	= (fd_set *)&init_files.close_on_exec_init,
> >  		.open_fds	= (fd_set *)&init_files.open_fds_init,
> > -		.rcu		= RCU_HEAD_INIT,
> >  	},
> >  	.file_lock	= __SPIN_LOCK_UNLOCKED(init_task.file_lock),
> >  };
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 76fc4d5..a79d6c3 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -77,7 +77,6 @@ static inline bool bdi_work_on_stack(struct bdi_work *work)
> >  static inline void bdi_work_init(struct bdi_work *work,
> >  				 struct wb_writeback_args *args)
> >  {
> > -	INIT_RCU_HEAD(&work->rcu_head);
> >  	work->args = *args;
> >  	work->state = WS_USED;
> >  }
> > diff --git a/fs/partitions/check.c b/fs/partitions/check.c
> > index e8865c1..03608a4 100644
> > --- a/fs/partitions/check.c
> > +++ b/fs/partitions/check.c
> > @@ -455,7 +455,6 @@ struct hd_struct *add_partition(struct gendisk *disk, int partno,
> >  	}
> >  
> >  	/* everything is up and running, commence */
> > -	INIT_RCU_HEAD(&p->rcu_head);
> >  	rcu_assign_pointer(ptbl->part[partno], p);
> >  
> >  	/* suppress uevent if the disk supresses it */
> > diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> > index b1ed1cd..7996fc2 100644
> > --- a/include/linux/init_task.h
> > +++ b/include/linux/init_task.h
> > @@ -49,7 +49,6 @@ extern struct group_info init_groups;
> >  		{ .first = &init_task.pids[PIDTYPE_PGID].node },	\
> >  		{ .first = &init_task.pids[PIDTYPE_SID].node },		\
> >  	},								\
> > -	.rcu		= RCU_HEAD_INIT,				\
> >  	.level		= 0,						\
> >  	.numbers	= { {						\
> >  		.nr		= 0,					\
> > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> > index 872a98e..7dd951e 100644
> > --- a/include/linux/rcupdate.h
> > +++ b/include/linux/rcupdate.h
> > @@ -77,12 +77,6 @@ extern void rcu_scheduler_starting(void);
> >  #error "Unknown RCU implementation specified to kernel configuration"
> >  #endif
> >  
> > -#define RCU_HEAD_INIT	{ .next = NULL, .func = NULL }
> > -#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
> > -#define INIT_RCU_HEAD(ptr) do { \
> > -       (ptr)->next = NULL; (ptr)->func = NULL; \
> > -} while (0)
> > -
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >  
> >  extern struct lockdep_map rcu_lock_map;
> > diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> > index 0e8ca03..4745647 100644
> > --- a/mm/backing-dev.c
> > +++ b/mm/backing-dev.c
> > @@ -653,7 +653,6 @@ int bdi_init(struct backing_dev_info *bdi)
> >  	bdi->max_ratio = 100;
> >  	bdi->max_prop_frac = PROP_FRAC_BASE;
> >  	spin_lock_init(&bdi->wb_lock);
> > -	INIT_RCU_HEAD(&bdi->rcu_head);
> >  	INIT_LIST_HEAD(&bdi->bdi_list);
> >  	INIT_LIST_HEAD(&bdi->wb_list);
> >  	INIT_LIST_HEAD(&bdi->work_list);
> > diff --git a/mm/slob.c b/mm/slob.c
> > index 837ebd6..6de238d 100644
> > --- a/mm/slob.c
> > +++ b/mm/slob.c
> > @@ -647,7 +647,6 @@ void kmem_cache_free(struct kmem_cache *c, void *b)
> >  	if (unlikely(c->flags & SLAB_DESTROY_BY_RCU)) {
> >  		struct slob_rcu *slob_rcu;
> >  		slob_rcu = b + (c->size - sizeof(struct slob_rcu));
> > -		INIT_RCU_HEAD(&slob_rcu->head);
> >  		slob_rcu->size = c->size;
> >  		call_rcu(&slob_rcu->head, kmem_rcu_free);
> >  	} else {
> > diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> > index 989fef8..bf4e3bc 100644
> > --- a/security/selinux/avc.c
> > +++ b/security/selinux/avc.c
> > @@ -288,7 +288,6 @@ static struct avc_node *avc_alloc_node(void)
> >  	if (!node)
> >  		goto out;
> >  
> > -	INIT_RCU_HEAD(&node->rhead);
> >  	INIT_HLIST_NODE(&node->list);
> >  	avc_cache_stats_incr(allocations);
> >  
> > diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c
> > index 7100072..3dae9ef 100644
> > --- a/security/selinux/netnode.c
> > +++ b/security/selinux/netnode.c
> > @@ -182,8 +182,6 @@ static void sel_netnode_insert(struct sel_netnode *node)
> >  		BUG();
> >  	}
> >  
> > -	INIT_RCU_HEAD(&node->rcu);
> > -
> >  	/* we need to impose a limit on the growth of the hash table so check
> >  	 * this bucket to make sure it is within the specified bounds */
> >  	list_add_rcu(&node->list, &sel_netnode_hash[idx].list);
> > -- 
> > 1.7.0
> > 
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com

Re: [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD

[Mathieu Desnoyers]

* Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> On Thu, Mar 18, 2010 at 03:35:20PM -0400, Mathieu Desnoyers wrote:
> > * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> > > From: Alexey Dobriyan <adobriyan@gmail.com>
> > > 
> > > call_rcu() will unconditionally reinitialize RCU head anyway.  New users
> > > of these macros constantly appear, so remove them.
> > 
> > Hrm. So do we have something that checks for double-use of a RCU head at
> > the moment ? (using call_rcu() twice on the same head without being
> > certain that the first callback have finished its execution).
> > 
> > I think that hiding rcu head initialization into call_rcu() is one more
> > step towards misuses that will silently corrupt rcu head lists. So I
> > think we should first add the double-use debugging option before we
> > remove the RCU head initializations.
> 
> So your thought is to have rcu_do_batch() do something like the
> following?
> 
> 	...
> 
> 	next = list->next;
> 	prefetch(next);
> 	list->next = RCU_HEAD_INIT_PTR;
> 	func = list->func;
> 	list->func = RCU_HEAD_INIT_PTR;
> 	func(list);
> 	... /* touching anything referenced by "list" is use-after-free. */
> 
> Then have __call_rcu() do something like the following before initializing
> the ->func and ->next pointers:
> 
> 	WARN_ON_ONCE(head->next != RCU_HEAD_INIT_PTR ||
> 		     head->func != RCU_HEAD_INIT_PTR);
> 
> And then require that all users of call_rcu() and friends use one of the
> RCU_INIT() macros?
> 
> Or did you have something else in mind?

More precisely poisoning an extra field of the rcu_head, as done in the
following patch.

I posted it a few months ago, but has been rejected on the ground that
it should be re-done in within the debug objects infrastructure.  But I
had to focus on other things and never found time to do these changes.
It needs a separate patch which adds missing INIT_RCU_HEAD() to a few
more kernel sites.

The reason why I add a supplementary field for the poison is to be able
to warn for detection of incoherent list_head both in call_rcu and in
rcu_do_batch(), which does not seem possible with the scheme you propose
above. The sequence is:

init ->         debug = NULL
call_rcu ->     WARN_ON_ONCE(debug != NULL)
                debug = LIST_POISON1
rcu_do_batch -> WARN_ON_ONCE(debug != LIST_POISON1)
                debug = NULL


tree rcu: Add debug RCU head option

Poisoning the rcu_head callback list. Only for rcu tree for now.

Helps finding racy users of call_rcu(), which results in hangs because list
entries are overwritten and/or skipped.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: mingo@elte.hu
CC: akpm@linux-foundation.org
---
 include/linux/rcupdate.h |   11 +++++++++++
 include/net/dst.h        |    2 ++
 kernel/rcutree.c         |   10 ++++++++++
 lib/Kconfig.debug        |    9 +++++++++
 4 files changed, 32 insertions(+)

Index: linux-2.6-lttng/include/linux/rcupdate.h
===================================================================
--- linux-2.6-lttng.orig/include/linux/rcupdate.h	2009-11-22 20:25:49.000000000 -0500
+++ linux-2.6-lttng/include/linux/rcupdate.h	2009-11-22 22:11:48.000000000 -0500
@@ -49,6 +49,9 @@
 struct rcu_head {
 	struct rcu_head *next;
 	void (*func)(struct rcu_head *head);
+#ifdef CONFIG_DEBUG_RCU_HEAD
+	struct rcu_head *debug;
+#endif
 };
 
 /* Exported common interfaces */
@@ -77,11 +80,19 @@ extern int rcu_scheduler_active;
 #error "Unknown RCU implementation specified to kernel configuration"
 #endif
 
+#ifdef CONFIG_DEBUG_RCU_HEAD
+#define RCU_HEAD_INIT 	{ .next = NULL, .func = NULL, .debug = NULL }
+#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
+#define INIT_RCU_HEAD(ptr) do { \
+       (ptr)->next = NULL; (ptr)->func = NULL; (ptr)->debug = NULL; \
+} while (0)
+#else
 #define RCU_HEAD_INIT	{ .next = NULL, .func = NULL }
 #define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
 #define INIT_RCU_HEAD(ptr) do { \
        (ptr)->next = NULL; (ptr)->func = NULL; \
 } while (0)
+#endif
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 extern struct lockdep_map rcu_lock_map;
Index: linux-2.6-lttng/kernel/rcutree.c
===================================================================
--- linux-2.6-lttng.orig/kernel/rcutree.c	2009-11-22 21:38:56.000000000 -0500
+++ linux-2.6-lttng/kernel/rcutree.c	2009-11-22 22:10:49.000000000 -0500
@@ -39,6 +39,7 @@
 #include <asm/atomic.h>
 #include <linux/bitops.h>
 #include <linux/module.h>
+#include <linux/poison.h>
 #include <linux/completion.h>
 #include <linux/moduleparam.h>
 #include <linux/percpu.h>
@@ -1010,6 +1011,10 @@ static void rcu_do_batch(struct rcu_stat
 		next = list->next;
 		prefetch(next);
 		trace_rcu_tree_callback(list);
+#ifdef DEBUG_RCU_HEAD
+		WARN_ON_ONCE(list->debug != LIST_POISON1);
+		list->debug = NULL;
+#endif
 		list->func(list);
 		list = next;
 		if (++count >= rdp->blimit)
@@ -1291,6 +1296,11 @@ __call_rcu(struct rcu_head *head, void (
 	unsigned long flags;
 	struct rcu_data *rdp;
 
+#ifdef DEBUG_RCU_HEAD
+	WARN_ON_ONCE(head->debug);
+	head->debug = LIST_POISON1;
+#endif
+
 	head->func = func;
 	head->next = NULL;
 
Index: linux-2.6-lttng/lib/Kconfig.debug
===================================================================
--- linux-2.6-lttng.orig/lib/Kconfig.debug	2009-11-22 22:01:03.000000000 -0500
+++ linux-2.6-lttng/lib/Kconfig.debug	2009-11-22 22:10:49.000000000 -0500
@@ -652,6 +652,15 @@ config DEBUG_LIST
 
 	  If unsure, say N.
 
+config DEBUG_RCU_HEAD
+	bool "Debug RCU callbacks"
+	depends on DEBUG_KERNEL
+	depends on TREE_RCU
+	help
+	  Enable this to turn on debugging of RCU list heads (call_rcu() usage).
+	  Seems to find problems more quickly with stress-tests in single-cpu
+	  mode.
+
 config DEBUG_SG
 	bool "Debug SG table operations"
 	depends on DEBUG_KERNEL
Index: linux-2.6-lttng/include/net/dst.h
===================================================================
--- linux-2.6-lttng.orig/include/net/dst.h	2009-11-22 20:25:49.000000000 -0500
+++ linux-2.6-lttng/include/net/dst.h	2009-11-22 22:10:49.000000000 -0500
@@ -154,7 +154,9 @@ static inline void dst_hold(struct dst_e
 	 * If your kernel compilation stops here, please check
 	 * __pad_to_align_refcnt declaration in struct dst_entry
 	 */
+#ifndef CONFIG_DEBUG_RCU_HEAD
 	BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63);
+#endif
 	atomic_inc(&dst->__refcnt);
 }
 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD

[Paul E. McKenney]

On Thu, Mar 18, 2010 at 04:22:02PM -0400, Mathieu Desnoyers wrote:
> * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> > On Thu, Mar 18, 2010 at 03:35:20PM -0400, Mathieu Desnoyers wrote:
> > > * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> > > > From: Alexey Dobriyan <adobriyan@gmail.com>
> > > > 
> > > > call_rcu() will unconditionally reinitialize RCU head anyway.  New users
> > > > of these macros constantly appear, so remove them.
> > > 
> > > Hrm. So do we have something that checks for double-use of a RCU head at
> > > the moment ? (using call_rcu() twice on the same head without being
> > > certain that the first callback have finished its execution).
> > > 
> > > I think that hiding rcu head initialization into call_rcu() is one more
> > > step towards misuses that will silently corrupt rcu head lists. So I
> > > think we should first add the double-use debugging option before we
> > > remove the RCU head initializations.
> > 
> > So your thought is to have rcu_do_batch() do something like the
> > following?
> > 
> > 	...
> > 
> > 	next = list->next;
> > 	prefetch(next);
> > 	list->next = RCU_HEAD_INIT_PTR;
> > 	func = list->func;
> > 	list->func = RCU_HEAD_INIT_PTR;
> > 	func(list);
> > 	... /* touching anything referenced by "list" is use-after-free. */
> > 
> > Then have __call_rcu() do something like the following before initializing
> > the ->func and ->next pointers:
> > 
> > 	WARN_ON_ONCE(head->next != RCU_HEAD_INIT_PTR ||
> > 		     head->func != RCU_HEAD_INIT_PTR);
> > 
> > And then require that all users of call_rcu() and friends use one of the
> > RCU_INIT() macros?
> > 
> > Or did you have something else in mind?
> 
> More precisely poisoning an extra field of the rcu_head, as done in the
> following patch.
> 
> I posted it a few months ago, but has been rejected on the ground that
> it should be re-done in within the debug objects infrastructure.  But I
> had to focus on other things and never found time to do these changes.
> It needs a separate patch which adds missing INIT_RCU_HEAD() to a few
> more kernel sites.

Indeed!

> The reason why I add a supplementary field for the poison is to be able
> to warn for detection of incoherent list_head both in call_rcu and in
> rcu_do_batch(), which does not seem possible with the scheme you propose
> above. The sequence is:
> 
> init ->         debug = NULL
> call_rcu ->     WARN_ON_ONCE(debug != NULL)
>                 debug = LIST_POISON1
> rcu_do_batch -> WARN_ON_ONCE(debug != LIST_POISON1)
>                 debug = NULL
> 
> 
> tree rcu: Add debug RCU head option
> 
> Poisoning the rcu_head callback list. Only for rcu tree for now.
> 
> Helps finding racy users of call_rcu(), which results in hangs because list
> entries are overwritten and/or skipped.

This does look attractive!

Some comments below.

> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> CC: mingo@elte.hu
> CC: akpm@linux-foundation.org
> ---
>  include/linux/rcupdate.h |   11 +++++++++++
>  include/net/dst.h        |    2 ++
>  kernel/rcutree.c         |   10 ++++++++++
>  lib/Kconfig.debug        |    9 +++++++++
>  4 files changed, 32 insertions(+)
> 
> Index: linux-2.6-lttng/include/linux/rcupdate.h
> ===================================================================
> --- linux-2.6-lttng.orig/include/linux/rcupdate.h	2009-11-22 20:25:49.000000000 -0500
> +++ linux-2.6-lttng/include/linux/rcupdate.h	2009-11-22 22:11:48.000000000 -0500
> @@ -49,6 +49,9 @@
>  struct rcu_head {
>  	struct rcu_head *next;
>  	void (*func)(struct rcu_head *head);
> +#ifdef CONFIG_DEBUG_RCU_HEAD
> +	struct rcu_head *debug;
> +#endif
>  };
> 
>  /* Exported common interfaces */
> @@ -77,11 +80,19 @@ extern int rcu_scheduler_active;
>  #error "Unknown RCU implementation specified to kernel configuration"
>  #endif
> 
> +#ifdef CONFIG_DEBUG_RCU_HEAD
> +#define RCU_HEAD_INIT 	{ .next = NULL, .func = NULL, .debug = NULL }
> +#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
> +#define INIT_RCU_HEAD(ptr) do { \
> +       (ptr)->next = NULL; (ptr)->func = NULL; (ptr)->debug = NULL; \
> +} while (0)
> +#else
>  #define RCU_HEAD_INIT	{ .next = NULL, .func = NULL }
>  #define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT

RCU_HEAD() is identical in either case, so should be pulled out of the
#ifdef, right?

>  #define INIT_RCU_HEAD(ptr) do { \
>         (ptr)->next = NULL; (ptr)->func = NULL; \
>  } while (0)
> +#endif
> 
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
>  extern struct lockdep_map rcu_lock_map;
> Index: linux-2.6-lttng/kernel/rcutree.c
> ===================================================================
> --- linux-2.6-lttng.orig/kernel/rcutree.c	2009-11-22 21:38:56.000000000 -0500
> +++ linux-2.6-lttng/kernel/rcutree.c	2009-11-22 22:10:49.000000000 -0500
> @@ -39,6 +39,7 @@
>  #include <asm/atomic.h>
>  #include <linux/bitops.h>
>  #include <linux/module.h>
> +#include <linux/poison.h>
>  #include <linux/completion.h>
>  #include <linux/moduleparam.h>
>  #include <linux/percpu.h>
> @@ -1010,6 +1011,10 @@ static void rcu_do_batch(struct rcu_stat
>  		next = list->next;
>  		prefetch(next);
>  		trace_rcu_tree_callback(list);
> +#ifdef DEBUG_RCU_HEAD

This needs to be CONFIG_DEBUG_RCU_HEAD, right?

> +		WARN_ON_ONCE(list->debug != LIST_POISON1);
> +		list->debug = NULL;
> +#endif
>  		list->func(list);
>  		list = next;
>  		if (++count >= rdp->blimit)
> @@ -1291,6 +1296,11 @@ __call_rcu(struct rcu_head *head, void (
>  	unsigned long flags;
>  	struct rcu_data *rdp;
> 
> +#ifdef DEBUG_RCU_HEAD

Ditto here.

> +	WARN_ON_ONCE(head->debug);
> +	head->debug = LIST_POISON1;
> +#endif
> +
>  	head->func = func;
>  	head->next = NULL;
> 
> Index: linux-2.6-lttng/lib/Kconfig.debug
> ===================================================================
> --- linux-2.6-lttng.orig/lib/Kconfig.debug	2009-11-22 22:01:03.000000000 -0500
> +++ linux-2.6-lttng/lib/Kconfig.debug	2009-11-22 22:10:49.000000000 -0500
> @@ -652,6 +652,15 @@ config DEBUG_LIST
> 
>  	  If unsure, say N.
> 
> +config DEBUG_RCU_HEAD
> +	bool "Debug RCU callbacks"
> +	depends on DEBUG_KERNEL
> +	depends on TREE_RCU
> +	help
> +	  Enable this to turn on debugging of RCU list heads (call_rcu() usage).
> +	  Seems to find problems more quickly with stress-tests in single-cpu
> +	  mode.
> +
>  config DEBUG_SG
>  	bool "Debug SG table operations"
>  	depends on DEBUG_KERNEL
> Index: linux-2.6-lttng/include/net/dst.h
> ===================================================================
> --- linux-2.6-lttng.orig/include/net/dst.h	2009-11-22 20:25:49.000000000 -0500
> +++ linux-2.6-lttng/include/net/dst.h	2009-11-22 22:10:49.000000000 -0500
> @@ -154,7 +154,9 @@ static inline void dst_hold(struct dst_e
>  	 * If your kernel compilation stops here, please check
>  	 * __pad_to_align_refcnt declaration in struct dst_entry
>  	 */
> +#ifndef CONFIG_DEBUG_RCU_HEAD
>  	BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63);
> +#endif

You lost me on this one.

>  	atomic_inc(&dst->__refcnt);
>  }

Would you be willing to add this to TINY_RCU as well?  It would be under
#ifdef, so would not affect the size of production builds.

							Thanx, Paul

[tip:core/urgent] rcu: Fix local_irq_disable() CONFIG_PROVE_RCU=y false positives

[tip-bot for Lai Jiangshan]

Commit-ID:  0cff810f54b3b52075c27f7a7021d5b195264b6c
Gitweb:     http://git.kernel.org/tip/0cff810f54b3b52075c27f7a7021d5b195264b6c
Author:     Lai Jiangshan <laijs@cn.fujitsu.com>
AuthorDate: Thu, 18 Mar 2010 12:25:33 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 18 Mar 2010 21:25:32 +0100

rcu: Fix local_irq_disable() CONFIG_PROVE_RCU=y false positives

It is documented that local_irq_disable() also delimits RCU_SCHED
read-site critical sections.

See the document of synchronize_sched() or
Documentation/RCU/whatisRCU.txt.

So we have to test irqs_disabled() in rcu_read_lock_sched_held().
Otherwise rcu-lockdep brings incorrect complaint.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: dipankar@in.ibm.com
Cc: mathieu.desnoyers@polymtl.ca
Cc: josh@joshtriplett.org
Cc: dvhltc@us.ibm.com
Cc: niv@us.ibm.com
Cc: peterz@infradead.org
Cc: rostedt@goodmis.org
Cc: Valdis.Kletnieks@vt.edu
Cc: dhowells@redhat.com
Cc: eric.dumazet@gmail.com
LKML-Reference: <1268940334-10892-1-git-send-email-paulmck@linux.vnet.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 include/linux/rcupdate.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index e1bdc4b..872a98e 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -149,7 +149,7 @@ static inline int rcu_read_lock_sched_held(void)
 		return 1;
 	if (debug_locks)
 		lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
-	return lockdep_opinion || preempt_count() != 0;
+	return lockdep_opinion || preempt_count() != 0 || irqs_disabled();
 }
 #else /* #ifdef CONFIG_PREEMPT */
 static inline int rcu_read_lock_sched_held(void)
@@ -180,7 +180,7 @@ static inline int rcu_read_lock_bh_held(void)
 #ifdef CONFIG_PREEMPT
 static inline int rcu_read_lock_sched_held(void)
 {
-	return !rcu_scheduler_active || preempt_count() != 0;
+	return !rcu_scheduler_active || preempt_count() != 0 || irqs_disabled();
 }
 #else /* #ifdef CONFIG_PREEMPT */
 static inline int rcu_read_lock_sched_held(void)

Re: [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD

[Mathieu Desnoyers]

* Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> On Thu, Mar 18, 2010 at 04:22:02PM -0400, Mathieu Desnoyers wrote:
> > * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> > > On Thu, Mar 18, 2010 at 03:35:20PM -0400, Mathieu Desnoyers wrote:
> > > > * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
> > > > > From: Alexey Dobriyan <adobriyan@gmail.com>
> > > > > 
> > > > > call_rcu() will unconditionally reinitialize RCU head anyway.  New users
> > > > > of these macros constantly appear, so remove them.
> > > > 
> > > > Hrm. So do we have something that checks for double-use of a RCU head at
> > > > the moment ? (using call_rcu() twice on the same head without being
> > > > certain that the first callback have finished its execution).
> > > > 
> > > > I think that hiding rcu head initialization into call_rcu() is one more
> > > > step towards misuses that will silently corrupt rcu head lists. So I
> > > > think we should first add the double-use debugging option before we
> > > > remove the RCU head initializations.
> > > 
> > > So your thought is to have rcu_do_batch() do something like the
> > > following?
> > > 
> > > 	...
> > > 
> > > 	next = list->next;
> > > 	prefetch(next);
> > > 	list->next = RCU_HEAD_INIT_PTR;
> > > 	func = list->func;
> > > 	list->func = RCU_HEAD_INIT_PTR;
> > > 	func(list);
> > > 	... /* touching anything referenced by "list" is use-after-free. */
> > > 
> > > Then have __call_rcu() do something like the following before initializing
> > > the ->func and ->next pointers:
> > > 
> > > 	WARN_ON_ONCE(head->next != RCU_HEAD_INIT_PTR ||
> > > 		     head->func != RCU_HEAD_INIT_PTR);
> > > 
> > > And then require that all users of call_rcu() and friends use one of the
> > > RCU_INIT() macros?
> > > 
> > > Or did you have something else in mind?
> > 
> > More precisely poisoning an extra field of the rcu_head, as done in the
> > following patch.
> > 
> > I posted it a few months ago, but has been rejected on the ground that
> > it should be re-done in within the debug objects infrastructure.  But I
> > had to focus on other things and never found time to do these changes.
> > It needs a separate patch which adds missing INIT_RCU_HEAD() to a few
> > more kernel sites.
> 
> Indeed!
> 
> > The reason why I add a supplementary field for the poison is to be able
> > to warn for detection of incoherent list_head both in call_rcu and in
> > rcu_do_batch(), which does not seem possible with the scheme you propose
> > above. The sequence is:
> > 
> > init ->         debug = NULL
> > call_rcu ->     WARN_ON_ONCE(debug != NULL)
> >                 debug = LIST_POISON1
> > rcu_do_batch -> WARN_ON_ONCE(debug != LIST_POISON1)
> >                 debug = NULL
> > 
> > 
> > tree rcu: Add debug RCU head option
> > 
> > Poisoning the rcu_head callback list. Only for rcu tree for now.
> > 
> > Helps finding racy users of call_rcu(), which results in hangs because list
> > entries are overwritten and/or skipped.
> 
> This does look attractive!
> 
> Some comments below.
> 
> > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> > CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > CC: mingo@elte.hu
> > CC: akpm@linux-foundation.org
> > ---
> >  include/linux/rcupdate.h |   11 +++++++++++
> >  include/net/dst.h        |    2 ++
> >  kernel/rcutree.c         |   10 ++++++++++
> >  lib/Kconfig.debug        |    9 +++++++++
> >  4 files changed, 32 insertions(+)
> > 
> > Index: linux-2.6-lttng/include/linux/rcupdate.h
> > ===================================================================
> > --- linux-2.6-lttng.orig/include/linux/rcupdate.h	2009-11-22 20:25:49.000000000 -0500
> > +++ linux-2.6-lttng/include/linux/rcupdate.h	2009-11-22 22:11:48.000000000 -0500
> > @@ -49,6 +49,9 @@
> >  struct rcu_head {
> >  	struct rcu_head *next;
> >  	void (*func)(struct rcu_head *head);
> > +#ifdef CONFIG_DEBUG_RCU_HEAD
> > +	struct rcu_head *debug;
> > +#endif
> >  };
> > 
> >  /* Exported common interfaces */
> > @@ -77,11 +80,19 @@ extern int rcu_scheduler_active;
> >  #error "Unknown RCU implementation specified to kernel configuration"
> >  #endif
> > 
> > +#ifdef CONFIG_DEBUG_RCU_HEAD
> > +#define RCU_HEAD_INIT 	{ .next = NULL, .func = NULL, .debug = NULL }
> > +#define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
> > +#define INIT_RCU_HEAD(ptr) do { \
> > +       (ptr)->next = NULL; (ptr)->func = NULL; (ptr)->debug = NULL; \
> > +} while (0)
> > +#else
> >  #define RCU_HEAD_INIT	{ .next = NULL, .func = NULL }
> >  #define RCU_HEAD(head) struct rcu_head head = RCU_HEAD_INIT
> 
> RCU_HEAD() is identical in either case, so should be pulled out of the
> #ifdef, right?
> 

Yep. Will do.

> >  #define INIT_RCU_HEAD(ptr) do { \
> >         (ptr)->next = NULL; (ptr)->func = NULL; \
> >  } while (0)
> > +#endif
> > 
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >  extern struct lockdep_map rcu_lock_map;
> > Index: linux-2.6-lttng/kernel/rcutree.c
> > ===================================================================
> > --- linux-2.6-lttng.orig/kernel/rcutree.c	2009-11-22 21:38:56.000000000 -0500
> > +++ linux-2.6-lttng/kernel/rcutree.c	2009-11-22 22:10:49.000000000 -0500
> > @@ -39,6 +39,7 @@
> >  #include <asm/atomic.h>
> >  #include <linux/bitops.h>
> >  #include <linux/module.h>
> > +#include <linux/poison.h>
> >  #include <linux/completion.h>
> >  #include <linux/moduleparam.h>
> >  #include <linux/percpu.h>
> > @@ -1010,6 +1011,10 @@ static void rcu_do_batch(struct rcu_stat
> >  		next = list->next;
> >  		prefetch(next);
> >  		trace_rcu_tree_callback(list);
> > +#ifdef DEBUG_RCU_HEAD
> 
> This needs to be CONFIG_DEBUG_RCU_HEAD, right?
> 

Yes. I think this came from how the patch evolved (used a define before moving
to menus). Will fix.

> > +		WARN_ON_ONCE(list->debug != LIST_POISON1);
> > +		list->debug = NULL;
> > +#endif
> >  		list->func(list);
> >  		list = next;
> >  		if (++count >= rdp->blimit)
> > @@ -1291,6 +1296,11 @@ __call_rcu(struct rcu_head *head, void (
> >  	unsigned long flags;
> >  	struct rcu_data *rdp;
> > 
> > +#ifdef DEBUG_RCU_HEAD
> 
> Ditto here.
> 
> > +	WARN_ON_ONCE(head->debug);
> > +	head->debug = LIST_POISON1;
> > +#endif
> > +
> >  	head->func = func;
> >  	head->next = NULL;
> > 
> > Index: linux-2.6-lttng/lib/Kconfig.debug
> > ===================================================================
> > --- linux-2.6-lttng.orig/lib/Kconfig.debug	2009-11-22 22:01:03.000000000 -0500
> > +++ linux-2.6-lttng/lib/Kconfig.debug	2009-11-22 22:10:49.000000000 -0500
> > @@ -652,6 +652,15 @@ config DEBUG_LIST
> > 
> >  	  If unsure, say N.
> > 
> > +config DEBUG_RCU_HEAD
> > +	bool "Debug RCU callbacks"
> > +	depends on DEBUG_KERNEL
> > +	depends on TREE_RCU
> > +	help
> > +	  Enable this to turn on debugging of RCU list heads (call_rcu() usage).
> > +	  Seems to find problems more quickly with stress-tests in single-cpu
> > +	  mode.
> > +
> >  config DEBUG_SG
> >  	bool "Debug SG table operations"
> >  	depends on DEBUG_KERNEL
> > Index: linux-2.6-lttng/include/net/dst.h
> > ===================================================================
> > --- linux-2.6-lttng.orig/include/net/dst.h	2009-11-22 20:25:49.000000000 -0500
> > +++ linux-2.6-lttng/include/net/dst.h	2009-11-22 22:10:49.000000000 -0500
> > @@ -154,7 +154,9 @@ static inline void dst_hold(struct dst_e
> >  	 * If your kernel compilation stops here, please check
> >  	 * __pad_to_align_refcnt declaration in struct dst_entry
> >  	 */
> > +#ifndef CONFIG_DEBUG_RCU_HEAD
> >  	BUILD_BUG_ON(offsetof(struct dst_entry, __refcnt) & 63);
> > +#endif
> 
> You lost me on this one.
> 

Basically, the networking code generate a build error when the size of a struct
dst_entry grows (because it is very performance sensitive). So the agreement
here has been to disable the check when the DEBUG_RCU_HEAD config option is
active.

> >  	atomic_inc(&dst->__refcnt);
> >  }
> 
> Would you be willing to add this to TINY_RCU as well?  It would be under
> #ifdef, so would not affect the size of production builds.

Sure, I'll add it to v2. Do you think it is worthwhile to submit it even though
it's not based on the debug objects infrastructure ?

Thanks,

Mathieu

> 
> 							Thanx, Paul

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com