by Frederic Weisbecker

[permalink] [raw]

Hi Thomas,

Thomas Gleixner wrote:
>
> Nothing to be sorry about. That's why we review code.
>

Thanks.

>> Um, but not every timer has it's owner task, for example, if we start
>> a timer in interrupt handle function, this timer in not owns any tasks.
>> And itimer is started by userspace task so we can get it's owner, that
>> why i print hex address for timer/hrtimer, and print task name for itimer.
>
> Well, lot's of timers have an owner task. At least all user space
> related ones. And if the timer is rearmed in interrupt context, then
> this does not change the owner at all.
>

Sorry, i'm confused, for example, has below sequence:

Task1 running----->| (interrupt)
|------------- start timerT(start timerT in interrupt handler)

......
( After a while, schedule to another task, and interruption coming )
......

Task2 running----->| (interrupt)
|------------- start timerT again

Then, which task is the timerT owner?

Am I missed something?

>>> How should that work ?
>>>
>> We put/get timer in a rb-tree base on the specify order, for example:
>> we default use this order:
>>
>> sort_dimension__add("timer", &default_cmp);
>> sort_dimension__add("itimer-type", &default_cmp);
>>
>> if timer_info->timer is bigger, we put it to left child, littler to right
>> child, if the timer_info->timer is the same, then we compare
>> timer_info->itimer_type.
>
> Hmm, I wonder whether a hash table would be more efficient for the
> recording side.
>

Um. i'll record it address your way.

>
>> We search timer base on timer_info->timer and
>> timer_info->itimer_type(not timer_info->type), if we find the
>> timer's type is changed(for example, the timer is "ITIMER" before,
>> and change to "HRTIMER" later), is should a bug. this case is hardly
>> to happen but should catch it.
>
> No, it's not a bug at all. You _can_ have a hrtimer and a timer_list
> timer at the same address in a trace. There are two ways to make that
> happen:
>
> 1) kmalloc'ed memory contains a timer_list. timer operation is done,
> memory is kfreed. Now another kmalloc gets the just freed memory
> and has a hrtimer at the same address which was used by the
> timer_list before.
>
> 2) timer_list and hrtimer are also allocated on stack. There is no
> guarantee that they are at different addresses.
>

Yeah, my mistake.

> Simply because the macro hides the fact that this is an assignment of
> a value to a variable. That makes the code harder to read.
>
> FILLL_RAW_FIELD_VALUE(event, value_sec, data);
> vs.
> value_sec = get_value(data, event, "value_sec");
>
> The latter is fast to parse and entirely clear.
>

Yeah.

> Btw, you agreed above that the open coded call of raw_field_value()
> is clearer than the macro magic. :)
>

Sorry, i misunderstand your mean before.

Thanks,
Xiao

2009-12-22 13:01:52

by Xiao Guangrong

[permalink] [raw]

2009-12-29 05:23:31

by Xiao Guangrong

[permalink] [raw]

Subject: [PATCH v3 3/5] perf tools: fix getting point

The point that got by raw_field_ptr() is point to @data offset not
tracepoint saved.

Introduce raw_field_array() to get array data

Changlog v1->v2:
- fix the typo 'arry' -> 'array' that is pointed out by Frederic Weisbecker

- use '(void *)(unsigned long)value' instead of 'memcpy()' as
Frederic Weisbecker's suggestion

Signed-off-by: Xiao Guangrong <[email protected]>
---
tools/perf/builtin-sched.c | 3 +--
tools/perf/util/trace-event-parse.c | 23 ++++++++++++++++++++---
tools/perf/util/trace-event.h | 3 +++
3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index 702322f..9b91836 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -630,8 +630,7 @@ static void test_calibrations(void)

#define FILL_ARRAY(ptr, array, event, data) \
do { \
- void *__array = raw_field_ptr(event, #array, data); \
- memcpy(ptr.array, __array, sizeof(ptr.array)); \
+ raw_field_array(event, #array, data, ptr.array, sizeof(ptr.array));\
} while(0)

#define FILL_COMMON_FIELDS(ptr, event, data) \
diff --git a/tools/perf/util/trace-event-parse.c b/tools/perf/util/trace-event-parse.c
index c5c32be..037bbe3 100644
--- a/tools/perf/util/trace-event-parse.c
+++ b/tools/perf/util/trace-event-parse.c
@@ -840,7 +840,7 @@ static int event_read_fields(struct event *event, struct format_field **fields)
field->flags |= FIELD_IS_ARRAY;

type = read_token(&token);
- while (strcmp(token, "]") != 0) {
+ while (strcmp(token, "]") != 0) {
if (last_type == EVENT_ITEM &&
type == EVENT_ITEM)
len = 2;
@@ -1919,13 +1919,30 @@ raw_field_value(struct event *event, const char *name, void *data)

void *raw_field_ptr(struct event *event, const char *name, void *data)
{
+ unsigned long long value;
+
+ value = raw_field_value(event, name, data);
+
+ if (!value)
+ return NULL;
+
+ return (void *)(unsigned long)value;
+}
+
+unsigned long long
+raw_field_array(struct event *event, const char *name, void *data,
+ void *array, int array_size)
+{
+ int len;
struct format_field *field;

field = find_any_field(event, name);
if (!field)
- return NULL;
+ return 0ULL;

- return data + field->offset;
+ len = min(array_size, field->size);
+ memcpy(array, data + field->offset, len);
+ return len;
}

static int get_common_info(const char *type, int *offset, int *size)
diff --git a/tools/perf/util/trace-event.h b/tools/perf/util/trace-event.h
index 6ad4056..c3b5c39 100644
--- a/tools/perf/util/trace-event.h
+++ b/tools/perf/util/trace-event.h
@@ -255,6 +255,9 @@ unsigned long long read_size(void *ptr, int size);
unsigned long long
raw_field_value(struct event *event, const char *name, void *data);
void *raw_field_ptr(struct event *event, const char *name, void *data);
+unsigned long long
+raw_field_array(struct event *event, const char *name, void *data,
+ void *array, int array_size);
unsigned long long eval_flag(const char *flag);

int read_tracing_data(int fd, struct perf_event_attr *pattrs, int nb_events);
--
1.6.1.2

2009-12-29 05:23:45

by Xiao Guangrong

[permalink] [raw]

On Wed, 2009-12-30 at 10:36 +0100, Peter Zijlstra wrote:
> On Wed, 2009-12-30 at 10:28 +0100, Ingo Molnar wrote:
> > * Peter Zijlstra <[email protected]> wrote:
> >
> > > On Tue, 2009-12-29 at 13:21 +0800, Xiao Guangrong wrote:
> > > > 'inject' event is a very useful feature and it's suggested by Ingo
> > > > [ See http://lkml.org/lkml/2009/12/28/31 ]
> > >
> > > I really dislike the name, event injection to me would be like a write()
> > > interface where you provide the actual event data to be stuffed in the
> > > output stream.
> > >
> > > This just seems like a very weird way of getting data out. A kind of like
> > > sysconf() but done very strange.
> >
> > What kind of API would you suggest?
>
> sysconf() seems ideal for getting single, mostly constant variables out
> of the kernel, we already have non POSIX (read Linux specific) names in
> there.
>
> _SC_KERNEL_RELOCATION_OFFSET
> _SC_KERNEL_HZ
>
> or somethings like that comes to mind.

OK so there is no sysconf() syscall and its all implemented in glibc,
which is utter suckage.. will have to come up with something saner then.

2009-12-30 11:30:49

by Ingo Molnar

[permalink] [raw]

Subject: Re: [PATCH v3 1/5] perf_event: introduce 'inject' event and get HZ

* Peter Zijlstra <[email protected]> wrote:

> OK so there is no sysconf() syscall and its all implemented in glibc, which
> is utter suckage.. will have to come up with something saner then.

I think integrating it into the perf stream of events would be the most
desirable approach - tooling could make use of it easily.

For data record (and hierarchy) description /debug/tracing/events/ would be
useful.

We dont want to actually 'activate' them in an 'event' fashion - but we could
use event IDs and the record format and the transport to describe and recover
the values.

For transport we could use the mmap ring-buffer and it would also be nice to
add (or resurrect) some 'quick & easy' read()/write() based transport. It a
bit like a value read-out from a counter - just here it's not a regular
counter but some sort of kernel-internal value that we want to provide.

Hm?

Ingo