Messages in this thread | | | Date | Mon, 15 Nov 2010 13:57:16 +0100 | From | Oleg Nesterov <> | Subject | Re: [RFC/RFT PATCH v3] sched: automated per tty task groups |
| |
I continue to play the advocatus diaboli ;)
On 11/15, Mike Galbraith wrote: > > +static inline bool > +task_wants_autogroup(struct task_struct *p, struct task_group *tg) > +{ > + if (tg != &root_task_group) > + return false; > + > + if (p->sched_class != &fair_sched_class) > + return false; > + > + if (p->flags & PF_EXITING) > + return false;
Hmm, why? Perhaps PF_EXITING was needed in the previous version to avoid the race with release_task(). But now it is always safe to use signal->autogroup.
And the exiting task can do a lot before it disappears, probably we shouldn't ignore ->autogroup.
> +static void > +autogroup_move_group(struct task_struct *p, struct autogroup *ag) > +{ > + struct autogroup *prev; > + struct task_struct *t; > + struct rq *rq; > + unsigned long flags; > + > + rq = task_rq_lock(p, &flags); > + prev = p->signal->autogroup; > + if (prev == ag) { > + task_rq_unlock(rq, &flags); > + return; > + } > + > + p->signal->autogroup = autogroup_kref_get(ag); > + __sched_move_task(p, rq); > + task_rq_unlock(rq, &flags); > + > + rcu_read_lock(); > + list_for_each_entry_rcu(t, &p->thread_group, thread_group) { > + sched_move_task(t); > + } > + rcu_read_unlock();
Not sure I understand why do we need rq->lock...
It can't protect the change of signal->autogroup, multiple callers can use different rq's.
However. Currently the only callers holds ->siglock, so we are safe. Perhaps we should just document that autogroup_move_group() needs ->siglock.
This also mean the patch can be simplified even more, __sched_move_task() is not needed.
> +void sched_autogroup_fork(struct signal_struct *sig) > +{ > + sig->autogroup = autogroup_kref_get(current->signal->autogroup); > +}
Well, in theory this can race with another thread doing autogroup_move_group(). We can read the old ->autogroup, and then use it after it was already freed.
Probably this needs ->siglock too.
Oleg.
| |