lkml.org 
[lkml]   [2010]   [Dec]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch v2 4/4] taskstats: Export "cdata_wait" CPU times with taskstats
    On 12/13, Michael Holzheu wrote:
    >
    > On Mon, 2010-12-13 at 14:05 +0100, Michael Holzheu wrote:
    > > > And this looks racy, or I missed something again. group_dead can be
    > > > true, but this doesn't mean all other threads have already passed
    > > > taskstats_exit()->fill_tgid_exit()->delayacct_add_tsk().
    > >
    > > I think you are right.
    > >
    > > One way to fix that could be to separate the aggregation from the
    > > sending. We could call fill_tgid_exit()->delayacct_add_tsk() before
    > > atomic_dec_and_test(&tsk->signal->live) in do_exit() and
    > > taskstats_exit() with the sender part afterwards.

    Yes, I think this should fix the race. Some nits below...

    > --- a/include/linux/taskstats_kern.h
    > +++ b/include/linux/taskstats_kern.h
    > @@ -21,7 +21,8 @@ static inline void taskstats_tgid_free(s
    > kmem_cache_free(taskstats_cache, sig->stats);
    > }
    >
    > -extern void taskstats_exit(struct task_struct *, int group_dead);
    > +extern void taskstats_exit_notify(struct task_struct *, int group_dead);
    > +extern void taskstats_exit_add_thread(struct task_struct *);

    You forgot to update the !CONFIG_TASKSTATS case ;)

    > -static void fill_tgid_exit(struct task_struct *tsk)
    > +static void alloc_signal_stats(struct task_struct *tsk)
    > +{
    > + struct signal_struct *sig = tsk->signal;
    > + struct taskstats *stats;
    > +
    > + /* No problem if kmem_cache_zalloc() fails */
    > + stats = kmem_cache_zalloc(taskstats_cache, GFP_KERNEL);
    > +
    > + spin_lock_irq(&tsk->sighand->siglock);
    > + if (!sig->stats) {
    > + sig->stats = stats;
    > + stats = NULL;
    > + }
    > + spin_unlock_irq(&tsk->sighand->siglock);
    > +
    > + if (stats)
    > + kmem_cache_free(taskstats_cache, stats);
    > +}
    > +
    > +void taskstats_exit_add_thread(struct task_struct *tsk)
    > {
    > unsigned long flags;
    >
    > + if (tsk->signal->stats == NULL && !thread_group_empty(tsk))
    > + alloc_signal_stats(tsk);
    > +
    > spin_lock_irqsave(&tsk->sighand->siglock, flags);
    > if (!tsk->signal->stats)
    > goto ret;

    Well. I do not like the fact we take ->siglock unconditionally.
    And _irqsave is not needed. And we take it twice if sig->stats == NULL.
    And "if (!tsk->signal->stats)" under ->siglock in
    taskstats_exit_add_thread() looks a bit ugly...

    How about

    void taskstats_exit_add_thread(struct task_struct *tsk)
    {
    struct taskstats *stats = NULL;

    if (!tsk->signal->stats) {
    if (thread_group_empty(tsk)
    return;

    stats = kmem_cache_zalloc(taskstats_cache, GFP_KERNEL);
    if (!stats)
    return;
    }

    spin_lock_irq(&tsk->sighand->siglock);
    if (!tsk->signal->stats) {
    tsk->signal->stats = stats;
    stats = NULL;
    }
    /*
    * Each accounting subsystem calls its functions here to
    * accumalate its per-task stats for tsk, into the per-tgid structure
    *
    * per-task-foo(tsk->signal->stats, tsk);
    */
    delayacct_add_tsk(tsk->signal->stats, tsk);
    spin_unlock_irq(&tsk->sighand->siglock);

    if (unlikely(stats))
    kmem_cache_free(taskstats_cache, stats);
    }

    ?

    Note that it absorbs alloc_signal_stats().

    But up to you, probably this is matter of taste.

    Oleg.



    \
     
     \ /
      Last update: 2010-12-13 15:43    [W:0.030 / U:65.144 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site