Re: [PATCH] Extending getrusage

[Claudio Scordino <cloud.of.andor@gmail.com>]

On Friday 21 April 2006 16:32, Ulrich Drepper wrote:
> On 4/21/06, Claudio Scordino <cloud.of.andor@gmail.com> wrote:
> > Recently, while writing some code at user level, I needed a fast way
> > to have such information about another process.
> 
> That's not very specific.  And one program isn't really a compelling
> reason.  You should specify with some level of detail why you need
> that information and why you cannot depend on collaboration of the
> process you try to get the information for.

I'm just adding a new feature "for free":

- Without adding any new syscall
- Without changing the prototype of the current syscall (sys_getrusage)
- Ensuring backward compatibility (all old programs will continue to work properly)

This new feature can be used by any process that needs to profile another process for whatever reason.

Since it just adds a new feature without any drawback, I don't understand why it would be wrong...
 
> 
> > -               return -EINVAL;
> > -       return getrusage(current, who, ru);
> > +       struct rusage r;
> > +       struct task_struct *tsk = current;
> > +       read_lock(&tasklist_lock);
> 
> You are introducing scalability problems where there were none before.
>  Even if there is some justification revealed in the end IMO the patch
> shouldn't be accepted in this form.  You should not slow down the
> normal case of operation.  If the current thread is observed the lock
> isn't needed.

I think you're wrong: I'm not introducing any *new* scalability issue. 

Currently the code is:

asmlinkage long sys_getrusage(int who, struct rusage __user *ru)
{
        if (who != RUSAGE_SELF && who != RUSAGE_CHILDREN)
                return -EINVAL;
        return getrusage(current, who, ru);
}

which in turn calls

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;
}

As you can see, the lock is acquired also when not needed. That's how it is currently implemented.

With my patch, k_getrusage is called directly from sys_getrusage without going through getrusage.

               Claudio