Messages in this thread | ![/](/images/icornerl.gif) | | Date | Sun, 21 Nov 2010 16:44:35 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH v4] sched: automated per session task groups |
| |
On 11/21, Ingo Molnar wrote: > > Btw., there's a small cleanup in the patch that i picked up (see below), and i also > edited the commit log a bit - so you might want to pick up the version below.
I didn't read the patch in details, but a couple of nits...
> +static void > +autogroup_move_group(struct task_struct *p, struct autogroup *ag) > +{ > + struct autogroup *prev; > + struct task_struct *t; > + > + spin_lock(&p->sighand->siglock);
This needs spin_lock_irq(), ->siglock is irq-safe.
The same for other lockers, but:
> +static inline struct autogroup *autogroup_get(struct task_struct *p) > +{ > + struct autogroup *ag; > + > + /* task may be moved after we unlock.. tough */ > + spin_lock(&p->sighand->siglock);
This is called by fs/proc. In this case nothing protects us from release_task(), we can hit ->siglock == NULL (or we can race with exec which changes ->sighand in theory).
This needs lock_task_sighand() (it can fail). Perhaps something else have the same problem...
If the task is current and it is not exiting, or it is the new child (sched_autogroup_fork), then it is safe to use ->siglock directly.
And a pure cosmetic nit,
> +void sched_autogroup_fork(struct signal_struct *sig) > +{ > + struct sighand_struct *sighand = current->sighand; > + > + spin_lock(&sighand->siglock); > + sig->autogroup = autogroup_kref_get(current->signal->autogroup); > + spin_unlock(&sighand->siglock); > +}
This looks a bit confusing. We do not need current->sighand->siglock to set sig->autogroup. Nobody except us can see this new signal_struct, and in any case current->sighand->siglock can't help.
It is needed for autogroup_kref_get(), but we already have autogroup_get(). I'd suggest
void sched_autogroup_fork(struct signal_struct *sig) { sig->autogroup = autogroup_get(current); }
Oleg.
| ![\](/images/icornerr.gif) |