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

[Ravikiran G Thirumalai <kiran@scalex86.org>]

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