Messages in this thread | | | Subject | Re: [RFC/RFT PATCH] sched: automated per tty task groups | From | Mike Galbraith <> | Date | Thu, 21 Oct 2010 21:11:34 +0200 |
| |
On Thu, 2010-10-21 at 18:29 +0200, Oleg Nesterov wrote: > On 10/21, Peter Zijlstra wrote: > > > > On Thu, 2010-10-21 at 06:51 -0400, Mathieu Desnoyers wrote: > > > * Mike Galbraith (efault@gmx.de) wrote: > > > [...] > > > > +static void > > > > +autogroup_attach_tty(struct task_struct *p, struct task_group **tg) > > > > +{ > > > > + struct tty_struct *tty = p->signal->tty; > > > > + > > > > + if (!tty) > > > > + return; > > > > + > > > > + *tg = p->signal->tty->tg; > > > > +} > > minor nit, I think in theory this needs barrier(), or > > struct tty_struct *tty = ACCESS_ONCE(p->signal->tty); > > if (tty) > *tg = tty->tg;
Thanks.
> > > > +static inline void > > > > +autogroup_check_attach(struct task_struct *p, struct task_group **tg) > > > > +{ > > > > + if (!sysctl_sched_autogroup_enabled || *tg != &root_task_group || > > > > + p->sched_class != &fair_sched_class) > > > > + return; > > > > + > > > > + rcu_read_lock(); > > > > + > > > > + autogroup_attach_tty(p, tg); > > > > + > > > > + rcu_read_unlock(); > > > > +} > > > > + > > > > > Meanwhile, a little question about locking here: how is > > > the read lock supposed to protect from p->signal (and p->signal->tty) > > > modifications ? What's the locking scheme here ? So maybe just simple > > > rcu_dereference are missing, or maybe the tsk->sighand->siglock might be > > > required. In all cases, I feel something is missing there. > > > > Oleg, could you comment? > > No, I don't understand this ;) But I know nothig about task groups, > most probably this is OK. > > It is not clear to me why do we need rcu_read_lock() and how it can help. > The tty can go away right after dereferencing signal->tty.
It was inherited.
> Even if the task doesn't exit, it (or its sub-thread) can do sys_setsid() > at any moment and free this tty. If any thread was moved to tty->sg, doesn't > this mean that, say, ->cfs_rq will point to the already freed tg->cfs_rq?
Ah, so isn't as safe as it looked. Thanks!
> >From http://marc.info/?l=linux-kernel&m=128764874422614 > > +int sched_autogroup_handler(struct ctl_table *table, int write, > + void __user *buffer, size_t *lenp, loff_t *ppos) > +{ > + struct task_struct *p, *t; > + struct task_group *tg; > + int ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos); > + > + if (ret || !write) > + return ret; > + > + for_each_process(p) { > > Hmm. This needs rcu lock at least?
(used to be paranoid locking there.. vs required locking)
> + tg = task_group(p); > > Why?
A cleanup leftover.
> > + sched_move_task(p); > + list_for_each_entry_rcu(t, &p->thread_group, thread_group) { > + sched_move_task(t); > + } > + } > > Looks like, you can just do > > do_each_thread(p, t) { > sched_move_task(t); > } while_each_thread(p, t); > > With the same effect.
Yeah.
So in theory, the tty can go away on me. I knew this was too easy.
-Mike
| |