From: Mike Galbraith <efault@gmx.de>
To: Oleg Nesterov <oleg@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
Peter Zijlstra <a.p.zijlstra@chello.nl>,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
Ingo Molnar <mingo@elte.hu>, LKML <linux-kernel@vger.kernel.org>,
Markus Trippelsdorf <markus@trippelsdorf.de>
Subject: Re: [RFC/RFT PATCH v3] sched: automated per tty task groups
Date: Sat, 13 Nov 2010 04:42:04 -0700 [thread overview]
Message-ID: <1289648524.22764.149.camel@maggy.simson.net> (raw)
In-Reply-To: <20101112181240.GB8659@redhat.com>
On Fri, 2010-11-12 at 19:12 +0100, Oleg Nesterov wrote:
> On 11/11, Mike Galbraith wrote:
> >
> > On Thu, 2010-11-11 at 21:27 +0100, Oleg Nesterov wrote:
> >
> > > But the real problem is that copy_process() can fail after that,
> > > and in this case we have the unbalanced kref_get().
> >
> > Memory leak, will fix.
> >
> > > > +++ linux-2.6.36.git/kernel/exit.c
> > > > @@ -174,6 +174,7 @@ repeat:
> > > > write_lock_irq(&tasklist_lock);
> > > > tracehook_finish_release_task(p);
> > > > __exit_signal(p);
> > > > + sched_autogroup_exit(p);
> > >
> > > This doesn't look right. Note that "p" can run/sleep after that
> > > (or in parallel), set_task_rq() can use the freed ->autogroup.
> >
> > So avoiding refcounting rcu released task_group backfired. Crud.
>
> Just in case, the lock order may be wrong. sched_autogroup_exit()
> takes task_group_lock under write_lock(tasklist), while
> sched_autogroup_handler() takes them in reverse order.
Bug self destructs when global classifier goes away.
> I am not sure, but perhaps this can be simpler?
> wake_up_new_task() does autogroup_fork(), and do_exit() does
> sched_autogroup_exit() before the last schedule. Possible?
That's what I was going to do. That said, I couldn't have had the
problem if I'd tied final put directly to life of container, and am
thinking I should do that instead when I go back to p->signal.
> Very basic question. Currently sched_autogroup_create_attach()
> has the only caller, __proc_set_tty(). It is a bit strange that
> signal->tty change is process-wide, but sched_autogroup_create_attach()
> move the single thread, the caller. What about other threads in
> this thread group? The same for proc_clear_tty().
Yeah, I really should (will) move all on the spot, though it doesn't
seem to matter in general practice, forks afterward land in the right
bucket. With per tty or p->signal, migration will pick up stragglers
lazily.. unless they're pinned.
> > +void sched_autogroup_create_attach(struct task_struct *p)
> > +{
> > + autogroup_move_task(p, autogroup_create());
> > +
> > + /*
> > + * Correct freshly allocated group's refcount.
> > + * Move takes a reference on destination, but
> > + * create already initialized refcount to 1.
> > + */
> > + if (p->autogroup != &autogroup_default)
> > + autogroup_kref_put(p->autogroup);
> > +}
>
> OK, but I don't understand "p->autogroup != &autogroup_default"
> check. This is true if autogroup_create() succeeds. Otherwise
> autogroup_create() does autogroup_kref_get(autogroup_default),
> doesn't this mean we need unconditional _put ?
D'oh, target fixation :) Thanks.
> And can't resist, minor cosmetic nit,
>
> > static inline struct task_group *task_group(struct task_struct *p)
> > {
> > + struct task_group *tg;
> > struct cgroup_subsys_state *css;
> >
> > css = task_subsys_state_check(p, cpu_cgroup_subsys_id,
> > lockdep_is_held(&task_rq(p)->lock));
> > - return container_of(css, struct task_group, css);
> > + tg = container_of(css, struct task_group, css);
> > +
> > + autogroup_task_group(p, &tg);
>
> Fell free to ignore, but imho
>
> return autogroup_task_group(p, tg);
>
> looks a bit better. Why autogroup_task_group() returns its
> result via pointer?
No particularly good reason, I'll do the cosmetic change.
Thanks,
-Mike
next prev parent reply other threads:[~2010-11-13 11:42 UTC|newest]
Thread overview: 264+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-19 9:16 [RFC/RFT PATCH] sched: automated per tty task groups Mike Galbraith
2010-10-19 9:26 ` Peter Zijlstra
2010-10-19 9:39 ` Mike Galbraith
2010-10-19 9:43 ` Peter Zijlstra
2010-10-19 9:46 ` Mike Galbraith
2010-10-21 7:55 ` Mike Galbraith
2010-10-21 10:28 ` Peter Zijlstra
2010-10-19 9:29 ` Peter Zijlstra
2010-10-19 9:42 ` Mike Galbraith
2010-10-19 11:29 ` Mike Galbraith
2010-10-19 11:56 ` Ingo Molnar
2010-10-19 13:12 ` Mike Galbraith
2010-10-19 15:28 ` Linus Torvalds
2010-10-19 18:13 ` Mike Galbraith
2010-10-19 18:53 ` Mike Galbraith
2010-10-20 2:56 ` Ingo Molnar
2010-10-21 8:11 ` Mike Galbraith
2010-10-21 8:31 ` Ingo Molnar
2010-10-21 8:39 ` Mike Galbraith
2010-10-21 8:48 ` Markus Trippelsdorf
2010-10-21 8:52 ` Mike Galbraith
[not found] ` <20101021115723.GA1587@arch.trippelsdorf.de>
2010-10-21 16:22 ` Mathieu Desnoyers
2010-10-21 10:51 ` Mathieu Desnoyers
2010-10-21 11:25 ` Peter Zijlstra
2010-10-21 16:29 ` Oleg Nesterov
2010-10-21 19:11 ` Mike Galbraith
2010-10-26 7:07 ` [RFC/RFT PATCH v3] " Mike Galbraith
2010-10-26 7:29 ` Mike Galbraith
2010-10-26 15:47 ` Linus Torvalds
2010-10-27 1:58 ` Mike Galbraith
2010-11-11 15:26 ` Mike Galbraith
2010-11-11 18:04 ` Ingo Molnar
2010-11-11 18:34 ` Linus Torvalds
2010-11-11 19:08 ` Mike Galbraith
2010-11-11 19:37 ` Linus Torvalds
2010-11-11 20:29 ` Oleg Nesterov
2010-11-11 19:15 ` Markus Trippelsdorf
2010-11-11 19:35 ` Mike Galbraith
2010-11-11 19:38 ` Markus Trippelsdorf
2010-11-11 19:58 ` Mike Galbraith
2010-11-11 20:27 ` Oleg Nesterov
2010-11-11 22:20 ` Mike Galbraith
2010-11-12 18:12 ` Oleg Nesterov
2010-11-13 11:42 ` Mike Galbraith [this message]
2010-11-14 17:19 ` Mike Galbraith
2010-11-14 17:49 ` Markus Trippelsdorf
2010-11-14 18:10 ` Mike Galbraith
2010-11-14 19:28 ` Linus Torvalds
2010-11-14 20:20 ` Linus Torvalds
2010-11-14 20:27 ` Markus Trippelsdorf
2010-11-14 20:48 ` Linus Torvalds
2010-11-14 23:43 ` Mike Galbraith
2010-11-15 0:15 ` Linus Torvalds
2010-11-15 0:26 ` Linus Torvalds
2010-11-15 1:13 ` Mike Galbraith
2010-11-15 3:12 ` Linus Torvalds
2010-11-15 14:00 ` Mike Galbraith
2010-11-15 8:57 ` Peter Zijlstra
2010-11-15 11:32 ` Mike Galbraith
2010-11-15 11:46 ` Mike Galbraith
2010-11-15 12:57 ` Oleg Nesterov
2010-11-15 21:25 ` Mike Galbraith
2010-11-15 22:48 ` Peter Zijlstra
2010-11-16 1:56 ` Vivek Goyal
2010-11-16 2:18 ` Linus Torvalds
2010-11-17 8:06 ` Balbir Singh
2010-11-16 14:02 ` Mike Galbraith
2010-11-16 14:11 ` Peter Zijlstra
2010-11-16 14:47 ` Dhaval Giani
2010-11-16 17:03 ` Lennart Poettering
2010-11-16 17:11 ` Linus Torvalds
2010-11-16 18:16 ` Lennart Poettering
2010-11-16 18:21 ` Peter Zijlstra
2010-11-16 18:33 ` Paul Menage
2010-11-16 18:55 ` david
2010-11-16 18:59 ` Peter Zijlstra
2010-11-16 19:09 ` Vivek Goyal
2010-11-16 19:13 ` Peter Zijlstra
2010-11-16 19:22 ` Vivek Goyal
2010-11-16 19:25 ` Peter Zijlstra
2010-11-16 19:40 ` Vivek Goyal
2010-11-16 19:43 ` Peter Zijlstra
2010-11-16 19:49 ` Linus Torvalds
2010-11-16 19:35 ` Linus Torvalds
2010-11-16 20:03 ` Lennart Poettering
2010-11-16 20:12 ` Peter Zijlstra
2010-11-16 18:49 ` Linus Torvalds
2010-11-16 19:03 ` Pekka Enberg
2010-11-16 20:21 ` Kay Sievers
2010-11-16 20:35 ` Linus Torvalds
2010-11-16 20:31 ` Lennart Poettering
2010-11-17 13:21 ` Stephen Clark
2010-11-16 19:08 ` david
2010-11-16 20:33 ` Lennart Poettering
2010-11-16 20:38 ` Linus Torvalds
2010-11-16 21:14 ` Lennart Poettering
2010-11-17 13:23 ` Stephen Clark
2010-11-18 22:33 ` Hans-Peter Jansen
2010-11-18 23:12 ` Samuel Thibault
2010-11-18 23:35 ` Mike Galbraith
2010-11-18 23:43 ` Samuel Thibault
2010-11-18 23:51 ` Linus Torvalds
2010-11-19 0:02 ` Samuel Thibault
2010-11-19 0:07 ` Samuel Thibault
2010-11-19 11:57 ` Peter Zijlstra
2010-11-19 14:24 ` Samuel Thibault
2010-11-19 14:43 ` Peter Zijlstra
2010-11-19 14:55 ` Samuel Thibault
2010-11-19 0:42 ` Linus Torvalds
2010-11-19 0:59 ` Samuel Thibault
2010-11-19 1:11 ` Linus Torvalds
2010-11-19 1:12 ` Mike Galbraith
2010-11-19 1:23 ` Samuel Thibault
2010-11-19 2:28 ` Mike Galbraith
2010-11-19 9:02 ` Samuel Thibault
2010-11-19 11:49 ` Peter Zijlstra
2010-11-19 12:19 ` Peter Zijlstra
2010-11-19 12:55 ` Mathieu Desnoyers
2010-11-19 13:00 ` Peter Zijlstra
2010-11-19 13:20 ` Mathieu Desnoyers
2010-11-19 12:31 ` Paul Menage
2010-11-19 12:51 ` Peter Zijlstra
2010-11-19 13:03 ` Mike Galbraith
2010-11-19 12:38 ` Mike Galbraith
2010-11-22 6:22 ` Balbir Singh
2010-11-18 23:29 ` Mike Galbraith
2010-11-16 20:44 ` Pekka Enberg
2010-11-16 19:27 ` Dhaval Giani
2010-11-16 19:42 ` Diego Calleja
2010-11-16 19:45 ` Linus Torvalds
2010-11-16 19:56 ` Paul Menage
2010-11-16 20:17 ` Vivek Goyal
2010-11-16 20:50 ` Lennart Poettering
2010-11-20 22:16 ` Mika Laitio
2010-11-21 0:19 ` Mike Galbraith
2010-11-16 20:28 ` Lennart Poettering
2010-11-16 20:46 ` David Miller
2010-11-16 21:08 ` Lennart Poettering
2010-11-16 21:14 ` David Miller
2010-11-16 20:52 ` Alan Cox
2010-11-16 21:08 ` Linus Torvalds
2010-11-16 21:19 ` Lennart Poettering
2010-11-16 23:39 ` Ted Ts'o
2010-11-17 0:21 ` Lennart Poettering
2010-11-17 2:06 ` Ted Ts'o
2010-11-17 14:57 ` Vivek Goyal
2010-11-17 15:01 ` Lennart Poettering
2010-11-17 17:16 ` John Stoffel
2010-11-19 5:20 ` Andev
2010-11-19 11:59 ` Peter Zijlstra
2010-11-19 13:03 ` Ben Gamari
2010-11-19 13:07 ` Theodore Tso
2010-11-19 16:29 ` David Miller
2010-11-19 16:34 ` Lennart Poettering
2010-11-19 16:43 ` David Miller
2010-11-19 17:51 ` Linus Torvalds
2010-11-19 19:12 ` Ben Gamari
2010-11-19 19:48 ` Linus Torvalds
2010-11-20 1:33 ` Lennart Poettering
2010-11-19 20:38 ` Paul Menage
2010-11-20 1:13 ` Lennart Poettering
2010-11-20 4:25 ` Balbir Singh
2010-11-20 15:41 ` Lennart Poettering
2010-11-22 6:24 ` Balbir Singh
2010-11-22 19:21 ` Lennart Poettering
2010-11-19 19:31 ` Mike Galbraith
2010-11-19 13:21 ` Peter Zijlstra
2010-11-17 22:34 ` Lennart Poettering
2010-11-17 22:37 ` Peter Zijlstra
2010-11-17 22:45 ` Lennart Poettering
2010-11-17 22:52 ` Peter Zijlstra
2010-11-18 15:00 ` Stephen Clark
2010-11-17 23:49 ` Lennart Poettering
2010-11-16 21:17 ` Lennart Poettering
2010-11-17 20:59 ` James Cloos
2010-11-22 6:16 ` Balbir Singh
2010-11-16 18:57 ` Stephen Clark
2010-11-16 19:12 ` Vivek Goyal
2010-11-16 19:57 ` Mike Galbraith
2010-11-16 20:36 ` Lennart Poettering
2010-11-16 19:42 ` Markus Trippelsdorf
2010-11-16 18:08 ` Peter Zijlstra
2010-11-16 18:56 ` Stephen Clark
2010-11-16 20:05 ` Lennart Poettering
2010-11-16 20:15 ` Peter Zijlstra
2010-11-19 0:35 ` H. Peter Anvin
2010-11-19 0:42 ` Samuel Thibault
2010-11-19 3:15 ` Mathieu Desnoyers
[not found] ` <20101120090955.GB12043@balbir.in.ibm.com>
2010-11-20 19:47 ` Mike Galbraith
2010-11-16 13:04 ` Oleg Nesterov
2010-11-16 14:18 ` Mike Galbraith
2010-11-16 15:03 ` Oleg Nesterov
2010-11-16 15:41 ` Mike Galbraith
2010-11-16 17:28 ` Ingo Molnar
2010-11-16 17:42 ` Mike Galbraith
2010-11-20 19:35 ` [PATCH v4] sched: automated per session " Mike Galbraith
2010-11-30 15:39 ` [tip:sched/core] sched: Add 'autogroup' scheduling feature: " tip-bot for Mike Galbraith
2010-12-15 17:50 ` Oleg Nesterov
2010-12-16 7:53 ` Mike Galbraith
2010-12-16 14:09 ` Mike Galbraith
2010-12-16 15:07 ` Oleg Nesterov
2011-01-04 14:18 ` [tip:sched/core] sched, autogroup: Fix potential access to freed memory tip-bot for Mike Galbraith
2010-12-20 13:08 ` [tip:sched/core] sched: Add 'autogroup' scheduling feature: automated per session task groups Bharata B Rao
2010-12-20 13:19 ` Peter Zijlstra
2010-12-20 15:46 ` Bharata B Rao
2010-12-20 15:53 ` Bharata B Rao
2010-12-21 8:33 ` Peter Zijlstra
2010-12-20 16:39 ` Mike Galbraith
2010-12-21 5:04 ` Bharata B Rao
2010-12-21 5:50 ` Mike Galbraith
2010-12-04 17:39 ` [PATCH v4] sched: " Colin Walters
2010-12-04 18:33 ` Linus Torvalds
2010-12-04 20:01 ` Colin Walters
2010-12-04 22:39 ` Linus Torvalds
2010-12-04 23:43 ` Colin Walters
2010-12-05 0:31 ` Linus Torvalds
2010-12-05 7:47 ` Ray Lee
2010-12-05 19:22 ` Colin Walters
2010-12-05 20:47 ` Linus Torvalds
2010-12-05 22:47 ` Colin Walters
2010-12-05 22:58 ` Jesper Juhl
2010-12-05 23:05 ` Jesper Juhl
2010-12-07 18:51 ` Peter Zijlstra
2010-12-05 10:18 ` Con Kolivas
2010-12-05 11:36 ` Mike Galbraith
2010-12-05 20:58 ` Ingo Molnar
2010-12-04 23:31 ` david
2010-12-05 11:11 ` Nikos Chantziaras
2010-12-05 15:12 ` [PATCH v4] Regression: " Alan Cox
2010-12-05 16:16 ` Florian Mickler
2010-12-05 19:48 ` Alan Cox
2010-12-06 16:03 ` Florian Mickler
2010-12-05 16:59 ` Mike Galbraith
2010-12-05 17:09 ` Mike Galbraith
2010-12-05 17:15 ` Mike Galbraith
2010-12-06 0:28 ` [PATCH v4] " Valdis.Kletnieks
2010-11-16 14:01 ` [RFC/RFT PATCH v3] sched: automated per tty " Peter Zijlstra
2010-11-16 14:19 ` Mike Galbraith
2010-11-17 1:31 ` Kyle McMartin
2010-11-17 1:50 ` Linus Torvalds
2010-11-17 1:56 ` Kyle McMartin
2010-11-17 2:14 ` Mike Galbraith
2010-11-15 0:02 ` Mike Galbraith
2010-11-15 22:41 ` Valdis.Kletnieks
2010-11-15 23:25 ` Linus Torvalds
2010-11-20 19:33 ` Jesper Juhl
2010-11-20 19:51 ` Mike Galbraith
2010-11-20 20:37 ` Jesper Juhl
2010-11-20 22:02 ` Konstantin Svist
2010-11-20 22:15 ` Samuel Thibault
2010-11-20 22:18 ` Thomas Fjellstrom
2010-11-20 20:25 ` Samuel Thibault
2010-11-15 23:46 ` Mike Galbraith
2010-11-15 23:50 ` Linus Torvalds
2010-11-16 0:04 ` Mike Galbraith
2010-11-16 1:18 ` Linus Torvalds
2010-11-16 1:55 ` Paul Menage
2010-11-16 12:58 ` Mike Galbraith
2010-11-16 18:25 ` Paul Menage
2010-11-16 13:59 ` Peter Zijlstra
2010-11-16 14:26 ` Mike Galbraith
2010-10-21 11:27 ` [RFC/RFT PATCH] " Mike Galbraith
2010-10-20 13:55 ` Markus Trippelsdorf
2010-10-20 14:41 ` Mike Galbraith
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1289648524.22764.149.camel@maggy.simson.net \
--to=efault@gmx.de \
--cc=a.p.zijlstra@chello.nl \
--cc=linux-kernel@vger.kernel.org \
--cc=markus@trippelsdorf.de \
--cc=mathieu.desnoyers@efficios.com \
--cc=mingo@elte.hu \
--cc=oleg@redhat.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).