lkml.org 
[lkml]   [2010]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4] sched: automated per session task groups
    From
    Date
    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



    \
     
     \ /
      Last update: 2010-11-21 17:39    [W:0.023 / U:0.208 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site