Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310215 > unrolled thread

Re: regression 4.4: deadlock in with cgroup percpu_rwsem

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: regression 4.4: deadlock in with cgroup percpu_rwsem Christian Borntraeger <borntraeger@de.ibm.com> - 2016-01-15 16:20 +0100
    Re: regression 4.4: deadlock in with cgroup percpu_rwsem Peter Zijlstra <peterz@infradead.org> - 2016-01-18 19:40 +0100
      Re: regression 4.4: deadlock in with cgroup percpu_rwsem Christian Borntraeger <borntraeger@de.ibm.com> - 2016-01-18 19:50 +0100
        Re: regression 4.4: deadlock in with cgroup percpu_rwsem Heiko Carstens <heiko.carstens@de.ibm.com> - 2016-01-19 11:00 +0100
          Re: regression 4.4: deadlock in with cgroup percpu_rwsem Christian Borntraeger <borntraeger@de.ibm.com> - 2016-01-19 20:40 +0100
            Re: regression 4.4: deadlock in with cgroup percpu_rwsem Tejun Heo <tj@kernel.org> - 2016-01-19 20:40 +0100
              Re: regression 4.4: deadlock in with cgroup percpu_rwsem Heiko Carstens <heiko.carstens@de.ibm.com> - 2016-01-20 08:10 +0100
                Re: regression 4.4: deadlock in with cgroup percpu_rwsem Christian Borntraeger <borntraeger@de.ibm.com> - 2016-01-20 11:20 +0100
                  Re: regression 4.4: deadlock in with cgroup percpu_rwsem Peter Zijlstra <peterz@infradead.org> - 2016-01-20 11:40 +0100
                    Re: regression 4.4: deadlock in with cgroup percpu_rwsem Peter Zijlstra <peterz@infradead.org> - 2016-01-20 11:50 +0100
                      Re: regression 4.4: deadlock in with cgroup percpu_rwsem Tejun Heo <tj@kernel.org> - 2016-01-20 16:40 +0100
                        Re: regression 4.4: deadlock in with cgroup percpu_rwsem Tejun Heo <tj@kernel.org> - 2016-01-20 17:10 +0100
                          Re: regression 4.4: deadlock in with cgroup percpu_rwsem Peter Zijlstra <peterz@infradead.org> - 2016-01-20 17:50 +0100
                            Re: regression 4.4: deadlock in with cgroup percpu_rwsem Tejun Heo <tj@kernel.org> - 2016-01-20 18:00 +0100
                        Re: regression 4.4: deadlock in with cgroup percpu_rwsem "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-01-23 03:10 +0100
                          Re: regression 4.4: deadlock in with cgroup percpu_rwsem Christoph Hellwig <hch@lst.de> - 2016-01-25 09:50 +0100
                            Re: regression 4.4: deadlock in with cgroup percpu_rwsem Tejun Heo <tj@kernel.org> - 2016-01-25 20:40 +0100
                              Re: regression 4.4: deadlock in with cgroup percpu_rwsem Christoph Hellwig <hch@lst.de> - 2016-01-26 16:00 +0100
                                Re: regression 4.4: deadlock in with cgroup percpu_rwsem Tejun Heo <tj@kernel.org> - 2016-01-26 16:30 +0100
                                  Re: regression 4.4: deadlock in with cgroup percpu_rwsem Christoph Hellwig <hch@lst.de> - 2016-01-26 17:50 +0100
                    Re: regression 4.4: deadlock in with cgroup percpu_rwsem Peter Zijlstra <peterz@infradead.org> - 2016-01-20 12:00 +0100
                      Re: regression 4.4: deadlock in with cgroup percpu_rwsem Christian Borntraeger <borntraeger@de.ibm.com> - 2016-01-21 09:30 +0100
                        Re: regression 4.4: deadlock in with cgroup percpu_rwsem Peter Zijlstra <peterz@infradead.org> - 2016-01-21 10:30 +0100

Page 1 of 2  [1] 2  Next page →

#1310215 — Re: regression 4.4: deadlock in with cgroup percpu_rwsem

On 01/15/2016 08:30 AM, Christian Borntraeger wrote:
> On 01/14/2016 08:56 PM, Tejun Heo wrote:
>> Hello,
>>
>> Thanks a lot for the report and detailed analysis.  Can you please
>> test whether the following patch fixes the issue?
>>
>> Thanks.
>>
> 
> 
> Yes, the deadlock is gone and the system is still running.
> After some time I had the following WARN in the logs, though.
> Not sure yet if that is related.
> 
> [25331.763607] DEBUG_LOCKS_WARN_ON(lock->owner != current)
> [25331.763630] ------------[ cut here ]------------
> [25331.763634] WARNING: at kernel/locking/mutex-debug.c:80
> [25331.763637] Modules linked in: nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp iptable_filter ip_tables x_tables bridge stp llc btrfs xor raid6_pq ghash_s390 prng ecb aes_s390 des_s390 des_generic sha512_s390 sha256_s390 sha1_s390 sha_common eadm_sch nfsd auth_rpcgss oid_registry nfs_acl lockd vhost_net tun vhost macvtap macvlan grace sunrpc dm_service_time dm_multipath dm_mod autofs4
> [25331.763708] CPU: 56 PID: 114657 Comm: systemd-udevd Not tainted 4.4.0+ #91
> [25331.763711] task: 000000fadc79de40 ti: 000000f95e7f8000 task.ti: 000000f95e7f8000
> [25331.763715] Krnl PSW : 0404c00180000000 00000000001b7f32 (debug_mutex_unlock+0x16a/0x188)
> [25331.763726]            R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 EA:3
> Krnl GPRS: 0000004c00000037 000000fadc79de40 000000000000002b 0000000000000000
> [25331.763732]            000000000028da3c 0000000000000000 000000f95e7fbf08 000000fab8e10df0
> [25331.763735]            000000000000005c 000000facc0dc000 000000000000005c 000000000033e14a
> [25331.763738]            0700000000000000 000000fab8e10df0 00000000001b7f2e 000000f95e7fbc80
> [25331.763746] Krnl Code: 00000000001b7f22: c0200042784c	larl	%r2,a06fba
>            00000000001b7f28: c0e50006ad50	brasl	%r14,28d9c8
>           #00000000001b7f2e: a7f40001		brc	15,1b7f30
>           >00000000001b7f32: a7f4ffe1		brc	15,1b7ef4
>            00000000001b7f36: c03000429c9f	larl	%r3,a0b874
>            00000000001b7f3c: c0200042783f	larl	%r2,a06fba
>            00000000001b7f42: c0e50006ad43	brasl	%r14,28d9c8
>            00000000001b7f48: a7f40001		brc	15,1b7f4a
> [25331.763795] Call Trace:
> [25331.763798] ([<00000000001b7f2e>] debug_mutex_unlock+0x166/0x188)
> [25331.763804]  [<0000000000836a08>] __mutex_unlock_slowpath+0xa8/0x190
> [25331.763808]  [<000000000033e14a>] seq_read+0x1c2/0x450
> [25331.763813]  [<0000000000311e72>] __vfs_read+0x42/0x100
> [25331.763818]  [<000000000031284e>] vfs_read+0x76/0x130
> [25331.763821]  [<000000000031361e>] SyS_read+0x66/0xd8
> [25331.763826]  [<000000000083af06>] system_call+0xd6/0x270
> [25331.763829]  [<000003ffae1f19c8>] 0x3ffae1f19c8
> [25331.763831] INFO: lockdep is turned off.
> [25331.763833] Last Breaking-Event-Address:
> [25331.763836]  [<00000000001b7f2e>] debug_mutex_unlock+0x166/0x188
> [25331.763839] ---[ end trace 45177640eb39ef44 ]---
> 

I restarted the test with panic_on_warn. Hopefully I can get a dump to check
which mutex this was.

Christian

[toc] | [next] | [standalone]

#1311740

On Fri, Jan 15, 2016 at 04:13:34PM +0100, Christian Borntraeger wrote:
> > Yes, the deadlock is gone and the system is still running.
> > After some time I had the following WARN in the logs, though.
> > Not sure yet if that is related.
> > 
> > [25331.763607] DEBUG_LOCKS_WARN_ON(lock->owner != current)
> > [25331.763630] ------------[ cut here ]------------
> > [25331.763634] WARNING: at kernel/locking/mutex-debug.c:80

> I restarted the test with panic_on_warn. Hopefully I can get a dump to check
> which mutex this was.

Hard to reproduce warnings like this tend to point towards memory
corruption. Someone stepped on the mutex value and tickles the sanity
check.

With lockdep and debugging enabled the mutex gets quite a bit bigger, so
it gets more likely to be hit by 'random' corruption.

The locking in seq_read() seems rather straight forward.

[toc] | [prev] | [next] | [standalone]

#1311744

On 01/18/2016 07:32 PM, Peter Zijlstra wrote:
> On Fri, Jan 15, 2016 at 04:13:34PM +0100, Christian Borntraeger wrote:
>>> Yes, the deadlock is gone and the system is still running.
>>> After some time I had the following WARN in the logs, though.
>>> Not sure yet if that is related.
>>>
>>> [25331.763607] DEBUG_LOCKS_WARN_ON(lock->owner != current)
>>> [25331.763630] ------------[ cut here ]------------
>>> [25331.763634] WARNING: at kernel/locking/mutex-debug.c:80
> 
>> I restarted the test with panic_on_warn. Hopefully I can get a dump to check
>> which mutex this was.
> 
> Hard to reproduce warnings like this tend to point towards memory
> corruption. Someone stepped on the mutex value and tickles the sanity
> check.
> 
> With lockdep and debugging enabled the mutex gets quite a bit bigger, so
> it gets more likely to be hit by 'random' corruption.
> 
> The locking in seq_read() seems rather straight forward.

I was able to reproduce. The dump shows a mutex that has an owner field, which
does not exists as a task so this all looks fishy. The good thing is, that I
can reproduce the issue within some hours. (exact same backtrace). Will add some
more debug data to get a handle where we come from.

[toc] | [prev] | [next] | [standalone]

#1312056

On Mon, Jan 18, 2016 at 07:48:16PM +0100, Christian Borntraeger wrote:
> On 01/18/2016 07:32 PM, Peter Zijlstra wrote:
> > On Fri, Jan 15, 2016 at 04:13:34PM +0100, Christian Borntraeger wrote:
> >>> Yes, the deadlock is gone and the system is still running.
> >>> After some time I had the following WARN in the logs, though.
> >>> Not sure yet if that is related.
> >>>
> >>> [25331.763607] DEBUG_LOCKS_WARN_ON(lock->owner != current)
> >>> [25331.763630] ------------[ cut here ]------------
> >>> [25331.763634] WARNING: at kernel/locking/mutex-debug.c:80
> > 
> >> I restarted the test with panic_on_warn. Hopefully I can get a dump to check
> >> which mutex this was.
> > 
> > Hard to reproduce warnings like this tend to point towards memory
> > corruption. Someone stepped on the mutex value and tickles the sanity
> > check.
> > 
> > With lockdep and debugging enabled the mutex gets quite a bit bigger, so
> > it gets more likely to be hit by 'random' corruption.
> > 
> > The locking in seq_read() seems rather straight forward.
> 
> I was able to reproduce. The dump shows a mutex that has an owner field, which
> does not exists as a task so this all looks fishy. The good thing is, that I
> can reproduce the issue within some hours. (exact same backtrace). Will add some
> more debug data to get a handle where we come from.

Did the owner field show to something that still looks like a task_struct?

[toc] | [prev] | [next] | [standalone]

#1312466

On 01/19/2016 10:55 AM, Heiko Carstens wrote:
> On Mon, Jan 18, 2016 at 07:48:16PM +0100, Christian Borntraeger wrote:
>> On 01/18/2016 07:32 PM, Peter Zijlstra wrote:
>>> On Fri, Jan 15, 2016 at 04:13:34PM +0100, Christian Borntraeger wrote:
>>>>> Yes, the deadlock is gone and the system is still running.
>>>>> After some time I had the following WARN in the logs, though.
>>>>> Not sure yet if that is related.
>>>>>
>>>>> [25331.763607] DEBUG_LOCKS_WARN_ON(lock->owner != current)
>>>>> [25331.763630] ------------[ cut here ]------------
>>>>> [25331.763634] WARNING: at kernel/locking/mutex-debug.c:80
>>>
>>>> I restarted the test with panic_on_warn. Hopefully I can get a dump to check
>>>> which mutex this was.
>>>
>>> Hard to reproduce warnings like this tend to point towards memory
>>> corruption. Someone stepped on the mutex value and tickles the sanity
>>> check.
>>>
>>> With lockdep and debugging enabled the mutex gets quite a bit bigger, so
>>> it gets more likely to be hit by 'random' corruption.
>>>
>>> The locking in seq_read() seems rather straight forward.
>>
>> I was able to reproduce. The dump shows a mutex that has an owner field, which
>> does not exists as a task so this all looks fishy. The good thing is, that I
>> can reproduce the issue within some hours. (exact same backtrace). Will add some
>> more debug data to get a handle where we come from.
> 
> Did the owner field show to something that still looks like a task_struct?

No, its not a task_struct. Activating some more debug information did indeed 
revealed several other issues (overwritten redzones etc). Unfortunately I 
only saw the broken things after the facts, so I do not know which code did that.
When I disabled the cgroup controllers in libvirt I was no longer able to trigger
the bugs. Still trying to narrow things down.

Christian

[toc] | [prev] | [next] | [standalone]

#1312468

Hello,

On Tue, Jan 19, 2016 at 08:36:18PM +0100, Christian Borntraeger wrote:
> No, its not a task_struct. Activating some more debug information did indeed 
> revealed several other issues (overwritten redzones etc). Unfortunately I 
> only saw the broken things after the facts, so I do not know which code did that.
> When I disabled the cgroup controllers in libvirt I was no longer able to trigger
> the bugs. Still trying to narrow things down.

Hmmm... that's worrying.  CONFIG_DEBUG_PAGEALLOC sometimes can catch
these sort of bugs red-handed.  Might worth trying.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1312940

On Tue, Jan 19, 2016 at 02:38:45PM -0500, Tejun Heo wrote:
> Hello,
> 
> On Tue, Jan 19, 2016 at 08:36:18PM +0100, Christian Borntraeger wrote:
> > No, its not a task_struct. Activating some more debug information did indeed 
> > revealed several other issues (overwritten redzones etc). Unfortunately I 
> > only saw the broken things after the facts, so I do not know which code did that.
> > When I disabled the cgroup controllers in libvirt I was no longer able to trigger
> > the bugs. Still trying to narrow things down.
> 
> Hmmm... that's worrying.  CONFIG_DEBUG_PAGEALLOC sometimes can catch
> these sort of bugs red-handed.  Might worth trying.

Christian, just to avoid that you get surprised like I did:
CONFIG_DEBUG_PAGEALLOC requires in the meantime an additional kernel
parameter "debug_pagealloc=on" to be active.

That change was introduced a year ago, so it was probably only me who
wasn't aware of that change :)

[toc] | [prev] | [next] | [standalone]

#1313056

On 01/20/2016 08:07 AM, Heiko Carstens wrote:
> On Tue, Jan 19, 2016 at 02:38:45PM -0500, Tejun Heo wrote:
>> Hello,
>>
>> On Tue, Jan 19, 2016 at 08:36:18PM +0100, Christian Borntraeger wrote:
>>> No, its not a task_struct. Activating some more debug information did indeed 
>>> revealed several other issues (overwritten redzones etc). Unfortunately I 
>>> only saw the broken things after the facts, so I do not know which code did that.
>>> When I disabled the cgroup controllers in libvirt I was no longer able to trigger
>>> the bugs. Still trying to narrow things down.
>>
>> Hmmm... that's worrying.  CONFIG_DEBUG_PAGEALLOC sometimes can catch
>> these sort of bugs red-handed.  Might worth trying.
> 
> Christian, just to avoid that you get surprised like I did:
> CONFIG_DEBUG_PAGEALLOC requires in the meantime an additional kernel
> parameter "debug_pagealloc=on" to be active.
> 
> That change was introduced a year ago, so it was probably only me who
> wasn't aware of that change :)

I had CONFIG_DEBUG_PAGEALLOC, but not the command line. :-(

With that enabled I now have:

[  561.043895] Unable to handle kernel pointer dereference in virtual kernel address space
[  561.043902] failing address: 000000fa14b30000 TEID: 000000fa14b30803
[  561.043905] Fault in home space mode while using kernel ASCE.
[  561.043911] AS:0000000000fa5007 R3:000000ff627ff007 S:000000ff62759800 P:000000fa14b30400 
[  561.043953] Oops: 0011 ilc:3 [#1] SMP DEBUG_PAGEALLOC
[  561.043964] Modules linked in: nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ipt_REJECT nf_reject_ipv4 xt_tcpudp iptable_filter ip_tables x_tables bridge stp llc btrfs xor raid6_pq ghash_s390 prng ecb aes_s390 des_s390 des_generic sha512_s390 sha256_s390 sha1_s390 sha_common eadm_sch nfsd auth_rpcgss vhost_net tun oid_registry nfs_acl lockd vhost macvtap macvlan grace sunrpc dm_service_time dm_multipath dm_mod autofs4
[  561.044057] CPU: 52 PID: 215 Comm: ksoftirqd/52 Not tainted 4.4.0+ #94
[  561.044062] task: 000000fa5bc48000 ti: 000000fa5bc50000 task.ti: 000000fa5bc50000
[  561.044066] Krnl PSW : 0704e00180000000 00000000001aa1ee (remove_entity_load_avg+0x1e/0x1b8)
[  561.044080]            R:0 T:1 IO:1 EX:1 Key:0 M:1 W:0 P:0 AS:3 CC:2 PM:0 EA:3
Krnl GPRS: 0000000000000000 000000fa0933b3d8 000000fa0b411860 000000fa14b30000
[  561.044087]            00000000001ad750 0000000000000001 0000000000000000 000000000000000a
[  561.044093]            0000000000d28b0c 0000000000c4ba28 0000000000000028 0000000000000140
[  561.044095]            000000fa389f0348 000000000084cfb0 00000000001ad774 000000fa5bc53b88
[  561.044105] Krnl Code: 00000000001aa1dc: c0d0003516ea	larl	%r13,84cfb0
           00000000001aa1e2: e33020780004	lg	%r3,120(%r2)
          #00000000001aa1e8: e30020880004	lg	%r0,136(%r2)
          >00000000001aa1ee: e34030580004	lg	%r4,88(%r3)
           00000000001aa1f4: b9e90014		sgrk	%r1,%r4,%r0
           00000000001aa1f8: ec140095007c	cgij	%r1,0,4,1aa322
           00000000001aa1fe: eb11000a000c	srlg	%r1,%r1,10
           00000000001aa204: ec160013007c	cgij	%r1,0,6,1aa22a
[  561.044170] Call Trace:
[  561.044176] ([<00000000001ad750>] free_fair_sched_group+0x80/0xf8)
[  561.044181]  [<0000000000192656>] free_sched_group+0x2e/0x58
[  561.044187]  [<00000000001ded82>] rcu_process_callbacks+0x3fa/0x928
[  561.044194]  [<00000000001676a4>] __do_softirq+0xd4/0x4b0
[  561.044199]  [<0000000000167abe>] run_ksoftirqd+0x3e/0xa8
[  561.044204]  [<000000000018d5bc>] smpboot_thread_fn+0x16c/0x2a0
[  561.044210]  [<0000000000188704>] kthread+0x10c/0x128
[  561.044216]  [<000000000083d8a2>] kernel_thread_starter+0x6/0xc
[  561.044220]  [<000000000083d89c>] kernel_thread_starter+0x0/0xc
[  561.044223] INFO: lockdep is turned off.
[  561.044225] Last Breaking-Event-Address:
[  561.044230]  [<00000000001ad76e>] free_fair_sched_group+0x9e/0xf8
[  561.044237]  
[  561.044241] Kernel panic - not syncing: Fatal exception in interrupt


Will look into that and see if fixing this makes the problem go away.
(unless somebody else has a quick idea)

Christian

[toc] | [prev] | [next] | [standalone]

#1313092

On Wed, Jan 20, 2016 at 11:15:05AM +0100, Christian Borntraeger wrote:
> [  561.044066] Krnl PSW : 0704e00180000000 00000000001aa1ee (remove_entity_load_avg+0x1e/0x1b8)

> [  561.044176] ([<00000000001ad750>] free_fair_sched_group+0x80/0xf8)
> [  561.044181]  [<0000000000192656>] free_sched_group+0x2e/0x58
> [  561.044187]  [<00000000001ded82>] rcu_process_callbacks+0x3fa/0x928

Urgh,.. lemme stare at that.

[toc] | [prev] | [next] | [standalone]

#1313104

On Wed, Jan 20, 2016 at 11:30:36AM +0100, Peter Zijlstra wrote:
> On Wed, Jan 20, 2016 at 11:15:05AM +0100, Christian Borntraeger wrote:
> > [  561.044066] Krnl PSW : 0704e00180000000 00000000001aa1ee (remove_entity_load_avg+0x1e/0x1b8)
> 
> > [  561.044176] ([<00000000001ad750>] free_fair_sched_group+0x80/0xf8)
> > [  561.044181]  [<0000000000192656>] free_sched_group+0x2e/0x58
> > [  561.044187]  [<00000000001ded82>] rcu_process_callbacks+0x3fa/0x928
> 
> Urgh,.. lemme stare at that.

TJ, is css_offline guaranteed to be called in hierarchical order? I
got properly lost in the whole cgroup destroy code. There's endless
workqueues and rcu callbacks there.

So the current place in free_fair_sched_group() is far too late to be
calling remove_entity_load_avg(). But I'm not sure where I should put
it, it needs to be in a place where we know the group is going to die
but its parent is guaranteed to still exist.

Would offline be that place?

[toc] | [prev] | [next] | [standalone]

#1313308

Hello,

On Wed, Jan 20, 2016 at 11:47:58AM +0100, Peter Zijlstra wrote:
> TJ, is css_offline guaranteed to be called in hierarchical order? I

No, they aren't.  The ancestors of a css are guaranteed to stay around
until css_free is called on the css and that's the only ordering
guarantee.

> got properly lost in the whole cgroup destroy code. There's endless
> workqueues and rcu callbacks there.

Yeah, it's hairy.  I wondered about adding support for bouncing to
workqueue in both percpu_ref and rcu which would make things easier to
follow.  Not sure how often this pattern happens tho.

> So the current place in free_fair_sched_group() is far too late to be
> calling remove_entity_load_avg(). But I'm not sure where I should put
> it, it needs to be in a place where we know the group is going to die
> but its parent is guaranteed to still exist.
> 
> Would offline be that place?

Hmmm... css_free would be with the following patch.

diff -u b/kernel/cgroup.c work/kernel/cgroup.c
--- b/kernel/cgroup.c
+++ work/kernel/cgroup.c
@@ -4725,14 +4725,14 @@
 
 	if (ss) {
 		/* css free path */
+		struct cgroup_subsys_state *parent = css->parent;
 		int id = css->id;
 
-		if (css->parent)
-			css_put(css->parent);
-
 		ss->css_free(css);
 		cgroup_idr_remove(&ss->css_idr, id);
 		cgroup_put(cgrp);
+		if (parent)
+			css_put(parent);
 	} else {
 		/* cgroup free path */
 		atomic_dec(&cgrp->root->nr_cgrps);


Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1313325

On Wed, Jan 20, 2016 at 10:30:07AM -0500, Tejun Heo wrote:
> > So the current place in free_fair_sched_group() is far too late to be
> > calling remove_entity_load_avg(). But I'm not sure where I should put
> > it, it needs to be in a place where we know the group is going to die
> > but its parent is guaranteed to still exist.
> > 
> > Would offline be that place?
> 
> Hmmm... css_free would be with the following patch.

I thought a bit more about this and I think the right thing to do here
is making both css_offline and css_free follow the ancestry order.
I'll post a patch to do that soon.  offline is called at the head of
destruction when the css is made invisble and draining of existing
refs starts.  free at the end of that process.  Tree ordering
shouldn't be where the two differ.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1313364

On Wed, Jan 20, 2016 at 11:04:35AM -0500, Tejun Heo wrote:
> On Wed, Jan 20, 2016 at 10:30:07AM -0500, Tejun Heo wrote:
> > > So the current place in free_fair_sched_group() is far too late to be
> > > calling remove_entity_load_avg(). But I'm not sure where I should put
> > > it, it needs to be in a place where we know the group is going to die
> > > but its parent is guaranteed to still exist.
> > > 
> > > Would offline be that place?
> > 
> > Hmmm... css_free would be with the following patch.
> 
> I thought a bit more about this and I think the right thing to do here
> is making both css_offline and css_free follow the ancestry order.
> I'll post a patch to do that soon.  offline is called at the head of
> destruction when the css is made invisble and draining of existing
> refs starts.  free at the end of that process.  Tree ordering
> shouldn't be where the two differ.

OK, that would be good. Meanwhile the above seems to suggest that
css_offline is already hierarchical?

I get the feeling the way sched uses the css_{offline,release,free} is
sub-optimal. cpu_cgrp_subsys::css_free := sched_destroy_group() does a
call_rcu, whereas if I read the comment with css_free_work_fn()
correctly, this is already after a grace-period, so yet another doesn't
make sense.

[toc] | [prev] | [next] | [standalone]

#1313372

Hello, Peter.

On Wed, Jan 20, 2016 at 05:49:32PM +0100, Peter Zijlstra wrote:
> > I thought a bit more about this and I think the right thing to do here
> > is making both css_offline and css_free follow the ancestry order.
> > I'll post a patch to do that soon.  offline is called at the head of
> > destruction when the css is made invisble and draining of existing
> > refs starts.  free at the end of that process.  Tree ordering
> > shouldn't be where the two differ.
> 
> OK, that would be good. Meanwhile the above seems to suggest that
> css_offline is already hierarchical?

No, I was thinking just fixing css_free and leaving css_offline
unordered as the latter is more involved.  Will fix both soon.

> I get the feeling the way sched uses the css_{offline,release,free} is
> sub-optimal. cpu_cgrp_subsys::css_free := sched_destroy_group() does a
> call_rcu, whereas if I read the comment with css_free_work_fn()
> correctly, this is already after a grace-period, so yet another doesn't
> make sense.

Here are what the three callbacks do

 css_offline

	The css is no longer visible to userland and it's guaranteed
	that all future css_tryget_online() will fail.

 css_released

	The reference count hit zero and css_free will be called on
	the css after a RCU grace period.

 css_free

	A RCU grace period has passed after css's last ref is put.
	The css can be freed now.

So, as long as sched adheres to css refcnting, there's no need to do
another RCUing off of css_free.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1315485

On Wed, Jan 20, 2016 at 10:30:07AM -0500, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jan 20, 2016 at 11:47:58AM +0100, Peter Zijlstra wrote:
> > TJ, is css_offline guaranteed to be called in hierarchical order? I
> 
> No, they aren't.  The ancestors of a css are guaranteed to stay around
> until css_free is called on the css and that's the only ordering
> guarantee.
> 
> > got properly lost in the whole cgroup destroy code. There's endless
> > workqueues and rcu callbacks there.
> 
> Yeah, it's hairy.  I wondered about adding support for bouncing to
> workqueue in both percpu_ref and rcu which would make things easier to
> follow.  Not sure how often this pattern happens tho.

This came up recently offlist for call_rcu(), so that a call to (say)
call_rcu_schedule_work() would do a schedule_work() after a grace period
elapsed, invoking the function passed in to call_rcu_schedule_work().
There are several existing cases that do this, so special-casing it seems
worthwhile.  Perhaps something vaguely similar would work for percpu_ref.

							Thanx, Paul

[toc] | [prev] | [next] | [standalone]

#1316293

On Fri, Jan 22, 2016 at 06:03:13PM -0800, Paul E. McKenney wrote:
> > Yeah, it's hairy.  I wondered about adding support for bouncing to
> > workqueue in both percpu_ref and rcu which would make things easier to
> > follow.  Not sure how often this pattern happens tho.
> 
> This came up recently offlist for call_rcu(), so that a call to (say)
> call_rcu_schedule_work() would do a schedule_work() after a grace period
> elapsed, invoking the function passed in to call_rcu_schedule_work().
> There are several existing cases that do this, so special-casing it seems
> worthwhile.  Perhaps something vaguely similar would work for percpu_ref.

FYI, my use case was also related to percpu-ref.  The percpu ref API
is unfortunately really hard to use and will almost always involve
a work queue due to the complex interaction between percpu_ref_kill
and percpu_ref_exit.  One thing that would help a lot of callers would
be a percpu_ref_exit_sync that kills the ref and waits for all references
to go away synchronously.

[toc] | [prev] | [next] | [standalone]

#1317235

Hello, Christoph.

On Mon, Jan 25, 2016 at 09:49:42AM +0100, Christoph Hellwig wrote:
> FYI, my use case was also related to percpu-ref.  The percpu ref API
> is unfortunately really hard to use and will almost always involve
> a work queue due to the complex interaction between percpu_ref_kill
> and percpu_ref_exit.  One thing that would help a lot of callers would

That's interesting.  Can you please elaborate on how kill and exit
interact to make things complex?

> be a percpu_ref_exit_sync that kills the ref and waits for all references
> to go away synchronously.

That shouldn't be difficult to implement.  One minor concern is that
it's almost guaranteed that there will be cases where the
synchronicity is exposed to userland.  Anyways, can you please
describe the use case?

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1318015

On Mon, Jan 25, 2016 at 02:38:36PM -0500, Tejun Heo wrote:
> On Mon, Jan 25, 2016 at 09:49:42AM +0100, Christoph Hellwig wrote:
> > FYI, my use case was also related to percpu-ref.  The percpu ref API
> > is unfortunately really hard to use and will almost always involve
> > a work queue due to the complex interaction between percpu_ref_kill
> > and percpu_ref_exit.  One thing that would help a lot of callers would
> 
> That's interesting.  Can you please elaborate on how kill and exit
> interact to make things complex?

That we need to first call kill to tear down the reference, then we get
a release callback which is in the calling context of the last
percpu_ref_put, but will need to call percpu_ref_exit from process context
again.  This means if any percpu_ref_put is from non-process context
we will always need a work_struct or similar to schedule the final
percpu_ref_exit.  Except when..

> > be a percpu_ref_exit_sync that kills the ref and waits for all references
> > to go away synchronously.
> 
> That shouldn't be difficult to implement.  One minor concern is that
> it's almost guaranteed that there will be cases where the
> synchronicity is exposed to userland.  Anyways, can you please
> describe the use case?

We use this completion scheme where the percpu_ref_exit is done from
the same context as the percpu_ref_kill which previously waits for
the last reference drop.  But for these cases exposing the synchronicity
to the caller (including userland) actually is intentional.

My use case is a new storage target, broadly similar to the SCSI target,
which happens to exhibit the same behavior.  In that case we only want
to return from the teardown function when all I/O on a 'queue' of sorts
has finished, for example during module removal.

[toc] | [prev] | [next] | [standalone]

#1318053

Hello, Christoph.

On Tue, Jan 26, 2016 at 03:51:57PM +0100, Christoph Hellwig wrote:
> > That's interesting.  Can you please elaborate on how kill and exit
> > interact to make things complex?
> 
> That we need to first call kill to tear down the reference, then we get
> a release callback which is in the calling context of the last
> percpu_ref_put, but will need to call percpu_ref_exit from process context
> again.  This means if any percpu_ref_put is from non-process context

Hmmm... why do you need to call percpu_ref_exit() from process
context?  All it does is freeing the percpu counter and resetting the
state, both of which can be done from any context.

> we will always need a work_struct or similar to schedule the final
> percpu_ref_exit.  Except when..

I don't think that's true.

> > > be a percpu_ref_exit_sync that kills the ref and waits for all references
> > > to go away synchronously.
> > 
> > That shouldn't be difficult to implement.  One minor concern is that
> > it's almost guaranteed that there will be cases where the
> > synchronicity is exposed to userland.  Anyways, can you please
> > describe the use case?
> 
> We use this completion scheme where the percpu_ref_exit is done from
> the same context as the percpu_ref_kill which previously waits for
> the last reference drop.  But for these cases exposing the synchronicity
> to the caller (including userland) actually is intentional.
> 
> My use case is a new storage target, broadly similar to the SCSI target,
> which happens to exhibit the same behavior.  In that case we only want
> to return from the teardown function when all I/O on a 'queue' of sorts
> has finished, for example during module removal.

It'd most likely end up doing synchronous destruction in a loop with
each iteration involving a full RCU grace period.  If there can be a
lot of devices, it can add up to a substantial amount of time.  Maybe
it's okay here but I've already been bitten several times by the exact
same issue.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1318135

On Tue, Jan 26, 2016 at 10:28:46AM -0500, Tejun Heo wrote:
> Hmmm... why do you need to call percpu_ref_exit() from process
> context?  All it does is freeing the percpu counter and resetting the
> state, both of which can be done from any context.

I checked and that's true indeed.  You cought me doing cargo cult
programming as the callers I looked at already do this.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web