Messages in this thread Patch in this message | | | Subject | Re: [PATCH resend] Reading POSIX CPU timer from outside the process. | From | Dario Faggioli <> | Date | Tue, 28 Dec 2010 22:38:24 +0100 |
| |
On Tue, 2010-12-28 at 17:38 +0100, Oleg Nesterov wrote: > This patch doesn't look right, > Sorry then... :-(
> > 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.
> > 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
> 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.
> 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 [unhandled content-type:application/pgp-signature] | |