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

Previous thread: Re: [PATCH V2 2/2] MSP onchip root hub over current quirk. by Alan Stern on Thursday, December 23, 2010 - 9:08 am. (1 message)

Next thread: [GIT PULL] sound fixes for 2.6.37 by Takashi Iwai on Thursday, December 23, 2010 - 9:58 am. (1 message)
From: Dario Faggioli
Date: Thursday, December 23, 2010 - 9:21 am

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 <raistlin@linux.it>
---
 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
+++ ...
From: Oleg Nesterov
Date: Thursday, December 23, 2010 - 9:44 am

Can't comment, I never understood this.



This changes the behaviour of sys_clock_settime(). Probably doesn't
matter since it does nothing, but perhaps !CPUCLOCK_PERTHREAD &&

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.

--

From: Dario Faggioli
Date: Thursday, December 23, 2010 - 10:38 am

If I can ask... What's that you never understood? Why the limitation is
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 -- dario.faggioli@jabber.org
From: Oleg Nesterov
Date: Thursday, December 23, 2010 - 11:12 am

Yes. IOW, I agree it looks strange, clock_gettime() can sample the

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.

--

From: Dario Faggioli
Date: Friday, December 24, 2010 - 4:36 am

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 -- dario.faggioli@jabber.org

From: Randy Dunlap
Date: Thursday, December 23, 2010 - 10:21 am

"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/
--

From: Dario Faggioli
Date: Thursday, December 23, 2010 - 10:43 am

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 -- dario.faggioli@jabber.org
From: Dario Faggioli
Date: Tuesday, December 28, 2010 - 3:55 am

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 <raistlin@linux.it>
---
 kernel/posix-cpu-timers.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/kernel/posix-cpu-timers.c ...
From: Oleg Nesterov
Date: Tuesday, December 28, 2010 - 9:38 am

This patch doesn't look right,


First of all, linux has no clock_getcpuclockid() system call, so
the changelog looks confusing.

glibc implements clock_getcpuclockid() via clock_getres(), that


How so? For example, with this change

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) {

--

From: Dario Faggioli
Date: Tuesday, December 28, 2010 - 2:38 pm

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
Which won't work because CPUCLOCK_PERTHREAD(which_clock) is always false
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
From: Oleg Nesterov
Date: Wednesday, December 29, 2010 - 6:21 am

I think we should change glibc if clock_getcpuclockid() doesn't work,

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

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.

--

From: Dario Faggioli
Date: Wednesday, December 29, 2010 - 7:10 am

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
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
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
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 -- dario.faggioli@jabber.org
From: Oleg Nesterov
Date: Wednesday, December 29, 2010 - 11:30 am

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.

--

From: torbenh
Date: Thursday, December 30, 2010 - 10:45 am

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.

-- 
torben Hohn
--

From: Dario Faggioli
Date: Tuesday, January 4, 2011 - 4:01 am

Well, I knew this, I knew it very well, as you can see from the code on
the git I pointed to (it's part of one of the examples!).

the thing is that I believe there are situations where it could be
useful to sample CLOCK_THREAD_CPUTIME_ID from a _different_ process, and
in that case, you can't access that thread's pthread-id, can you? :-O

The jack2 code I saw could be one of these "potential user", and AFAICT
your jack1 might be another one, no?
Yeah, I know, whether you can/want use this or not also depends on other
issues, but you are in a _different_ process --and thus you can't use
pthread_* calls-- aren't you?

I'm now looking at Oleg's solution and into glibc as well... I'll come
out with something ASAP.

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 -- dario.faggioli@jabber.org
Previous thread: Re: [PATCH V2 2/2] MSP onchip root hub over current quirk. by Alan Stern on Thursday, December 23, 2010 - 9:08 am. (1 message)

Next thread: [GIT PULL] sound fixes for 2.6.37 by Takashi Iwai on Thursday, December 23, 2010 - 9:58 am. (1 message)