* [PATCH tip/core/urgent 0/2] RCU lockdep fix and shrink RCU API @ 2010-03-18 19:25 Paul E. McKenney 2010-03-18 19:25 ` [PATCH tip/core/urgent 1/2] rcu: local_irq_disable() also delimits RCU_SCHED read-site critical sections Paul E. McKenney 2010-03-18 19:25 ` [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD Paul E. McKenney 0 siblings, 2 replies; 9+ messages in thread From: Paul E. McKenney @ 2010-03-18 19:25 UTC (permalink / raw) To: linux-kernel Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet 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(-) ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH tip/core/urgent 1/2] rcu: local_irq_disable() also delimits RCU_SCHED read-site critical sections 2010-03-18 19:25 [PATCH tip/core/urgent 0/2] RCU lockdep fix and shrink RCU API Paul E. McKenney @ 2010-03-18 19:25 ` Paul E. McKenney 2010-03-18 22:03 ` [tip:core/urgent] rcu: Fix local_irq_disable() CONFIG_PROVE_RCU=y false positives tip-bot for Lai Jiangshan 2010-03-18 19:25 ` [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD Paul E. McKenney 1 sibling, 1 reply; 9+ messages in thread From: Paul E. McKenney @ 2010-03-18 19:25 UTC (permalink / raw) To: linux-kernel Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [tip:core/urgent] rcu: Fix local_irq_disable() CONFIG_PROVE_RCU=y false positives 2010-03-18 19:25 ` [PATCH tip/core/urgent 1/2] rcu: local_irq_disable() also delimits RCU_SCHED read-site critical sections Paul E. McKenney @ 2010-03-18 22:03 ` tip-bot for Lai Jiangshan 0 siblings, 0 replies; 9+ messages in thread From: tip-bot for Lai Jiangshan @ 2010-03-18 22:03 UTC (permalink / raw) To: linux-tip-commits; +Cc: linux-kernel, paulmck, hpa, mingo, tglx, laijs, mingo 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) ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD 2010-03-18 19:25 [PATCH tip/core/urgent 0/2] RCU lockdep fix and shrink RCU API Paul E. McKenney 2010-03-18 19:25 ` [PATCH tip/core/urgent 1/2] rcu: local_irq_disable() also delimits RCU_SCHED read-site critical sections Paul E. McKenney @ 2010-03-18 19:25 ` Paul E. McKenney 2010-03-18 19:35 ` Mathieu Desnoyers 1 sibling, 1 reply; 9+ messages in thread From: Paul E. McKenney @ 2010-03-18 19:25 UTC (permalink / raw) To: linux-kernel Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra, 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, &cache, list) { -@@ -85,6 +94,7 @@ - obj->popularity = 0; - atomic_set(&obj->refcnt, 1); /* The cache holds a reference */ - spin_lock_init(&obj->lock); -+ INIT_RCU_HEAD(&obj->rcu); - - spin_lock_irqsave(&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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD 2010-03-18 19:25 ` [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD Paul E. McKenney @ 2010-03-18 19:35 ` Mathieu Desnoyers 2010-03-18 20:03 ` Paul E. McKenney 0 siblings, 1 reply; 9+ messages in thread From: Mathieu Desnoyers @ 2010-03-18 19:35 UTC (permalink / raw) To: Paul E. McKenney Cc: linux-kernel, mingo, laijs, dipankar, akpm, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra * 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, &cache, list) { > -@@ -85,6 +94,7 @@ > - obj->popularity = 0; > - atomic_set(&obj->refcnt, 1); /* The cache holds a reference */ > - spin_lock_init(&obj->lock); > -+ INIT_RCU_HEAD(&obj->rcu); > - > - spin_lock_irqsave(&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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD 2010-03-18 19:35 ` Mathieu Desnoyers @ 2010-03-18 20:03 ` Paul E. McKenney 2010-03-18 20:22 ` Mathieu Desnoyers 0 siblings, 1 reply; 9+ messages in thread From: Paul E. McKenney @ 2010-03-18 20:03 UTC (permalink / raw) To: Mathieu Desnoyers Cc: linux-kernel, mingo, laijs, dipankar, akpm, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra 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, &cache, list) { > > -@@ -85,6 +94,7 @@ > > - obj->popularity = 0; > > - atomic_set(&obj->refcnt, 1); /* The cache holds a reference */ > > - spin_lock_init(&obj->lock); > > -+ INIT_RCU_HEAD(&obj->rcu); > > - > > - spin_lock_irqsave(&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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD 2010-03-18 20:03 ` Paul E. McKenney @ 2010-03-18 20:22 ` Mathieu Desnoyers 2010-03-18 21:04 ` Paul E. McKenney 0 siblings, 1 reply; 9+ messages in thread From: Mathieu Desnoyers @ 2010-03-18 20:22 UTC (permalink / raw) To: Paul E. McKenney Cc: linux-kernel, mingo, laijs, dipankar, akpm, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra * 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD 2010-03-18 20:22 ` Mathieu Desnoyers @ 2010-03-18 21:04 ` Paul E. McKenney 2010-03-19 0:34 ` Mathieu Desnoyers 0 siblings, 1 reply; 9+ messages in thread From: Paul E. McKenney @ 2010-03-18 21:04 UTC (permalink / raw) To: Mathieu Desnoyers Cc: linux-kernel, mingo, laijs, dipankar, akpm, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD 2010-03-18 21:04 ` Paul E. McKenney @ 2010-03-19 0:34 ` Mathieu Desnoyers 0 siblings, 0 replies; 9+ messages in thread From: Mathieu Desnoyers @ 2010-03-19 0:34 UTC (permalink / raw) To: Paul E. McKenney Cc: linux-kernel, mingo, laijs, dipankar, akpm, josh, dvhltc, niv, tglx, peterz, rostedt, Valdis.Kletnieks, dhowells, eric.dumazet, Alexey Dobriyan, Peter Zijlstra * 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-03-19 0:34 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-03-18 19:25 [PATCH tip/core/urgent 0/2] RCU lockdep fix and shrink RCU API Paul E. McKenney 2010-03-18 19:25 ` [PATCH tip/core/urgent 1/2] rcu: local_irq_disable() also delimits RCU_SCHED read-site critical sections Paul E. McKenney 2010-03-18 22:03 ` [tip:core/urgent] rcu: Fix local_irq_disable() CONFIG_PROVE_RCU=y false positives tip-bot for Lai Jiangshan 2010-03-18 19:25 ` [PATCH tip/core/urgent 2/2] rcu: remove INIT_RCU_HEAD, RCU_HEAD_INIT, RCU_HEAD Paul E. McKenney 2010-03-18 19:35 ` Mathieu Desnoyers 2010-03-18 20:03 ` Paul E. McKenney 2010-03-18 20:22 ` Mathieu Desnoyers 2010-03-18 21:04 ` Paul E. McKenney 2010-03-19 0:34 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox