lkml.org 
[lkml]   [2006]   [Jan]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [rfc][patch] Avoid taking global tasklist_lock for single threadedprocess at getrusage()
    Sorry for the delay..

    On Tue, Jan 10, 2006 at 10:03:35PM +0300, Oleg Nesterov wrote:
    > Ravikiran G Thirumalai wrote:
    > >
    > > > >
    > > > > Don't we still need rmb for the RUSAGE_SELF case? we do not take the
    > > > > siglock for rusage self and the non c* signal fields are written to
    > > > > at __exit_signal...
    > > >
    > > > I think it is unneeded because RUSAGE_SELF case is "racy" anyway even
    > > > if we held both locks, task_struct->xxx counters can change at any
    > > > moment.
    > > >
    > > > But may be you are right.
    > >
    > > Hmm...access to task_struct->xxx has been racy, but accessing the
    > > signal->* counters were not. What if read of the signal->utime was a
    > > hoisted read and signal->stime was a read after the counter is updated?
    > > This was not a possibility earlier no?
    >
    > Sorry, I can't undestand. Could you please be more verbose ?

    What I meant to say was, if a thread has just exited, since we do not use
    locks anymore in ST case, the read of signal->utime may happen out of order,

    (excuse long lines)

    Last thread (RUSAGE_SELF) Exiting thread


    k_getrusage() __exit_signal()
    . .
    load sig->utime (hoisted read) .
    . sig->utime = cputime_add(sig->utime, tsk->utime);
    . sig->stime = cputime_add(sig->stime, tsk->stime);
    .
    .
    spin_unlock(&sighand->siglock); --> (A)
    .
    __unhash_process()
    .
    detach_pid(p, PIDTYPE_PGID);
    if (!thread_group_empty()) .
    .
    don't take any lock based on if --> (B)
    .
    .
    utime = cputime_add(utime, p->signal->utime); /* use cached load above */
    stime = cputime_add(stime, p->signal->stime); /* load from memory */

    So although writes happen in order due to (A) above, there is no guarantee
    interms of read order when we do not take locks,(as far as my understanding
    goes) so I think a rmb() is needed at (B), else as in this example, some
    counters may have values before the exiting thread updated the sig-> fields
    and some after the thread updated the sig-> fields. This might have a
    significant effect than the task_struct->xxx inaccuracies. Of course
    this is theoretical. This was not a possibility earlier because
    __exit_signal and k_getrusage() could not run at the same time due to the
    exiting thread taking tasklist lock for write and k_getrusage thread taking
    the lock for read.
    I am also cc'ing experts in memory re-ordering issues to check if I am
    missing something :)

    I think we need a rmb() at sys_times too based on the above. I will make a
    patch for that.


    >
    > > >
    > > > > What is wrong with optimizing by not taking the siglock in RUSAGE_BOTH
    > > > > and RUSAGE_CHILDREN? I would like to add that in too unless I am
    > > > > missing something and the optimization is incorrect.
    > > >
    > > > We can't have contention on ->siglock when need_lock == 0, so why should
    > > > we optimize this case?
    > >
    > > We would be saving 1 buslocked operation in that case (on some arches), a
    > > cacheline fetch for exclusive (since signal and sighand are on different memory
    > > locations), and disabling/enabling onchip interrupts. But yes, this would be a
    > > smaller optimization....Unless you have strong objections this can also
    > > go in?
    >
    > I don't have strong objections, but I am not a maintainer.
    >
    > However, do you have any numbers or thoughts why this optimization
    > can make any _visible_ effect?

    We know we don't need locks there, so I do not understand why we
    should keep them. Locks are always serializing and expensive operations. I
    believe on some arches disabling on-chip interrupts is also an expensive
    operation...some arches might use hypervisor calls to do that which I guess
    will have its own overhead...so why have it when we know we don't need it?

    Thanks,
    Kiran
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-01-16 21:58    [W:0.161 / U:0.468 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site