Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH v4] sched: automated per session task groups | From | Mike Galbraith <> | Date | Sun, 21 Nov 2010 09:35:49 -0700 |
| |
On Sun, 2010-11-21 at 16:44 +0100, Oleg Nesterov wrote: > 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.
Ok.
> 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).
Oh my. Thanks.
(gad, signal_struct is sooo much harder to get right)
> 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.
Ok,
> 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); > }
I'll do that.. once the thing is non-toxic to tip.
Thanks a lot Oleg.
-Mike
| ![\](/images/icornerr.gif) |