> 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.
On Fri, Dec 30, 2005 at 08:57:41PM +0300, Oleg Nesterov wrote:
> > - 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().
You are right, this check was unnecessary
>
> > + }
> > + 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.
Yes, looks like we do. We probably need to do the same at sys_times too...
>
> > }
>
> 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?
Hmm, just that getrusage_self takes the siglock in the multi threaded
case which is not needed I think. Howz this patch?
getrusage_tasklistlock_optimization-v4
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).
Index: linux-2.6.15-rc6/kernel/sys.c
===================================================================
--- linux-2.6.15-rc6.orig/kernel/sys.c 2006-01-03 12:12:05.000000000 -0800
+++ linux-2.6.15-rc6/kernel/sys.c 2006-01-04 13:41:17.000000000 -0800
@@ -1664,9 +1664,6 @@ asmlinkage long sys_setrlimit(unsigned i
* a lot simpler! (Which we're not doing right now because we're not
* measuring them yet).
*
- * This expects to be called with tasklist_lock read-locked or better,
- * and the siglock not locked. It may momentarily take the siglock.
- *
* When sampling multiple threads for RUSAGE_SELF, under SMP we might have
* races with threads incrementing their own counters. But since word
* reads are atomic, we either get new values or old values and we don't
@@ -1674,6 +1671,25 @@ asmlinkage long sys_setrlimit(unsigned i
* the c* fields from p->signal from races with exit.c updating those
* fields when reaping, so a sample either gets all the additions of a
* given child after it's reaped, or none so this sample is before reaping.
+ *
+ * tasklist_lock locking optimisation:
+ * If we are current and single threaded, we do not need to take the tasklist
+ * lock or the siglock. No one else can take our signal_struct away,
+ * no one else can reap the children to update signal->c* counters, and
+ * no one else can race with the signal-> fields.
+ * If we do not take the tasklist_lock, the signal-> fields could be read
+ * out of order while another thread was just exiting. So we place a
+ * read memory barrier when we avoid the lock. On the writer side,
+ * write memory barrier is implied in __exit_signal as __exit_signal releases
+ * the siglock spinlock after updating the signal-> fields.
+ *
+ * We don't really need the siglock when we access the non c* fields
+ * of the signal_struct (for RUSAGE_SELF) even in multithreaded
+ * case, since we take the tasklist lock for read and the non c* signal->
+ * fields are updated only in __exit_signal, which is called with
+ * tasklist_lock taken for write, hence these two threads cannot execute
+ * concurrently.
+ *
*/
static void k_getrusage(struct task_struct *p, int who, struct rusage *r)
@@ -1681,37 +1697,51 @@ static void k_getrusage(struct task_stru
struct task_struct *t;
unsigned long flags;
cputime_t utime, stime;
+ int need_lock = 0;
memset((char *) r, 0, sizeof *r);
- if (unlikely(!p->signal))
- return;
+ need_lock = (p == current && thread_group_empty(p));
+
+ if (need_lock) {
+ read_lock(&tasklist_lock);
+ if (unlikely(!p->signal)) {
+ read_unlock(&tasklist_lock);
+ return;
+ }
+ } else
+ /* See locking comments above */
+ smp_rmb();
switch (who) {
case RUSAGE_CHILDREN:
- spin_lock_irqsave(&p->sighand->siglock, flags);
+ if (need_lock)
+ 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);
+ if (need_lock)
+ spin_unlock_irqrestore(&p->sighand->siglock, flags);
cputime_to_timeval(utime, &r->ru_utime);
cputime_to_timeval(stime, &r->ru_stime);
break;
case RUSAGE_SELF:
- spin_lock_irqsave(&p->sighand->siglock, flags);
utime = stime = cputime_zero;
goto sum_group;
case RUSAGE_BOTH:
- spin_lock_irqsave(&p->sighand->siglock, flags);
+ if (need_lock)
+ 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;
+ if (need_lock)
+ spin_unlock_irqrestore(&p->sighand->siglock, flags);
sum_group:
utime = cputime_add(utime, p->signal->utime);
stime = cputime_add(stime, p->signal->stime);
@@ -1729,21 +1759,21 @@ static void k_getrusage(struct task_stru
r->ru_majflt += t->maj_flt;
t = next_thread(t);
} while (t != p);
- spin_unlock_irqrestore(&p->sighand->siglock, flags);
cputime_to_timeval(utime, &r->ru_utime);
cputime_to_timeval(stime, &r->ru_stime);
break;
default:
BUG();
}
+
+ if (need_lock)
+ read_unlock(&tasklist_lock);
}
int getrusage(struct task_struct *p, int who, struct rusage __user *ru)
{
struct rusage r;
- read_lock(&tasklist_lock);
k_getrusage(p, who, &r);
- read_unlock(&tasklist_lock);
return copy_to_user(ru, &r, sizeof(r)) ? -EFAULT : 0;
}