On Tue, 2010-12-28 at 17:38 +0100, Oleg Nesterov wrote: 
quoted text
> This patch doesn't look right,
> 
Sorry then... :-(

quoted text
> > All that because clock_getcpuclockid forbids accessing thread
> > specific CPU-time clocks from outside the thread group,
> 
> First of all, linux has no clock_getcpuclockid() system call, so
> the changelog looks confusing.
> 
Sure, you're right, this could have been more clear.

quoted text
> >  	rcu_read_lock();
> >  	p = find_task_by_vpid(pid);
> > -	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
> > -		   same_thread_group(p, current) : has_group_leader_pid(p))) {
> > +	if (!p || (CPUCLOCK_PERTHREAD(which_clock) &&
> > +		   same_thread_group(p, current) && !has_group_leader_pid(p)))
> >  		error = -EINVAL;
> > -	}
> >  	rcu_read_unlock();
> 
> How so? For example, with this change
> clock_getres(MAKE_THREAD_CPUCLOCK(pid_of_sub_thread)) won't work, no?
> 
I tested all the clock_getres() calls that came to my mind (at least the
one that are possible from an userspace program), and they always worked
because of this (still in check_clock):

        const pid_t pid = CPUCLOCK_PID(which_clock);

        if (pid == 0)
                return 0;

Which triggers all the times, except when you actually try to get a CPU
clockid from outside the process, but that's not possible with getres.

Anyway, looking at the code again I agree, it may work, but it's not
something I really like! :-|

The whole point was about, given the current implementation of
clock_getcpuclockid done by glibc, can we remove that "failed with
success" (showed in the changelog) thing and come up with some
meaningful clockid for that situation? It's more than possible for the
answer to be no!!! :-P

quoted text
> I think, if we want to remove this limitation, we need something
> like the patch below. If it doesn't help, we should fix glibc.
> 
> --- x/kernel/posix-cpu-timers.c
> +++ x/kernel/posix-cpu-timers.c
> @@ -39,10 +39,8 @@ static int check_clock(const clockid_t w
>  
>  	rcu_read_lock();
>  	p = find_task_by_vpid(pid);
> -	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) ?
> -		   same_thread_group(p, current) : has_group_leader_pid(p))) {
> +	if (!p || !(CPUCLOCK_PERTHREAD(which_clock) || has_group_leader_pid(p)))
>  		error = -EINVAL;
> -	}
>  	rcu_read_unlock();
>
Which won't work because CPUCLOCK_PERTHREAD(which_clock) is always false
in this case. 

quoted text
> 	return error;
> @@ -350,10 +348,7 @@ int posix_cpu_clock_get(const clockid_t 
>  		p = find_task_by_vpid(pid);
>  		if (p) {
>  			if (CPUCLOCK_PERTHREAD(which_clock)) {
> -				if (same_thread_group(p, current)) {
> -					error = cpu_clock_sample(which_clock,
> -								 p, &rtn);
> -				}
> +				error = cpu_clock_sample(which_clock, p, &rtn);
Same as above... To the point that I'm now wondering if we ever take
this branch here...

BTW, again, I see your point, the fix might need to happen at glibc
level. I'll check that and come back if I find something interesting.

Thanks anyway,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
----------------------------------------------------------------------
Dario Faggioli, ReTiS Lab, Scuola Superiore Sant'Anna, Pisa  (Italy)

http://retis.sssup.it/people/faggioli -- dario.faggioli@jabber.org