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

[Oleg Nesterov <oleg@tv-sign.ru>]

> Ravikiran G Thirumalai wrote:
>
> Following patch avoids taking the global tasklist_lock when possible,
> if a process is single threaded during getrusage().  Any avoidance of
> tasklist_lock is good for NUMA boxes (and possibly for large SMPs).

> --- arch/mips/kernel/irixsig.c.orig	2005-12-27 14:49:57.000000000 -0800
> +++ arch/mips/kernel/irixsig.c	2005-12-27 14:52:47.000000000 -0800
> @@ -540,7 +540,7 @@ out:
>  #define IRIX_P_PGID   2
>  #define IRIX_P_ALL    7
>
> -extern int getrusage(struct task_struct *, int, struct rusage __user *);
> +extern int getrusage_both(struct task_struct *, struct rusage __user *);

I think it's better sense to move this declaration to include/.

> -static void k_getrusage(struct task_struct *p, int who, struct rusage *r)
> +int getrusage_children(struct rusage __user *ru)
>  {
> -	struct task_struct *t;
>  	unsigned long flags;
> +	int lockflag = 0;
>  	cputime_t utime, stime;
> +	struct task_struct *p = current;
> +	struct rusage r;
> +	memset((char *) &r, 0, sizeof (r));
>
> -	memset((char *) r, 0, sizeof *r);
> +	if (!thread_group_empty(p)) {
> +		read_lock(&tasklist_lock);
> +		if (unlikely(!p->signal)) {
> +			read_unlock(&tasklist_lock);
> +			goto ret;

Is this possible? 'current' always has valid signal/sighand.
Or better say, process can't call getrusage after exit_signal().

> +		}
> +		spin_lock_irqsave(&p->sighand->siglock, flags);
> +		lockflag = 1;
> +	}

What if another thread just exited? I think you need 'else smp_rmb()'.
here. Otherwise cpu can read signal->c* out of order.

> +int getrusage_self(struct rusage __user *ru)

Same comments.

> +int getrusage_both(struct task_struct *p, struct rusage __user *ru)
>  {
> +	unsigned long flags;
> +	cputime_t utime, stime;
>  	struct rusage r;
> +	struct task_struct *t;
> +	memset((char *) &r, 0, sizeof (r));
> +
>  	read_lock(&tasklist_lock);
> -	k_getrusage(p, who, &r);
> +	if (unlikely(!p->signal)) {
> +		read_unlock(&tasklist_lock);
> +		goto ret;
> +	}
> +
> +	spin_lock_irqsave(&p->sighand->siglock, flags);
> +	utime = p->signal->cutime;
> +	stime = p->signal->cstime;
> +	r.ru_nvcsw = p->signal->cnvcsw;
> +	r.ru_nivcsw = p->signal->cnivcsw;
> +	r.ru_minflt = p->signal->cmin_flt;
> +	r.ru_majflt = p->signal->cmaj_flt;
> +	spin_unlock_irqrestore(&p->sighand->siglock, flags);
> +
> +	utime = cputime_add(utime, p->signal->utime);
> +	stime = cputime_add(stime, p->signal->stime);
> +	r.ru_nvcsw += p->signal->nvcsw;
> +	r.ru_nivcsw += p->signal->nivcsw;
> +	r.ru_minflt += p->signal->min_flt;
> +	r.ru_majflt += p->signal->maj_flt;
> +
> +	t = p;
> +	do {
> +		utime = cputime_add(utime, t->utime);
> +		stime = cputime_add(stime, t->stime);
> +		r.ru_nvcsw += t->nvcsw;
> +		r.ru_nivcsw += t->nivcsw;
> +		r.ru_minflt += t->min_flt;
> +		r.ru_majflt += t->maj_flt;
> +		t = next_thread(t);
> +	} while (t != p);
> +
>  	read_unlock(&tasklist_lock);
> +	cputime_to_timeval(utime, &r.ru_utime);
> +	cputime_to_timeval(stime, &r.ru_stime);
> +
> +ret:
>  	return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0;
>  }

Looks we can factor out some code.

Actually I dont't understand why can't we move the locking into
k_getrusage,

k_getrusage()

	lock_flag = (p == current && thread_group_empty(p));
	if (lockflag) {
		read_lock(&tasklist_lock);
		spin_lock_irqsave(&p->sighand->siglock, flags);
	}

	and remove ->sighand locking under 'switch' statement.

Isn't this enough to solve perfomance problems?

Oleg.