Patchwork [2/5] trace-cmd: Use tracing directory to count CPUs

login
register
mail settings
Submitter Yoshihiro YUNOMAE
Date Aug. 22, 2012, 8:43 a.m.
Message ID <20120822084312.17293.47596.stgit@ltc189.sdl.hitachi.co.jp>
Download mbox | patch
Permalink /patch/179263/
State New
Headers show

Comments

Yoshihiro YUNOMAE - Aug. 22, 2012, 8:43 a.m.
From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Count debugfs/tracing/per_cpu/cpu* to determine the
number of CPUs.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
---

 trace-record.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 41 insertions(+), 0 deletions(-)
Steven Rostedt - Aug. 22, 2012, 1:41 p.m.
On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> 
> Count debugfs/tracing/per_cpu/cpu* to determine the
> number of CPUs.

I'm curious, do you find that sysconf doesn't return the # of CPUs the
system has? I've had boxes where the per_cpu/cpu* had more cpus than the
box actually holds. But this was a bug in the kernel, not the tool. This
change log needs to have rational instead of just explaining what the
patch does.

Thanks,

-- Steve

> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> Signed-off-by: Yoshihiro YUNOMAE <yoshihiro.yunomae.ez@hitachi.com>
> ---
Masami Hiramatsu - Aug. 23, 2012, 2:01 a.m.
(2012/08/22 22:41), Steven Rostedt wrote:
> On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
>> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>
>> Count debugfs/tracing/per_cpu/cpu* to determine the
>> number of CPUs.
> 
> I'm curious, do you find that sysconf doesn't return the # of CPUs the
> system has?

No, sysconf returns the number of hosts CPUs, not guests.

> I've had boxes where the per_cpu/cpu* had more cpus than the
> box actually holds. But this was a bug in the kernel, not the tool. This
> change log needs to have rational instead of just explaining what the
> patch does.

Ah, I see. Hmm, then this should be enabled by a command line
option or an environment variable.

Thank you,
Masami Hiramatsu - Aug. 23, 2012, 3 a.m.
(2012/08/23 11:01), Masami Hiramatsu wrote:
> (2012/08/22 22:41), Steven Rostedt wrote:
>> On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
>>> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>>
>>> Count debugfs/tracing/per_cpu/cpu* to determine the
>>> number of CPUs.
>>
>> I'm curious, do you find that sysconf doesn't return the # of CPUs the
>> system has?
> 
> No, sysconf returns the number of hosts CPUs, not guests.
> 
>> I've had boxes where the per_cpu/cpu* had more cpus than the
>> box actually holds. But this was a bug in the kernel, not the tool. This
>> change log needs to have rational instead of just explaining what the
>> patch does.
> 
> Ah, I see. Hmm, then this should be enabled by a command line
> option or an environment variable.

Oops, I misunderstood. I'll add more comment for why this
should be tried instead of sysconf.

Thank you,
Steven Rostedt - Aug. 23, 2012, 9:08 a.m.
On Thu, 2012-08-23 at 12:00 +0900, Masami Hiramatsu wrote:
> (2012/08/23 11:01), Masami Hiramatsu wrote:
> > (2012/08/22 22:41), Steven Rostedt wrote:
> >> On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
> >>> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> >>>
> >>> Count debugfs/tracing/per_cpu/cpu* to determine the
> >>> number of CPUs.
> >>
> >> I'm curious, do you find that sysconf doesn't return the # of CPUs the
> >> system has?
> > 
> > No, sysconf returns the number of hosts CPUs, not guests.
> > 
> >> I've had boxes where the per_cpu/cpu* had more cpus than the
> >> box actually holds. But this was a bug in the kernel, not the tool. This
> >> change log needs to have rational instead of just explaining what the
> >> patch does.
> > 
> > Ah, I see. Hmm, then this should be enabled by a command line
> > option or an environment variable.
> 
> Oops, I misunderstood. I'll add more comment for why this
> should be tried instead of sysconf.

And now that I understand why you are doing this, why not only do this
if the TRACE_AGENT or DEBUG_TRACING_DIR is defined. That is, if we are
doing it against a bare metal system, then sysconf should suffice, but
if we are tracing against a guest, then it should use the tracing
directory to determine the buffers.

We could add options to override this, but I would think the default
should just Do The Right Thing(tm).

-- Steve
Masami Hiramatsu - Aug. 23, 2012, 12:30 p.m.
(2012/08/23 18:08), Steven Rostedt wrote:
> On Thu, 2012-08-23 at 12:00 +0900, Masami Hiramatsu wrote:
>> (2012/08/23 11:01), Masami Hiramatsu wrote:
>>> (2012/08/22 22:41), Steven Rostedt wrote:
>>>> On Wed, 2012-08-22 at 17:43 +0900, Yoshihiro YUNOMAE wrote:
>>>>> From: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>>>>
>>>>> Count debugfs/tracing/per_cpu/cpu* to determine the
>>>>> number of CPUs.
>>>>
>>>> I'm curious, do you find that sysconf doesn't return the # of CPUs the
>>>> system has?
>>>
>>> No, sysconf returns the number of hosts CPUs, not guests.
>>>
>>>> I've had boxes where the per_cpu/cpu* had more cpus than the
>>>> box actually holds. But this was a bug in the kernel, not the tool. This
>>>> change log needs to have rational instead of just explaining what the
>>>> patch does.
>>>
>>> Ah, I see. Hmm, then this should be enabled by a command line
>>> option or an environment variable.
>>
>> Oops, I misunderstood. I'll add more comment for why this
>> should be tried instead of sysconf.
> 
> And now that I understand why you are doing this, why not only do this
> if the TRACE_AGENT or DEBUG_TRACING_DIR is defined. That is, if we are
> doing it against a bare metal system, then sysconf should suffice, but
> if we are tracing against a guest, then it should use the tracing
> directory to determine the buffers.
> 
> We could add options to override this, but I would think the default
> should just Do The Right Thing(tm).

Yeah, so I'd like to push this is the default method, and fix
the kernel bug (but I'm not sure that is a bug).

Thank you,

Patch

diff --git a/trace-record.c b/trace-record.c
index 9dc18a9..ed18951 100644
--- a/trace-record.c
+++ b/trace-record.c
@@ -1179,6 +1179,41 @@  static void expand_event_list(void)
 	}
 }
 
+static int count_tracingdir_cpus(void)
+{
+	char *tracing_dir = NULL;
+	char *percpu_dir = NULL;
+	struct dirent **namelist;
+	int count = 0, n;
+
+	/* Count cpus in per_cpu directory */
+	tracing_dir = tracecmd_find_tracing_dir();
+	if (!tracing_dir)
+		return 0;
+	percpu_dir = malloc_or_die(strlen(tracing_dir) + 9);
+	if (!percpu_dir)
+		goto err;
+
+	sprintf(percpu_dir, "%s/per_cpu", tracing_dir);
+
+	n = scandir(percpu_dir, &namelist, NULL, alphasort);
+	if (n > 0) {
+		while (n--) {
+			if (strncmp("cpu", namelist[n]->d_name, 3) == 0)
+				count++;
+			free(namelist[n]);
+		}
+		free(namelist);
+	}
+
+	if (percpu_dir)
+		free(percpu_dir);
+err:
+	if (tracing_dir)
+		free(tracing_dir);
+	return count;
+}
+
 static int count_cpus(void)
 {
 	FILE *fp;
@@ -1189,6 +1224,12 @@  static int count_cpus(void)
 	size_t n;
 	int r;
 
+	cpus = count_tracingdir_cpus();
+	if (cpus > 0)
+		return cpus;
+
+	warning("failed to use tracing_dir to determine number of CPUS");
+
 	cpus = sysconf(_SC_NPROCESSORS_CONF);
 	if (cpus > 0)
 		return cpus;