blk-cgroup: Fix RCU correctness warning in cfq_init_queue()
PATCH 1/1 blk-cgroup: Fix RCU correctness warning in cfq_init_queue()
With RCU correctness on, We see following warning. This patch fixes it.
[ 103.790505] ===================================================
[ 103.790509] [ INFO: suspicious rcu_dereference_check() usage. ]
[ 103.790511] ---------------------------------------------------
[ 103.790514] kernel/cgroup.c:4432 invoked rcu_dereference_check() without protection!
[ 103.790517]
[ 103.790517] other info that might help us debug this:
[ 103.790519]
[ 103.790521]
[ 103.790521] rcu_scheduler_active = 1, debug_locks = 1
[ 103.790524] 4 locks held by bash/4422:
[ 103.790526] #0: (&buffer->mutex){+.+.+.}, at: [<ffffffff8114befa>] sysfs_write_file+0x3c/0x144
[ 103.790537] #1: (s_active#102){.+.+.+}, at: [<ffffffff8114bfa5>] sysfs_write_file+0xe7/0x144
[ 103.790544] #2: (&q->sysfs_lock){+.+.+.}, at: [<ffffffff812263b1>] queue_attr_store+0x49/0x8f
[ 103.790552] #3: (&(&blkcg->lock)->rlock){......}, at: [<ffffffff8122e4db>] blkiocg_add_blkio_group+0x2b/0xad
[ 103.790560]
[ 103.790561] stack backtrace:
[ 103.790564] Pid: 4422, comm: bash Not tainted 2.6.34-rc4-blkio-second-crash #81
[ 103.790567] Call Trace:
[ 103.790572] [<ffffffff81068f57>] lockdep_rcu_dereference+0x9d/0xa5
[ 103.790577] [<ffffffff8107fac1>] css_id+0x44/0x57
[ 103.790581] [<ffffffff8122e503>] blkiocg_add_blkio_group+0x53/0xad
[ 103.790586] [<ffffffff81231936>] cfq_init_queue+0x139/0x32c
[ 103.790591] [<ffffffff8121f2d0>] elv_iosched_store+0xbf/0x1bf
[ 103.790595] [<ffffffff812263d8>] queue_attr_store+0x70/0x8f
[ 103.790599] [<ffffffff8114bfa5>] ? sysfs_write_file+0xe7/0x144
[ 103.790603] [<ffffffff8114bfc6>] sysfs_write_file+0x108/0x144
[ 103.790609] [<ffffffff810f527f>] vfs_write+0xae/0x10b
[ 103.790612] [<ffffffff81069863>] ? trace_hardirqs_on_caller+0x10c/0x130
[ 103.790616] [<ffffffff810f539c>] sys_write+0x4a/0x6e
[ 103.790622] [<ffffffff81002b5b>] system_call_fastpath+0x16/0x1b
[ 103.790625]--- block/cfq-iosched.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
block/cfq-iosched.c +2 -0
@@ -3741,8 +3741,10 @@ static void *cfq_init_queue(struct request_queue *q)* to make sure that cfq_put_cfqg() does not try to kfree root group */ atomic_set (&cfqg->ref,1 ); +rcu_read_lock ();blkiocg_add_blkio_group (&blkio_root_cgroup, &cfqg->blkg, (void *)cfqd,0 ); +rcu_read_unlock ();#endif /* * Not strictly needed (since RB_ROOT just clears the node and we
Signature · 1 line
1.6.2.5
Quoted 1 line · Vivek Goyal
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
(Just wanting to make sure that we are not papering over a real error!)
Thanx, Paul
Quoted 55 lines
[ 103.790505] =================================================== [ 103.790509] [ INFO: suspicious rcu_dereference_check() usage. ] [ 103.790511] --------------------------------------------------- [ 103.790514] kernel/cgroup.c:4432 invoked rcu_dereference_check() without protection! [ 103.790517] [ 103.790517] other info that might help us debug this: [ 103.790519] [ 103.790521] [ 103.790521] rcu_scheduler_active = 1, debug_locks = 1 [ 103.790524] 4 locks held by bash/4422: [ 103.790526] #0: (&buffer->mutex){+.+.+.}, at: [<ffffffff8114befa>] sysfs_write_file+0x3c/0x144 [ 103.790537] #1: (s_active#102){.+.+.+}, at: [<ffffffff8114bfa5>] sysfs_write_file+0xe7/0x144 [ 103.790544] #2: (&q->sysfs_lock){+.+.+.}, at: [<ffffffff812263b1>] queue_attr_store+0x49/0x8f [ 103.790552] #3: (&(&blkcg->lock)->rlock){......}, at: [<ffffffff8122e4db>] blkiocg_add_blkio_group+0x2b/0xad [ 103.790560] [ 103.790561] stack backtrace: [ 103.790564] Pid: 4422, comm: bash Not tainted 2.6.34-rc4-blkio-second-crash #81 [ 103.790567] Call Trace: [ 103.790572] [<ffffffff81068f57>] lockdep_rcu_dereference+0x9d/0xa5 [ 103.790577] [<ffffffff8107fac1>] css_id+0x44/0x57 [ 103.790581] [<ffffffff8122e503>] blkiocg_add_blkio_group+0x53/0xad [ 103.790586] [<ffffffff81231936>] cfq_init_queue+0x139/0x32c [ 103.790591] [<ffffffff8121f2d0>] elv_iosched_store+0xbf/0x1bf [ 103.790595] [<ffffffff812263d8>] queue_attr_store+0x70/0x8f [ 103.790599] [<ffffffff8114bfa5>] ? sysfs_write_file+0xe7/0x144 [ 103.790603] [<ffffffff8114bfc6>] sysfs_write_file+0x108/0x144 [ 103.790609] [<ffffffff810f527f>] vfs_write+0xae/0x10b [ 103.790612] [<ffffffff81069863>] ? trace_hardirqs_on_caller+0x10c/0x130 [ 103.790616] [<ffffffff810f539c>] sys_write+0x4a/0x6e [ 103.790622] [<ffffffff81002b5b>] system_call_fastpath+0x16/0x1b [ 103.790625]Signed-off-by Vivek Goyal <[email protected]>--- block/cfq-iosched.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)block/cfq-iosched.c +2
-0@@ -3741,8 +3741,10 @@ static void *cfq_init_queue(struct request_queue *q)* to make sure that cfq_put_cfqg() does not try to kfree root group */ atomic_set (&cfqg->ref,1 ); +rcu_read_lock ();blkiocg_add_blkio_group (&blkio_root_cgroup, &cfqg->blkg, (void *)cfqd,0 ); +rcu_read_unlock ();#endif /* * Not strictly needed (since RB_ROOT just clears the node and we Signature · 1 line
1.6.2.5
Quoted 6 lines · Paul E. McKenney
Quoted 1 line · Vivek Goyal
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of cfq_init_queue() can be running in parallel (for two different devices), and they both can call blkiocg_add_blkio_group(). But then we use a spin lock to protect blkio_cgroup.
spin_lock_irqsave(&blkcg->lock, flags);
So I guess two parallel updates should be fine.
Thanks
Vivek
Quoted 59 lines
(Just wanting to make sure that we are not papering over a real error!)
Thanx, Paul
Quoted 55 lines
[ 103.790505] =================================================== [ 103.790509] [ INFO: suspicious rcu_dereference_check() usage. ] [ 103.790511] --------------------------------------------------- [ 103.790514] kernel/cgroup.c:4432 invoked rcu_dereference_check() without protection! [ 103.790517] [ 103.790517] other info that might help us debug this: [ 103.790519] [ 103.790521] [ 103.790521] rcu_scheduler_active = 1, debug_locks = 1 [ 103.790524] 4 locks held by bash/4422: [ 103.790526] #0: (&buffer->mutex){+.+.+.}, at: [<ffffffff8114befa>] sysfs_write_file+0x3c/0x144 [ 103.790537] #1: (s_active#102){.+.+.+}, at: [<ffffffff8114bfa5>] sysfs_write_file+0xe7/0x144 [ 103.790544] #2: (&q->sysfs_lock){+.+.+.}, at: [<ffffffff812263b1>] queue_attr_store+0x49/0x8f [ 103.790552] #3: (&(&blkcg->lock)->rlock){......}, at: [<ffffffff8122e4db>] blkiocg_add_blkio_group+0x2b/0xad [ 103.790560] [ 103.790561] stack backtrace: [ 103.790564] Pid: 4422, comm: bash Not tainted 2.6.34-rc4-blkio-second-crash #81 [ 103.790567] Call Trace: [ 103.790572] [<ffffffff81068f57>] lockdep_rcu_dereference+0x9d/0xa5 [ 103.790577] [<ffffffff8107fac1>] css_id+0x44/0x57 [ 103.790581] [<ffffffff8122e503>] blkiocg_add_blkio_group+0x53/0xad [ 103.790586] [<ffffffff81231936>] cfq_init_queue+0x139/0x32c [ 103.790591] [<ffffffff8121f2d0>] elv_iosched_store+0xbf/0x1bf [ 103.790595] [<ffffffff812263d8>] queue_attr_store+0x70/0x8f [ 103.790599] [<ffffffff8114bfa5>] ? sysfs_write_file+0xe7/0x144 [ 103.790603] [<ffffffff8114bfc6>] sysfs_write_file+0x108/0x144 [ 103.790609] [<ffffffff810f527f>] vfs_write+0xae/0x10b [ 103.790612] [<ffffffff81069863>] ? trace_hardirqs_on_caller+0x10c/0x130 [ 103.790616] [<ffffffff810f539c>] sys_write+0x4a/0x6e [ 103.790622] [<ffffffff81002b5b>] system_call_fastpath+0x16/0x1b [ 103.790625]Signed-off-by Vivek Goyal <[email protected]>--- block/cfq-iosched.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)block/cfq-iosched.c +2
-0@@ -3741,8 +3741,10 @@ static void *cfq_init_queue(struct request_queue *q)* to make sure that cfq_put_cfqg() does not try to kfree root group */ atomic_set (&cfqg->ref,1 ); +rcu_read_lock ();blkiocg_add_blkio_group (&blkio_root_cgroup, &cfqg->blkg, (void *)cfqd,0 ); +rcu_read_unlock ();#endif /* * Not strictly needed (since RB_ROOT just clears the node and we Signature · 1 line
1.6.2.5
Quoted 8 lines · Vivek Goyal
Quoted 6 lines · Paul E. McKenney
Quoted 1 line · Vivek Goyal
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
Quoted 8 lines
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s rcu_dereference_check()? At first glance, css_id() needs to gain access to the blkio_cgroup structure that references the cgroup_subsys_state structure passed to css_id().
This means that there is only one blkio_cgroup structure referencing a given cgroup_subsys_state structure, right? Otherwise, we could still have concurrent access.
Thanx, Paul
Quoted 62 lines
Thanks
VivekQuoted 59 lines
(Just wanting to make sure that we are not papering over a real error!)
Thanx, Paul
Quoted 55 lines
[ 103.790505] =================================================== [ 103.790509] [ INFO: suspicious rcu_dereference_check() usage. ] [ 103.790511] --------------------------------------------------- [ 103.790514] kernel/cgroup.c:4432 invoked rcu_dereference_check() without protection! [ 103.790517] [ 103.790517] other info that might help us debug this: [ 103.790519] [ 103.790521] [ 103.790521] rcu_scheduler_active = 1, debug_locks = 1 [ 103.790524] 4 locks held by bash/4422: [ 103.790526] #0: (&buffer->mutex){+.+.+.}, at: [<ffffffff8114befa>] sysfs_write_file+0x3c/0x144 [ 103.790537] #1: (s_active#102){.+.+.+}, at: [<ffffffff8114bfa5>] sysfs_write_file+0xe7/0x144 [ 103.790544] #2: (&q->sysfs_lock){+.+.+.}, at: [<ffffffff812263b1>] queue_attr_store+0x49/0x8f [ 103.790552] #3: (&(&blkcg->lock)->rlock){......}, at: [<ffffffff8122e4db>] blkiocg_add_blkio_group+0x2b/0xad [ 103.790560] [ 103.790561] stack backtrace: [ 103.790564] Pid: 4422, comm: bash Not tainted 2.6.34-rc4-blkio-second-crash #81 [ 103.790567] Call Trace: [ 103.790572] [<ffffffff81068f57>] lockdep_rcu_dereference+0x9d/0xa5 [ 103.790577] [<ffffffff8107fac1>] css_id+0x44/0x57 [ 103.790581] [<ffffffff8122e503>] blkiocg_add_blkio_group+0x53/0xad [ 103.790586] [<ffffffff81231936>] cfq_init_queue+0x139/0x32c [ 103.790591] [<ffffffff8121f2d0>] elv_iosched_store+0xbf/0x1bf [ 103.790595] [<ffffffff812263d8>] queue_attr_store+0x70/0x8f [ 103.790599] [<ffffffff8114bfa5>] ? sysfs_write_file+0xe7/0x144 [ 103.790603] [<ffffffff8114bfc6>] sysfs_write_file+0x108/0x144 [ 103.790609] [<ffffffff810f527f>] vfs_write+0xae/0x10b [ 103.790612] [<ffffffff81069863>] ? trace_hardirqs_on_caller+0x10c/0x130 [ 103.790616] [<ffffffff810f539c>] sys_write+0x4a/0x6e [ 103.790622] [<ffffffff81002b5b>] system_call_fastpath+0x16/0x1b [ 103.790625]Signed-off-by Vivek Goyal <[email protected]>--- block/cfq-iosched.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)block/cfq-iosched.c +2
-0@@ -3741,8 +3741,10 @@ static void *cfq_init_queue(struct request_queue *q)* to make sure that cfq_put_cfqg() does not try to kfree root group */ atomic_set (&cfqg->ref,1 ); +rcu_read_lock ();blkiocg_add_blkio_group (&blkio_root_cgroup, &cfqg->blkg, (void *)cfqd,0 ); +rcu_read_unlock ();#endif /* * Not strictly needed (since RB_ROOT just clears the node and we Signature · 1 line
1.6.2.5
Quoted 14 lines · Paul E. McKenney
Quoted 13 lines · Vivek Goyal
Quoted 6 lines · Paul E. McKenney
Quoted 1 line · Vivek Goyal
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.
Quoted 6 lines
Quoted 3 lines
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s
rcu_dereference_check()?
Hi Paul,
I think adding these spinlock to condition checked might become little messy. And the reason being that this lock is subsystem (controller) specific and maintained by controller. Now if any controller implements a lock and we add that lock in css_id() rcu_dereference_check(), it will look ugly.
So probably a better way is to make sure that css_id() is always called under rcu read lock so that we don't hit this warning?
Quoted 7 lines
At first glance,
css_id()
needs to gain access to theblkio_cgroupstructure that references thecgroup_subsys_statestructure passed tocss_id().This means that there is only one
blkio_cgroupstructure referencing a givencgroup_subsys_statestructure, right? Otherwise, we could still have concurrent access.
Yes. In fact css object is embedded in blkio_cgroup structure. So we take a rcu_read_lock() so that data structures associated with cgroup subsystem don't go away and then take controller specific blkio_cgroup spin lock to make sure multiple writers don't end up modifying a list at the same time.
Am I missing something.
Thanks
Vivek
Quoted 59 lines
Quoted 59 lines
Quoted 59 lines
(Just wanting to make sure that we are not papering over a real error!)
Thanx, Paul
Quoted 55 lines
[ 103.790505] =================================================== [ 103.790509] [ INFO: suspicious rcu_dereference_check() usage. ] [ 103.790511] --------------------------------------------------- [ 103.790514] kernel/cgroup.c:4432 invoked rcu_dereference_check() without protection! [ 103.790517] [ 103.790517] other info that might help us debug this: [ 103.790519] [ 103.790521] [ 103.790521] rcu_scheduler_active = 1, debug_locks = 1 [ 103.790524] 4 locks held by bash/4422: [ 103.790526] #0: (&buffer->mutex){+.+.+.}, at: [<ffffffff8114befa>] sysfs_write_file+0x3c/0x144 [ 103.790537] #1: (s_active#102){.+.+.+}, at: [<ffffffff8114bfa5>] sysfs_write_file+0xe7/0x144 [ 103.790544] #2: (&q->sysfs_lock){+.+.+.}, at: [<ffffffff812263b1>] queue_attr_store+0x49/0x8f [ 103.790552] #3: (&(&blkcg->lock)->rlock){......}, at: [<ffffffff8122e4db>] blkiocg_add_blkio_group+0x2b/0xad [ 103.790560] [ 103.790561] stack backtrace: [ 103.790564] Pid: 4422, comm: bash Not tainted 2.6.34-rc4-blkio-second-crash #81 [ 103.790567] Call Trace: [ 103.790572] [<ffffffff81068f57>] lockdep_rcu_dereference+0x9d/0xa5 [ 103.790577] [<ffffffff8107fac1>] css_id+0x44/0x57 [ 103.790581] [<ffffffff8122e503>] blkiocg_add_blkio_group+0x53/0xad [ 103.790586] [<ffffffff81231936>] cfq_init_queue+0x139/0x32c [ 103.790591] [<ffffffff8121f2d0>] elv_iosched_store+0xbf/0x1bf [ 103.790595] [<ffffffff812263d8>] queue_attr_store+0x70/0x8f [ 103.790599] [<ffffffff8114bfa5>] ? sysfs_write_file+0xe7/0x144 [ 103.790603] [<ffffffff8114bfc6>] sysfs_write_file+0x108/0x144 [ 103.790609] [<ffffffff810f527f>] vfs_write+0xae/0x10b [ 103.790612] [<ffffffff81069863>] ? trace_hardirqs_on_caller+0x10c/0x130 [ 103.790616] [<ffffffff810f539c>] sys_write+0x4a/0x6e [ 103.790622] [<ffffffff81002b5b>] system_call_fastpath+0x16/0x1b [ 103.790625]Signed-off-by Vivek Goyal <[email protected]>--- block/cfq-iosched.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)block/cfq-iosched.c +2
-0@@ -3741,8 +3741,10 @@ static void *cfq_init_queue(struct request_queue *q)* to make sure that cfq_put_cfqg() does not try to kfree root group */ atomic_set (&cfqg->ref,1 ); +rcu_read_lock ();blkiocg_add_blkio_group (&blkio_root_cgroup, &cfqg->blkg, (void *)cfqd,0 ); +rcu_read_unlock ();#endif /* * Not strictly needed (since RB_ROOT just clears the node and we Signature · 1 line
1.6.2.5
Quoted 24 lines · Vivek Goyal
Quoted 14 lines · Paul E. McKenney
Quoted 13 lines · Vivek Goyal
Quoted 6 lines · Paul E. McKenney
Quoted 1 line · Vivek Goyal
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.Quoted 6 lines
Quoted 3 lines
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s
rcu_dereference_check()?Hi Paul,
Quoted 8 lines
I think adding these spinlock to condition checked might become little messy. And the reason being that this lock is subsystem (controller) specific and maintained by controller. Now if any controller implements a lock and we add that lock in
css_id()rcu_dereference_check(), it will look ugly.So probably a better way is to make sure that
css_id()is always called under rcu read lock so that we don't hit this warning?
As long as holding rcu_read_lock() prevents css_id() from the usual problems such as access memory that was concurrently freed, yes.
Quoted 8 lines
Quoted 7 lines
At first glance,
css_id()
needs to gain access to theblkio_cgroupstructure that references thecgroup_subsys_statestructure passed tocss_id().This means that there is only one
blkio_cgroupstructure referencing a givencgroup_subsys_statestructure, right? Otherwise, we could still have concurrent access.
Quoted 6 lines
Yes. In fact css object is embedded in
blkio_cgroupstructure. So we take arcu_read_lock()so that data structures associated with cgroup subsystem don't go away and then take controller specificblkio_cgroupspin lock to make sure multiple writers don't end up modifying a list at the same time.Am I missing something.
This sounds very good!
I did have to ask! ;-)
Thanx, Paul
Quoted 63 lines
Thanks
VivekQuoted 59 lines
Quoted 59 lines
Quoted 59 lines
(Just wanting to make sure that we are not papering over a real error!)
Thanx, Paul
Quoted 55 lines
[ 103.790505] =================================================== [ 103.790509] [ INFO: suspicious rcu_dereference_check() usage. ] [ 103.790511] --------------------------------------------------- [ 103.790514] kernel/cgroup.c:4432 invoked rcu_dereference_check() without protection! [ 103.790517] [ 103.790517] other info that might help us debug this: [ 103.790519] [ 103.790521] [ 103.790521] rcu_scheduler_active = 1, debug_locks = 1 [ 103.790524] 4 locks held by bash/4422: [ 103.790526] #0: (&buffer->mutex){+.+.+.}, at: [<ffffffff8114befa>] sysfs_write_file+0x3c/0x144 [ 103.790537] #1: (s_active#102){.+.+.+}, at: [<ffffffff8114bfa5>] sysfs_write_file+0xe7/0x144 [ 103.790544] #2: (&q->sysfs_lock){+.+.+.}, at: [<ffffffff812263b1>] queue_attr_store+0x49/0x8f [ 103.790552] #3: (&(&blkcg->lock)->rlock){......}, at: [<ffffffff8122e4db>] blkiocg_add_blkio_group+0x2b/0xad [ 103.790560] [ 103.790561] stack backtrace: [ 103.790564] Pid: 4422, comm: bash Not tainted 2.6.34-rc4-blkio-second-crash #81 [ 103.790567] Call Trace: [ 103.790572] [<ffffffff81068f57>] lockdep_rcu_dereference+0x9d/0xa5 [ 103.790577] [<ffffffff8107fac1>] css_id+0x44/0x57 [ 103.790581] [<ffffffff8122e503>] blkiocg_add_blkio_group+0x53/0xad [ 103.790586] [<ffffffff81231936>] cfq_init_queue+0x139/0x32c [ 103.790591] [<ffffffff8121f2d0>] elv_iosched_store+0xbf/0x1bf [ 103.790595] [<ffffffff812263d8>] queue_attr_store+0x70/0x8f [ 103.790599] [<ffffffff8114bfa5>] ? sysfs_write_file+0xe7/0x144 [ 103.790603] [<ffffffff8114bfc6>] sysfs_write_file+0x108/0x144 [ 103.790609] [<ffffffff810f527f>] vfs_write+0xae/0x10b [ 103.790612] [<ffffffff81069863>] ? trace_hardirqs_on_caller+0x10c/0x130 [ 103.790616] [<ffffffff810f539c>] sys_write+0x4a/0x6e [ 103.790622] [<ffffffff81002b5b>] system_call_fastpath+0x16/0x1b [ 103.790625]Signed-off-by Vivek Goyal <[email protected]>--- block/cfq-iosched.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)block/cfq-iosched.c +2
-0@@ -3741,8 +3741,10 @@ static void *cfq_init_queue(struct request_queue *q)* to make sure that cfq_put_cfqg() does not try to kfree root group */ atomic_set (&cfqg->ref,1 ); +rcu_read_lock ();blkiocg_add_blkio_group (&blkio_root_cgroup, &cfqg->blkg, (void *)cfqd,0 ); +rcu_read_unlock ();#endif /* * Not strictly needed (since RB_ROOT just clears the node and we Signature · 1 line
1.6.2.5
Quoted 22 lines
Quoted 22 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.Quoted 5 lines
Quoted 3 lines
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s
rcu_dereference_check()?Hi Paul,
I think adding these spinlock to condition checked might become little messy. And the reason being that this lock is subsystem (controller) specific and maintained by controller. Now if any controller implements a lock and we add that lock in
css_id()rcu_dereference_check(), it will look ugly.
Quoted 6 lines
Quoted 2 lines
So probably a better way is to make sure that
css_id()is always called under rcu read lock so that we don't hit this warning?As long as holding
rcu_read_lock()preventscss_id()from the usual problems such as access memory that was concurrently freed, yes.
blkiocg_add_blkio_group() also calls cgroup_path(), which also needs to be called within rcu_read_lock, so I think Vivek's patch is better than the one you posted in another mail thread.
Quoted 25 lines · Li Zefan
Quoted 25 lines
Quoted 24 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.Quoted 5 lines
Quoted 3 lines
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s
rcu_dereference_check()?Hi Paul,
I think adding these spinlock to condition checked might become little messy. And the reason being that this lock is subsystem (controller) specific and maintained by controller. Now if any controller implements a lock and we add that lock in
css_id()rcu_dereference_check(), it will look ugly.So probably a better way is to make sure that
css_id()is always called under rcu read lock so that we don't hit this warning?
Quoted 6 lines
Quoted 2 lines
As long as holding
rcu_read_lock()preventscss_id()from the usual problems such as access memory that was concurrently freed, yes.
blkiocg_add_blkio_group()also callscgroup_path(), which also needs to be called withinrcu_read_lock, so I think Vivek's patch is better than the one you posted in another mail thread.
My apologies, Vivek! I lost track of your patch. I have now replaced my patch with yours.
Thanx, Paul
Quoted 29 lines · Paul E. McKenney
Quoted 28 lines · Li Zefan
Quoted 27 lines
Quoted 16 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.Quoted 5 lines
Quoted 3 lines
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s
rcu_dereference_check()?Hi Paul,
Quoted 8 lines
I think adding these spinlock to condition checked might become little messy. And the reason being that this lock is subsystem (controller) specific and maintained by controller. Now if any controller implements a lock and we add that lock in
css_id()rcu_dereference_check(), it will look ugly.So probably a better way is to make sure that
css_id()is always called under rcu read lock so that we don't hit this warning?As long as holding
rcu_read_lock()preventscss_id()from the usual problems such as access memory that was concurrently freed, yes.
Quoted 6 lines
Quoted 3 lines
blkiocg_add_blkio_group()also callscgroup_path(), which also needs to be called withinrcu_read_lock, so I think Vivek's patch is better than the one you posted in another mail thread.My apologies, Vivek! I lost track of your patch. I have now replaced my patch with yours.
Thanks Paul.
I sent this patch to Jens also, thinking he will apply to his tree. Looks like he has not applied it yet though.
Jens, is it ok if this patch gets merged through paul's tree or should it go through blk tree?
Thanks
Vivek
Quoted 37 lines · Vivek Goyal
Quoted 29 lines · Paul E. McKenney
Quoted 28 lines · Li Zefan
Quoted 27 lines
Quoted 16 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.Quoted 5 lines
Quoted 3 lines
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s
rcu_dereference_check()?Hi Paul,
Quoted 8 lines
I think adding these spinlock to condition checked might become little messy. And the reason being that this lock is subsystem (controller) specific and maintained by controller. Now if any controller implements a lock and we add that lock in
css_id()rcu_dereference_check(), it will look ugly.So probably a better way is to make sure that
css_id()is always called under rcu read lock so that we don't hit this warning?As long as holding
rcu_read_lock()preventscss_id()from the usual problems such as access memory that was concurrently freed, yes.Quoted 6 lines
Quoted 3 lines
blkiocg_add_blkio_group()also callscgroup_path(), which also needs to be called withinrcu_read_lock, so I think Vivek's patch is better than the one you posted in another mail thread.My apologies, Vivek! I lost track of your patch. I have now replaced my patch with yours.
Quoted 7 lines
Thanks Paul.
I sent this patch to Jens also, thinking he will apply to his tree. Looks like he has not applied it yet though.
Jens, is it ok if this patch gets merged through paul's tree or should it go through blk tree?
I am happy for it to go either way, so just let me know!
Thanx, Paul
Quoted 40 lines · Paul E. McKenney
Quoted 39 lines · Vivek Goyal
Quoted 29 lines · Paul E. McKenney
Quoted 28 lines · Li Zefan
Quoted 27 lines
Quoted 16 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.Quoted 5 lines
Quoted 3 lines
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s
rcu_dereference_check()?Hi Paul,
Quoted 8 lines
I think adding these spinlock to condition checked might become little messy. And the reason being that this lock is subsystem (controller) specific and maintained by controller. Now if any controller implements a lock and we add that lock in
css_id()rcu_dereference_check(), it will look ugly.So probably a better way is to make sure that
css_id()is always called under rcu read lock so that we don't hit this warning?As long as holding
rcu_read_lock()preventscss_id()from the usual problems such as access memory that was concurrently freed, yes.Quoted 6 lines
Quoted 3 lines
blkiocg_add_blkio_group()also callscgroup_path(), which also needs to be called withinrcu_read_lock, so I think Vivek's patch is better than the one you posted in another mail thread.My apologies, Vivek! I lost track of your patch. I have now replaced my patch with yours.
Thanks Paul.
Quoted 7 lines
Quoted 5 lines
I sent this patch to Jens also, thinking he will apply to his tree. Looks like he has not applied it yet though.
Jens, is it ok if this patch gets merged through paul's tree or should it go through blk tree?
I am happy for it to go either way, so just let me know!
I am also happy to go either way. I guess you can go ahead with pulling it in.
Thanks
Vivek
Quoted 44 lines · Vivek Goyal
Quoted 43 lines · Paul E. McKenney
Quoted 42 lines · Vivek Goyal
Quoted 29 lines · Paul E. McKenney
Quoted 28 lines · Li Zefan
Quoted 27 lines
Quoted 16 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.Quoted 5 lines
Quoted 3 lines
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s
rcu_dereference_check()?Hi Paul,
Quoted 8 lines
I think adding these spinlock to condition checked might become little messy. And the reason being that this lock is subsystem (controller) specific and maintained by controller. Now if any controller implements a lock and we add that lock in
css_id()rcu_dereference_check(), it will look ugly.So probably a better way is to make sure that
css_id()is always called under rcu read lock so that we don't hit this warning?As long as holding
rcu_read_lock()preventscss_id()from the usual problems such as access memory that was concurrently freed, yes.Quoted 6 lines
Quoted 3 lines
blkiocg_add_blkio_group()also callscgroup_path(), which also needs to be called withinrcu_read_lock, so I think Vivek's patch is better than the one you posted in another mail thread.My apologies, Vivek! I lost track of your patch. I have now replaced my patch with yours.
Thanks Paul.
I sent this patch to Jens also, thinking he will apply to his tree. Looks like he has not applied it yet though.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Jens, is it ok if this patch gets merged through paul's tree or should it go through blk tree?
I am happy for it to go either way, so just let me know!
I am also happy to go either way. I guess you can go ahead with pulling it in.
I have it queued for 2.6.34.
Thanx, Paul
Quoted 45 lines · Paul E. McKenney
Quoted 44 lines · Vivek Goyal
Quoted 43 lines · Paul E. McKenney
Quoted 42 lines · Vivek Goyal
Quoted 29 lines · Paul E. McKenney
Quoted 28 lines · Li Zefan
Quoted 27 lines
Quoted 16 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.Quoted 5 lines
Quoted 3 lines
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s
rcu_dereference_check()?Hi Paul,
Quoted 8 lines
I think adding these spinlock to condition checked might become little messy. And the reason being that this lock is subsystem (controller) specific and maintained by controller. Now if any controller implements a lock and we add that lock in
css_id()rcu_dereference_check(), it will look ugly.So probably a better way is to make sure that
css_id()is always called under rcu read lock so that we don't hit this warning?As long as holding
rcu_read_lock()preventscss_id()from the usual problems such as access memory that was concurrently freed, yes.Quoted 6 lines
Quoted 3 lines
blkiocg_add_blkio_group()also callscgroup_path(), which also needs to be called withinrcu_read_lock, so I think Vivek's patch is better than the one you posted in another mail thread.My apologies, Vivek! I lost track of your patch. I have now replaced my patch with yours.
Thanks Paul.
I sent this patch to Jens also, thinking he will apply to his tree. Looks like he has not applied it yet though.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Jens, is it ok if this patch gets merged through paul's tree or should it go through blk tree?
I am happy for it to go either way, so just let me know!
I am also happy to go either way. I guess you can go ahead with pulling it in.
I have it queued for 2.6.34.
Thanks Paul. Where can I get your tree to clone from? I can test the changes. I can't find it on kernel.org.
Thanks
Vivek
Quoted 49 lines · Vivek Goyal
Quoted 48 lines · Paul E. McKenney
Quoted 47 lines · Vivek Goyal
Quoted 46 lines · Paul E. McKenney
Quoted 44 lines · Vivek Goyal
Quoted 29 lines · Paul E. McKenney
Quoted 28 lines · Li Zefan
Quoted 27 lines
Quoted 16 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
With RCU correctness on, We see following warning. This patch fixes it.
This is in initialization code, so that there cannot be any concurrent updates, correct? If so, looks good.
I think theoritically two instances of
cfq_init_queue()can be running in parallel (for two different devices), and they both can callblkiocg_add_blkio_group(). But then we use a spin lock to protectblkio_cgroup.Quoted 5 lines
Quoted 3 lines
spin_lock_irqsave(&blkcg->lock, flags);So I guess two parallel updates should be fine.
OK, in that case, would it be possible add this spinlock to the condition checked by css_id()'s
rcu_dereference_check()?Hi Paul,
Quoted 8 lines
I think adding these spinlock to condition checked might become little messy. And the reason being that this lock is subsystem (controller) specific and maintained by controller. Now if any controller implements a lock and we add that lock in
css_id()rcu_dereference_check(), it will look ugly.So probably a better way is to make sure that
css_id()is always called under rcu read lock so that we don't hit this warning?As long as holding
rcu_read_lock()preventscss_id()from the usual problems such as access memory that was concurrently freed, yes.Quoted 6 lines
Quoted 3 lines
blkiocg_add_blkio_group()also callscgroup_path(), which also needs to be called withinrcu_read_lock, so I think Vivek's patch is better than the one you posted in another mail thread.My apologies, Vivek! I lost track of your patch. I have now replaced my patch with yours.
Thanks Paul.
I sent this patch to Jens also, thinking he will apply to his tree. Looks like he has not applied it yet though.
Jens, is it ok if this patch gets merged through paul's tree or should it go through blk tree?
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
I am happy for it to go either way, so just let me know!
I am also happy to go either way. I guess you can go ahead with pulling it in.
I have it queued for 2.6.34.
Thanks Paul. Where can I get your tree to clone from? I can test the changes. I can't find it on kernel.org.
I have a early-alpha version of an RCU git tree at:
master.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-2.6-rcutest.git
This is my first attempt to create such a tree, so please let me know if you run into problems trying to use it. The branch "urgent.2010.40.27a", complete with a dyslexic month of April, contains the RCU lockdep fixes that are not known to me to be in other trees. The branch "rcutest.2010.04.27a" contains stuff intended for 2.6.35.
I expect that I will learn how one -really- sets up a tree over the next little while, at which point I will delete the above and create a real one. A more real one, anyway...
Thanx, Paul