Re: [rfc][patch] Avoid taking global tasklist_lock for single threadedprocess at getrusage()

Ravikiran G Thirumalai
2006-01-16T20:56:34Z

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