2010-12-23 16:21:56

by Dario Faggioli

Subject: [PATCH] Read THREAD_CPUTIME clock from other processes.

Trying to read CLOCK_THREAD_CPUTIME_ID of a thread from outside
the process that spawned it with this code:

if (clock_getcpuclockid(tid, &clockid) != 0) {
perror("clock_getcpuclockid");
exit(EXIT_FAILURE);
}

results in this:
### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds
### Testing tid 24209: clock_getcpuclockid: Success

OTH, if full-fledged processes are involved, the behaviour is this:
### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds
### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds

Test programs available here: http://gitorious.org/clockid.

This is because clock_getcpuclockid forbids accessing thread
specific CPU-time clocks from outside the thread group. This is
not requested (e.g., by POSIX) to be like this, or at least no
indication that such operation should fail can be found in
`man clock_getcpuclockid' and alike.

However, having such capability could be useful, if you want
to monitor the execution of a bunch of thread from some kind of
"manager" which might not be part of the same process. A typical
example that could benefit from this could be the JACK graph-manager.

Therefore, this patch removes such limitation and enables the
following behaviour, for the threaded and process-based case,
respectively:

### Testing tid 24704: CPU-time clock for PID 24704 is 1.049570008 seconds
### Testing tid 24706: CPU-time clock for PID 24706 is 1.028650801seconds

### Testing tid 24715: CPU-time clock for PID 24715 is 0.000957685 seconds
### Testing tid 24717: CPU-time clock for PID 24717 is 1.045351509 seconds

Signed-off-by: Dario Faggioli <[email protected]>
---
kernel/posix-cpu-timers.c | 23 ++++++++++++-----------
1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index 05bb717..b0ed8cf 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -39,10 +39,8 @@ static int check_clock(const clockid_t which_clock)

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)
error = -EINVAL;
- }
rcu_read_unlock();

return error;
@@ -349,18 +347,21 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
rcu_read_lock();
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);
- }
+
+ if (CPUCLOCK_PERTHREAD(which_clock) &&
+ same_thread_group(p, current)) {
+ error = cpu_clock_sample(which_clock,
+ p, &rtn);
} else {
read_lock(&tasklist_lock);
- if (thread_group_leader(p) && p->sighand) {
+ if (!CPUCLOCK_PERTHREAD(which_clock) &&
+ thread_group_leader(p) && p->sighand)
error =
cpu_clock_sample_group(which_clock,
- p, &rtn);
- }
+ p, &rtn);
+ else
+ error = cpu_clock_sample(which_clock,
+ p, &rtn);
read_unlock(&tasklist_lock);
}
}
--
1.7.2.3


--
<<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 -- [email protected]

Attachments:

2010-12-23 16:52:15

by Oleg Nesterov

Subject: Re: [PATCH] Read THREAD_CPUTIME clock from other processes.

On 12/23, Dario Faggioli wrote:
>
> Trying to read CLOCK_THREAD_CPUTIME_ID of a thread from outside
> the process that spawned it with this code:
>
> if (clock_getcpuclockid(tid, &clockid) != 0) {
> perror("clock_getcpuclockid");
> exit(EXIT_FAILURE);
> }
>
> results in this:
> ### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds
> ### Testing tid 24209: clock_getcpuclockid: Success
>
> OTH, if full-fledged processes are involved, the behaviour is this:
> ### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds
> ### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds
>
> Test programs available here: http://gitorious.org/clockid.
>
> This is because clock_getcpuclockid forbids accessing thread
> specific CPU-time clocks from outside the thread group. This is
> not requested (e.g., by POSIX) to be like this, or at least no
> indication that such operation should fail can be found in
> `man clock_getcpuclockid' and alike.
>
> However, having such capability could be useful, if you want
> to monitor the execution of a bunch of thread from some kind of
> "manager" which might not be part of the same process. A typical
> example that could benefit from this could be the JACK graph-manager.
>
> Therefore, this patch removes such limitation and enables the
> following behaviour, for the threaded and process-based case,
> respectively:

Can't comment, I never understood this.


A couple of nits on the patch itself,

> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -39,10 +39,8 @@ static int check_clock(const clockid_t which_clock)
>
> 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)
> error = -EINVAL;
> - }

This changes the behaviour of sys_clock_settime(). Probably doesn't
matter since it does nothing, but perhaps !CPUCLOCK_PERTHREAD &&
!group_leader should result in -EINAVL as before.

> @@ -349,18 +347,21 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
> rcu_read_lock();
> 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);
> - }
> +
> + if (CPUCLOCK_PERTHREAD(which_clock) &&
> + same_thread_group(p, current)) {
> + error = cpu_clock_sample(which_clock,
> + p, &rtn);
> } else {
> read_lock(&tasklist_lock);
> - if (thread_group_leader(p) && p->sighand) {
> + if (!CPUCLOCK_PERTHREAD(which_clock) &&
> + thread_group_leader(p) && p->sighand)
> error =
> cpu_clock_sample_group(which_clock,
> - p, &rtn);
> - }
> + p, &rtn);
> + else
> + error = cpu_clock_sample(which_clock,
> + p, &rtn);
> read_unlock(&tasklist_lock);

Can't understand... why did you duplicate cpu_clock_sample() ?

IOW, it seems to me you could simply kill the
"if (same_thread_group(p, current)) {" line with the same efect, no?

Oleg.

2010-12-23 17:21:15

by Randy Dunlap

Subject: Re: [PATCH] Read THREAD_CPUTIME clock from other processes.

On Thu, 23 Dec 2010 17:21:43 +0100 Dario Faggioli wrote:

> Trying to read CLOCK_THREAD_CPUTIME_ID of a thread from outside
> the process that spawned it with this code:
>
> if (clock_getcpuclockid(tid, &clockid) != 0) {
> perror("clock_getcpuclockid");
> exit(EXIT_FAILURE);
> }
>
> results in this:
> ### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds
> ### Testing tid 24209: clock_getcpuclockid: Success
>
> OTH, if full-fledged processes are involved, the behaviour is this:
> ### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds
> ### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds
>
> Test programs available here: http://gitorious.org/clockid.


"DNS service for this domain has expired with DNS Made Easy" :(

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
desserts: http://www.xenotime.net/linux/recipes/

2010-12-23 17:38:34

by Dario Faggioli

Subject: Re: [PATCH] Read THREAD_CPUTIME clock from other processes.

On Thu, 2010-12-23 at 17:44 +0100, Oleg Nesterov wrote:
> > Therefore, this patch removes such limitation and enables the
> > following behaviour, for the threaded and process-based case,
> > respectively:
>
> Can't comment, I never understood this.
>
If I can ask... What's that you never understood? Why the limitation is
there? Or something else?

> > 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)
> > error = -EINVAL;
> > - }
>
> This changes the behaviour of sys_clock_settime(). Probably doesn't
> matter since it does nothing, but perhaps !CPUCLOCK_PERTHREAD &&
> !group_leader should result in -EINAVL as before.
>
Oops, sure, you're right, I can fix this. :-)

> > @@ -349,18 +347,21 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
> > rcu_read_lock();
> > 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);
> > - }
> > +
> > + if (CPUCLOCK_PERTHREAD(which_clock) &&
> > + same_thread_group(p, current)) {
> > + error = cpu_clock_sample(which_clock,
> > + p, &rtn);
> > } else {
> > read_lock(&tasklist_lock);
> > - if (thread_group_leader(p) && p->sighand) {
> > + if (!CPUCLOCK_PERTHREAD(which_clock) &&
> > + thread_group_leader(p) && p->sighand)
> > error =
> > cpu_clock_sample_group(which_clock,
> > - p, &rtn);
> > - }
> > + p, &rtn);
> > + else
> > + error = cpu_clock_sample(which_clock,
> > + p, &rtn);
> > read_unlock(&tasklist_lock);
>
> Can't understand... why did you duplicate cpu_clock_sample() ?
>
> IOW, it seems to me you could simply kill the
> "if (same_thread_group(p, current)) {" line with the same efect, no?
>
Well, yes, but looking at the original code I thought that in the !
same_thread_group() case I might need the tasklist_lock...

Am I wrong? Is it there just because of cpu_clock_sample_group()?

Thanks and Regards,
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 -- [email protected]

Attachments:

2010-12-23 17:43:31

by Dario Faggioli

Subject: Re: [PATCH] Read THREAD_CPUTIME clock from other processes.

On Thu, 2010-12-23 at 09:21 -0800, Randy Dunlap wrote:
> On Thu, 23 Dec 2010 17:21:43 +0100 Dario Faggioli wrote:
>
> > Trying to read CLOCK_THREAD_CPUTIME_ID of a thread from outside
> > the process that spawned it with this code:
> >
> > if (clock_getcpuclockid(tid, &clockid) != 0) {
> > perror("clock_getcpuclockid");
> > exit(EXIT_FAILURE);
> > }
> >
> > results in this:
> > ### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds
> > ### Testing tid 24209: clock_getcpuclockid: Success
> >
> > OTH, if full-fledged processes are involved, the behaviour is this:
> > ### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds
> > ### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds
> >
> > Test programs available here: http://gitorious.org/clockid.
>
>
> "DNS service for this domain has expired with DNS Made Easy" :(
>
Yep, I saw that... It seems gitorious is having some sort of big big
problem! :-O

I've to run now, so, here's a really quick-&-dirty tar of the repo...

Thanks and Regards,
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 -- [email protected]

Attachments:

2010-12-23 18:19:32

by Oleg Nesterov

Subject: Re: [PATCH] Read THREAD_CPUTIME clock from other processes.

On 12/23, Dario Faggioli wrote:
>
> On Thu, 2010-12-23 at 17:44 +0100, Oleg Nesterov wrote:
> > > Therefore, this patch removes such limitation and enables the
> > > following behaviour, for the threaded and process-based case,
> > > respectively:
> >
> > Can't comment, I never understood this.
> >
> If I can ask... What's that you never understood? Why the limitation is
> there?

Yes. IOW, I agree it looks strange, clock_gettime() can sample the
whole group but not a single thread.

> > > @@ -349,18 +347,21 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
> > > rcu_read_lock();
> > > 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);
> > > - }
> > > +
> > > + if (CPUCLOCK_PERTHREAD(which_clock) &&
> > > + same_thread_group(p, current)) {
> > > + error = cpu_clock_sample(which_clock,
> > > + p, &rtn);
> > > } else {
> > > read_lock(&tasklist_lock);
> > > - if (thread_group_leader(p) && p->sighand) {
> > > + if (!CPUCLOCK_PERTHREAD(which_clock) &&
> > > + thread_group_leader(p) && p->sighand)
> > > error =
> > > cpu_clock_sample_group(which_clock,
> > > - p, &rtn);
> > > - }
> > > + p, &rtn);
> > > + else
> > > + error = cpu_clock_sample(which_clock,
> > > + p, &rtn);
> > > read_unlock(&tasklist_lock);
> >
> > Can't understand... why did you duplicate cpu_clock_sample() ?
> >
> > IOW, it seems to me you could simply kill the
> > "if (same_thread_group(p, current)) {" line with the same efect, no?
> >
> Well, yes, but looking at the original code I thought that in the !
> same_thread_group() case I might need the tasklist_lock...
>
> Am I wrong? Is it there just because of cpu_clock_sample_group()?

Yes, it is because of _group (we are going to sample all sub-threads),
not because of !same_thread_group().

Oh. In fact we should remove this tasklist, but this is another story.

Oleg.

2010-12-24 11:36:26

by Dario Faggioli

Subject: Re: [PATCH] Read THREAD_CPUTIME clock from other processes.

On Thu, 2010-12-23 at 18:38 +0100, Dario Faggioli wrote:
> > > 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)
> > > error = -EINVAL;
> > > - }
> >
> > This changes the behaviour of sys_clock_settime(). Probably doesn't
> > matter since it does nothing, but perhaps !CPUCLOCK_PERTHREAD &&
> > !group_leader should result in -EINAVL as before.
> >
> Oops, sure, you're right, I can fix this. :-)
>
Mmm... This is trickier than I remembered expected. Apparently, glibc
always gives us a !CPUCLOCK_PERTHREAD() clockid, since it uses
MAKE_PROCESS_CLOCK() when clock_cpugetclockid is called.

Therefore, the take two of this thing will be a little different from
this first posting...

Thanks and Regards,
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 -- [email protected]

Attachments:

2010-12-28 10:55:57

by Dario Faggioli

Subject: [PATCH resend] Reading POSIX CPU timer from outside the process.

Trying to call clock_getcpuclockid (and then clock_gettime) on a thread
from outside the process that spawned it results in this:
### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds
### Testing tid 24209: clock_getcpuclockid: Success

OTOH, if full-fledged processes are involved, the behaviour is this:
### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds
### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds

Test programs available here: http://gitorious.org/clockid

All that because clock_getcpuclockid forbids accessing thread
specific CPU-time clocks from outside the thread group, but this is not
requested (e.g., by POSIX), or at least no indication that it should fail
can be found in `man clock_getcpuclockid' and alike.
Moreover, having such capability could be useful, if you want to monitor
the execution of a bunch of thread from some kind of "manager" which might
not be part of the same process. A typical example is the JACK graph-manager.

Therefore, this commit makes clock_getcpuclockid behave as follows:
- if it's called on a tid which is also a PID (i.e., the thread is
a thread group leader), it returns the CLOCK_PROCESS_CPUTIME_ID of
the process;
- if it's called on a tid of a non-group leader, it returns the
CLOCK_THREAD_CPUTIME_ID of such specific thread.

This enables the following behaviour, for the threaded and process-based
scenarios, respectively:

### Testing tid 24704: CPU-time clock for PID 24704 is 1.049570008 seconds
### Testing tid 24706: CPU-time clock for PID 24706 is 1.028650801 seconds

### Testing tid 24715: CPU-time clock for PID 24715 is 0.000957685 seconds
### Testing tid 24717: CPU-time clock for PID 24717 is 1.045351509 seconds

Signed-off-by: Dario Faggioli <[email protected]>
---
kernel/posix-cpu-timers.c | 13 +++++--------
1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index 05bb717..ddbbabc 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -39,10 +39,9 @@ static int check_clock(const clockid_t which_clock)

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();

return error;
@@ -349,11 +348,9 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
rcu_read_lock();
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);
- }
+ if (CPUCLOCK_PERTHREAD(which_clock) ||
+ !thread_group_leader(p)) {
+ error = cpu_clock_sample(which_clock, p, &rtn);
} else {
read_lock(&tasklist_lock);
if (thread_group_leader(p) && p->sighand) {
--
1.7.2.3


--
<<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 -- [email protected]

Attachments:

2010-12-28 16:46:14

by Oleg Nesterov

Subject: Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

This patch doesn't look right,

On 12/28, Dario Faggioli wrote:
>
> Trying to call clock_getcpuclockid (and then clock_gettime) on a thread
> from outside the process that spawned it results in this:
> ### Testing tid 24207: CPU-time clock for PID 24207 is 1.132371729 seconds
> ### Testing tid 24209: clock_getcpuclockid: Success
>
> OTOH, if full-fledged processes are involved, the behaviour is this:
> ### Testing tid 24218: CPU-time clock for PID 24218 is 0.001059305 seconds
> ### Testing tid 24220: CPU-time clock for PID 24220 is 1.044057391 seconds
>
> Test programs available here: http://gitorious.org/clockid
>
> 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.

glibc implements clock_getcpuclockid() via clock_getres(), that
is why the change in check_clock() can help.

> Therefore, this commit makes clock_getcpuclockid behave as follows:
> - if it's called on a tid which is also a PID (i.e., the thread is
> a thread group leader), it returns the CLOCK_PROCESS_CPUTIME_ID of
> the process;
> - if it's called on a tid of a non-group leader, it returns the
> CLOCK_THREAD_CPUTIME_ID of such specific thread.

And both changes look wrong to me.

> --- a/kernel/posix-cpu-timers.c
> +++ b/kernel/posix-cpu-timers.c
> @@ -39,10 +39,9 @@ static int check_clock(const clockid_t which_clock)
>
> 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?

> @@ -349,11 +348,9 @@ int posix_cpu_clock_get(const clockid_t which_clock, struct timespec *tp)
> rcu_read_lock();
> 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);
> - }
> + if (CPUCLOCK_PERTHREAD(which_clock) ||
> + !thread_group_leader(p)) {
> + error = cpu_clock_sample(which_clock, p, &rtn);

IOW, we ignore CPUCLOCK_PERTHREAD() if !thread_group_leader(),
this can't be right.

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.

Oleg.

--- 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();

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);
} else {
read_lock(&tasklist_lock);
if (thread_group_leader(p) && p->sighand) {

2010-12-28 21:38:41

by Dario Faggioli

Subject: Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

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 -- [email protected]

Attachments:

2010-12-29 13:29:11

by Oleg Nesterov

Subject: Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

On 12/28, Dario Faggioli wrote:
>
> On Tue, 2010-12-28 at 17:38 +0100, Oleg Nesterov wrote:
>
> > > 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,

No, please note pid_of_sub_thread above.

> 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 we should change glibc if clock_getcpuclockid() doesn't work,
please see below.

> > 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.

I guess, this is because glibc passes MAKE_PROCESS_CLOCK() id, right?
But we shouldn't add the hacks to the kernel to hide the limitations
in glibc.

> 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.

Yes, please.


BTW. What is the test-case? I am looking at http://gitorious.org/clockid,
I guess it is clockid.c...

You do not need clock_getcpuclockid() at all. In fact I do not really
understand what this helper should actually do, probably it is only
needed to validate the pid. You can simply use MAKE_THREAD_CPUCLOCK()
to sample a single thread via clock_gettime().

IOW. Unless I missed something, with this patch, the only problem
is that getcpuclockid() always assumes MAKE_PROCESS_CPUCLOCK(),
I do not think this is the kernel problem.

Oleg.

2010-12-29 14:10:36

by Dario Faggioli

Subject: Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

On Wed, 2010-12-29 at 14:21 +0100, Oleg Nesterov wrote:
> > > 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,
>
> No, please note pid_of_sub_thread above.
>
Sure, I saw that... I was referring to all the clock_getres() and
clock_{get,set}time() I was able to call without using directly
MAKE_THREAD_CPUCLOCK.

> > Which won't work because CPUCLOCK_PERTHREAD(which_clock) is always false
> > in this case.
>
> I guess, this is because glibc passes MAKE_PROCESS_CLOCK() id, right?
> But we shouldn't add the hacks to the kernel to hide the limitations
> in glibc.
>
Exactly. Actually, I didn't noticed this too when I first started with
this patch, and the fact that my first version worked -- which was by
chance, I can say it now :-P -- made me think it was worthwhile to bend
the semantic a bit, to enable this new capability... But making it work
is hackish, I see it now. Sorry. :-)

> BTW. What is the test-case? I am looking at http://gitorious.org/clockid,
> I guess it is clockid.c...
>
Yep. A very trivial one, I agree, but it's just to show the issue.

> You do not need clock_getcpuclockid() at all. In fact I do not really
> understand what this helper should actually do, probably it is only
> needed to validate the pid. You can simply use MAKE_THREAD_CPUCLOCK()
> to sample a single thread via clock_gettime().
>
Fine, but, is that macro available for an application developer? Because
I can find it in kernel and glibc sources, but not in my /usr/include/*,
which is the motivation behind this attempt... But it might be my
fault! :-P

> IOW. Unless I missed something, with this patch, the only problem
> is that getcpuclockid() always assumes MAKE_PROCESS_CPUCLOCK(),
> I do not think this is the kernel problem.
>
Agreed, sorry for wasting (hopefully not too much) people's time. :-(

Thanks and Regards,
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 -- [email protected]

Attachments:

2010-12-29 18:37:56

by Oleg Nesterov

Subject: Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

On 12/29, Dario Faggioli wrote:
>
> On Wed, 2010-12-29 at 14:21 +0100, Oleg Nesterov wrote:
>
> > You do not need clock_getcpuclockid() at all. In fact I do not really
> > understand what this helper should actually do, probably it is only
> > needed to validate the pid. You can simply use MAKE_THREAD_CPUCLOCK()
> > to sample a single thread via clock_gettime().
> >
> Fine, but, is that macro available for an application developer? Because
> I can find it in kernel and glibc sources, but not in my /usr/include/*,
> which is the motivation behind this attempt... But it might be my
> fault! :-P

Yes, I do not see MAKE_*_CPUCLOCK() in /usr/include.

> > IOW. Unless I missed something, with this patch, the only problem
> > is that getcpuclockid() always assumes MAKE_PROCESS_CPUCLOCK(),
> > I do not think this is the kernel problem.
> >
> Agreed, sorry for wasting (hopefully not too much) people's time. :-(

No, I think you have a point. I'd suggest you to re-send the
patch which removes this limitation from kernel side.

My only objection was, we shouldn't add the hacks to overcome
the limitations in glibc. Say, posix_cpu_clock_get() should only
check CPUCLOCK_PERTHREAD(), it should not treat !group_leader
specially just because getcpuclockid() can construct the proper
clock id. This should be solved in userland.

Oleg.

2010-12-30 17:45:37

by torbenh

Subject: Re: [PATCH resend] Reading POSIX CPU timer from outside the process.

On Wed, Dec 29, 2010 at 02:21:30PM +0100, Oleg Nesterov wrote:
> On 12/28, Dario Faggioli wrote:
> >
> > On Tue, 2010-12-28 at 17:38 +0100, Oleg Nesterov wrote:
> >
> > > > 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,
>
> No, please note pid_of_sub_thread above.
>
> > 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 we should change glibc if clock_getcpuclockid() doesn't work,
> please see below.

http://www.kernel.org/doc/man-pages/online/pages/man3/pthread_getcpuclockid.3.html

this one works.
ok... it takes a pthread_t for identifying the thread.
but it works.

>
> > > 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.
>
> I guess, this is because glibc passes MAKE_PROCESS_CLOCK() id, right?
> But we shouldn't add the hacks to the kernel to hide the limitations
> in glibc.
>
> > 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.
>
> Yes, please.
>
>
> BTW. What is the test-case? I am looking at http://gitorious.org/clockid,
> I guess it is clockid.c...
>
> You do not need clock_getcpuclockid() at all. In fact I do not really
> understand what this helper should actually do, probably it is only
> needed to validate the pid. You can simply use MAKE_THREAD_CPUCLOCK()
> to sample a single thread via clock_gettime().
>
> IOW. Unless I missed something, with this patch, the only problem
> is that getcpuclockid() always assumes MAKE_PROCESS_CPUCLOCK(),
> I do not think this is the kernel problem.
>
> Oleg.
>

--
torben Hohn