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

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

[Stijn Devriendt]

> 'inject' event is a very useful feature and it's suggested by Ingo
> [ See http://lkml.org/lkml/2009/12/28/31 ]

Is it possible to, instead of injecting a specific inject_event enum,
pass a type/config pair (+task/cpu?) to inject the value of that
specific event? Then the HZ could have it's own perf_event type.

This integrates better with what I'm struggling with right now:
counters that are not ever incrementing but rather indicate
a discrete value inside the kernel that might change both
up and down. My dynticks knowledge is rather limited but
if HZ might vary then this approach is definately worth it.

Regards,
Stijn

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

[Xiao Guangrong]

Stijn Devriendt wrote:
>> 'inject' event is a very useful feature and it's suggested by Ingo
>> [ See http://lkml.org/lkml/2009/12/28/31 ]
> 
> Is it possible to, instead of injecting a specific inject_event enum,
> pass a type/config pair (+task/cpu?) to inject the value of that
> specific event? Then the HZ could have it's own perf_event type.
> 

Until now, 'inject' event just trigger parameter/query events, if
you need sample cpu/task, you can use other event.

> This integrates better with what I'm struggling with right now:
> counters that are not ever incrementing but rather indicate
> a discrete value inside the kernel that might change both
> up and down. My dynticks knowledge is rather limited but
> if HZ might vary then this approach is definately worth it.
> 

dynticks not touch HZ at all as i know.

Thanks,
Xiao

[PATCH 0/4] perf_event: introduce 'perf timer' to analyze timer's behavior

[Xiao Guangrong]

Hi,

We introduce 'perf timer' in this patchset, it can analyze timer
latency and timer function handle time, the usage and result is
like below:

# perf timer record
# perf timer lat --print-lat --print-handle
-------------------------------------------------------------------------------------------------------
|   Timer            |   TYPE   |   Avg-latency  | Max-latency  |  Max-latency-at-TS |Max-lat-at-Task |
|0xf7ad1f5c          |hrtimer   |996068.500    ns|1607650     ns|10270128658526      |init            |
|0xf7903f04          |timer     |0.625         HZ|2           HZ|10270344082394      |swapper         |
|0xf787a05c          |hrtimer   |200239.500    ns|359929      ns|10269316024808      |main            |
|main      :[   PROF]|itimer    |0.000         HZ|0           HZ|10237021270557      |main            |
|main      :[VIRTUAL]|itimer    |0.000         HZ|0           HZ|10257314773501      |main            |

......

-------------------------------------------------------------------------------------------------------
|   Timer            |   TYPE   | Avg-handle (ms)|Max-handle(ms)| Max-handle-at-TS(s)|Max-lat-at-func |
|0xf7ad1f5c          |hrtimer   |0.025           |0.025         |10270.129           |0xc016b5b0      |
|0xf7903f04          |timer     |0.009           |0.011         |10260.342           |0xc0159240      |
|0xf787a05c          |hrtimer   |0.031           |0.062         |10267.018           |0xc014cc40      |

And, in current code, it'll complain with below message when we use
'perf timer lat':
 
  # ./perf timer lat
  Warning: unknown op '{'
  Warning: Error: expected type 5 but read 1
  Warning: failed to read event print fmt for hrtimer_start
  Warning: unknown op '{'
  Warning: Error: expected type 5 but read 1
  Warning: failed to read event print fmt for hrtimer_expire_entry
 
 It's because perf parse "hrtimer_start" and "hrtimer_expire_entry" fail,
 but it not hurt using.
 
 See: http://lkml.org/lkml/2009/10/12/726
 
 include/trace/events/timer.h            |    8 
 tools/perf/Documentation/perf-timer.txt |   40 +
 tools/perf/Makefile                     |    1 
 tools/perf/builtin-sched.c              |    3 
 tools/perf/builtin-timer.c              |  954 ++++++++++++++++++++++++++++++++
 tools/perf/builtin.h                    |    1 
 tools/perf/command-list.txt             |    1 
 tools/perf/perf.c                       |    1 
 tools/perf/util/trace-event-parse.c     |   25 
 tools/perf/util/trace-event.h           |    4 
 10 files changed, 1030 insertions(+), 8 deletions(-)

Re: [PATCH 0/4] perf_event: introduce 'perf timer' to analyze timer's behavior

[Frederic Weisbecker]

On Tue, Dec 15, 2009 at 07:17:03PM +0800, Xiao Guangrong wrote:
> Hi,
> 
> We introduce 'perf timer' in this patchset, it can analyze timer
> latency and timer function handle time, the usage and result is
> like below:
> 
> # perf timer record
> # perf timer lat --print-lat --print-handle
> -------------------------------------------------------------------------------------------------------
> |   Timer            |   TYPE   |   Avg-latency  | Max-latency  |  Max-latency-at-TS |Max-lat-at-Task |
> |0xf7ad1f5c          |hrtimer   |996068.500    ns|1607650     ns|10270128658526      |init            |
> |0xf7903f04          |timer     |0.625         HZ|2           HZ|10270344082394      |swapper         |
> |0xf787a05c          |hrtimer   |200239.500    ns|359929      ns|10269316024808      |main            |
> |main      :[   PROF]|itimer    |0.000         HZ|0           HZ|10237021270557      |main            |
> |main      :[VIRTUAL]|itimer    |0.000         HZ|0           HZ|10257314773501      |main            |
> 
> ......
> 
> -------------------------------------------------------------------------------------------------------
> |   Timer            |   TYPE   | Avg-handle (ms)|Max-handle(ms)| Max-handle-at-TS(s)|Max-lat-at-func |
> |0xf7ad1f5c          |hrtimer   |0.025           |0.025         |10270.129           |0xc016b5b0      |
> |0xf7903f04          |timer     |0.009           |0.011         |10260.342           |0xc0159240      |
> |0xf787a05c          |hrtimer   |0.031           |0.062         |10267.018           |0xc014cc40      |
> 
> And, in current code, it'll complain with below message when we use
> 'perf timer lat':
>  
>   # ./perf timer lat
>   Warning: unknown op '{'
>   Warning: Error: expected type 5 but read 1
>   Warning: failed to read event print fmt for hrtimer_start
>   Warning: unknown op '{'
>   Warning: Error: expected type 5 but read 1
>   Warning: failed to read event print fmt for hrtimer_expire_entry


Oh and indeed we have some event format complexities to solve
there.

cat /debug/tracing/events/timer/hrtimer_start/format

print fmt: "hrtimer=%p function=%pf expires=%llu softexpires=%llu", REC->hrtimer, REC->function, (unsigned long 
long)(((ktime_t) { .tv64 = REC->expires }).tv64), (unsigned long long)(((ktime_t) { .tv64 = REC->softexpires 
}).tv64)

We should try to simplify this, may be we should just expose
__entry->expires as is.

[PATCH v2 0/5] perf_event: introduce 'perf timer' to analyze timer's behavior

[Xiao Guangrong]

Hi,

We introduce 'perf timer' in this patchset, it can report
number of activated/expired/canceled timers, timer latency
and timer function runtime.

Changlog v1->v2:
- export HZ in timer's tracepoint, we can use it to get
  TIMER/ITIMER_VIRTUAL/ITIMER_PROF exact latency as Ingo's
  suggestion

- rename 'perf timer latency' to 'perf timer report',
  because it not only show timer latency but also show timer
  function runtime, timer activated/canceled/expired statistics
  and canceled timer list(TODO)

Below changes are all from Thomas's suggestion:
- fix a lot of typos and bad ideas

- use hash table instead of RB tree to record timer

- let output information more readable

- report number of activated/canceled/expired timers

- support to filter timer types:
  '--type' can do it

TODO:
- Show canceled timer list that is suggested by Thomas

- Any suggestion are welcome.

===============================================
Usage:
- record timer events:
  #perf timer record
  #^C

- show timer statistics:
  #perf timer report
 
Statistics
==========
Activated timers number:
	HRTIMER: 37199 TIMER: 828 ITIMER_PROF: 2 ITIMER_VIRTUAL: 2 ITIMER_REAL: 18 

Expired timers number:
 	HRTIMER: 34912 TIMER: 617 ITIMER_PROF: 1 ITIMER_VIRTUAL: 1 ITIMER_REAL: 1 

Canceled timers number:
  	HRTIMER: 37200 TIMER: 829 ITIMER_PROF: 1 ITIMER_VIRTUAL: 1 ITIMER_REAL: 17
 
- show timer latency:
  #perf timer report --print-lat
                            Timer Latency List
[ Abbreviations: Avg = average; lat = latency; ts = timestamp ]
-------------------------------------------------------------------------
     Timer     |     TYPE      | Avg-lat   ms| Max-lat   ms| Max-lat-ts s
-------------------------------------------------------------------------
smbd           |HRTIMER        |1.563        |1.563        |40666.379    
irqbalance     |HRTIMER        |0.842        |2.484        |40670.231    
	...... 
vi             |HRTIMER        |0.123        |0.123        |40637.065    
events/1       |TIMER          |168.420      |1008.000     |40677.006    
events/0       |TIMER          |87.759       |1008.000     |40618.009    
	......
main           |ITIMER_PROF    |0.000        |0.000        |40661.023    
main           |ITIMER_VIRTUAL |0.000        |0.000        |40642.829    
main           |ITIMER_REAL    |0.133        |0.133        |40622.731 


- show timer function runtime:
 #perf timer report --print-runtime
                            Timer Function Runtime List
[ Abbreviations: Avg = average; func = function; rt = runtime; ts = timestamp ]
-------------------------------------------------------------------------------
     Timer     | Type  |Avg-rt ms|Max-rt ms|      Max-rt-func      | Max-rt-ts s
-------------------------------------------------------------------------------
smbd           |HRTIMER|0.020    |0.020    |hrtimer_wakeup         |40666.379
irqbalance     |HRTIMER|0.012    |0.020    |hrtimer_wakeup         |40670.231
	......
events/1       |TIMER  |0.009    |0.017    |delayed_work_timer_fn  |40646.090
events/0       |TIMER  |0.009    |0.091    |delayed_work_timer_fn  |40651.105

- you can use '--print-lat' and '--print-runtime' at the same time:
 #perf timer report --print-runtime --print-lat

                          Timer Latency List
[ Abbreviations: Avg = average; lat = latency; ts = timestamp ]
-------------------------------------------------------------------------
     Timer     |     TYPE      | Avg-lat   ms| Max-lat   ms| Max-lat-ts s
-------------------------------------------------------------------------
smbd           |HRTIMER        |1.563        |1.563        |40666.379    
irqbalance     |HRTIMER        |0.842        |2.484        |40670.231    
	......
	                       Timer Function Runtime List
[ Abbreviations: Avg = average; func = function; rt = runtime; ts = timestamp ]
-------------------------------------------------------------------------------
     Timer     | Type  |Avg-rt ms|Max-rt ms|      Max-rt-func      | Max-rt-ts s
-------------------------------------------------------------------------------
smbd           |HRTIMER|0.020    |0.020    |hrtimer_wakeup         |40666.379
irqbalance     |HRTIMER|0.012    |0.020    |hrtimer_wakeup         |40670.231

- Select timer type which you are interesting:
  #perf timer report --print-lat --type timer
                          Timer Latency List
[ Abbreviations: Avg = average; lat = latency; ts = timestamp ]
-------------------------------------------------------------------------
     Timer     |     TYPE      | Avg-lat   ms| Max-lat   ms| Max-lat-ts s
-------------------------------------------------------------------------
events/1       |TIMER          |168.420      |1008.000     |40677.006    
events/0       |TIMER          |87.759       |1008.000     |40618.009    
events/1       |TIMER          |57.000       |153.000      |40634.571   

You can specify more types, such as: '--type timer,hrtimer'

[PATCH v2 2/5]: trace_event: export HZ in timer's tracepoint format

[Xiao Guangrong]

Export HZ in timer's tracepoint, we can use it to get
TIMER/ITIMER_VIRTUAL/ITIMER_PROF exact latency and it's
suggested by Ingo

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 include/trace/events/timer.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
index 13ec15a..7749ae5 100644
--- a/include/trace/events/timer.h
+++ b/include/trace/events/timer.h
@@ -74,14 +74,17 @@ TRACE_EVENT(timer_expire_entry,
 	TP_STRUCT__entry(
 		__field( void *,	timer	)
 		__field( unsigned long,	now	)
+		__field( int,		hz	)
 	),
 
 	TP_fast_assign(
 		__entry->timer		= timer;
 		__entry->now		= jiffies;
+		__entry->hz		= HZ;
 	),
 
-	TP_printk("timer=%p now=%lu", __entry->timer, __entry->now)
+	TP_printk("timer=%p now=%lu HZ=%d", __entry->timer, __entry->now,
+		  __entry->hz)
 );
 
 /**
-- 
1.6.1.2

Re: [PATCH v2 2/5]: trace_event: export HZ in timer's tracepoint format

[Ingo Molnar]

* Xiao Guangrong <xiaoguangrong@cn.fujitsu.com> wrote:

> Export HZ in timer's tracepoint, we can use it to get
> TIMER/ITIMER_VIRTUAL/ITIMER_PROF exact latency and it's
> suggested by Ingo
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> ---
>  include/trace/events/timer.h |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
> index 13ec15a..7749ae5 100644
> --- a/include/trace/events/timer.h
> +++ b/include/trace/events/timer.h
> @@ -74,14 +74,17 @@ TRACE_EVENT(timer_expire_entry,
>  	TP_STRUCT__entry(
>  		__field( void *,	timer	)
>  		__field( unsigned long,	now	)
> +		__field( int,		hz	)
>  	),
>  
>  	TP_fast_assign(
>  		__entry->timer		= timer;
>  		__entry->now		= jiffies;
> +		__entry->hz		= HZ;
>  	),
>  
> -	TP_printk("timer=%p now=%lu", __entry->timer, __entry->now)
> +	TP_printk("timer=%p now=%lu HZ=%d", __entry->timer, __entry->now,
> +		  __entry->hz)
>  );

I think we can do something slightly different and more efficient: just create 
a new timer event to report the value of HZ.

That way we dont clutter the timer_expire_entry record format with a 
repetitive HZ field. It's an extra 4 bytes overhead: that has to be written, 
passed along, copied and thrown away in 99.9999% of the cases - such overhead 
should be avoided.

If you created a special timer_params event, which would produce precisely one 
event when triggered via say a new perf ioctl. I.e. add something like this to 
perf_event.h:

  #define PERF_EVENT_IOC_INJECT       _IOW('$', 7, __u64)

and add code to kernel/perf_event.c's perf_ioctl() function that takes that 
__u64 parameter as an event ID and injects an 'artificial' event.

Such a new feature would be useful for other things as well: backtesting rare 
events, injecting other types of 'parameter/query events', etc.

There might be more details to this, but it would be a useful scheme IMO - and 
it would still integrate nicely with the whole ftrace event enumeration scheme 
so tooling support would be easier.

What do you think?

	Ingo

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

[Xiao Guangrong]

'inject' event is a very useful feature and it's suggested by Ingo
[ See http://lkml.org/lkml/2009/12/28/31 ]

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 include/linux/perf_event.h  |   13 +++++++++++
 kernel/perf_event.c         |   47 +++++++++++++++++++++++++++++++++++++++++++
 tools/perf/builtin-record.c |   13 +++++++++++
 tools/perf/util/session.c   |    5 ++++
 tools/perf/util/session.h   |    3 +-
 5 files changed, 80 insertions(+), 1 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 9a1d276..6c93f88 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -228,6 +228,7 @@ struct perf_event_attr {
 #define PERF_EVENT_IOC_PERIOD		_IOW('$', 4, __u64)
 #define PERF_EVENT_IOC_SET_OUTPUT	_IO ('$', 5)
 #define PERF_EVENT_IOC_SET_FILTER	_IOW('$', 6, char *)
+#define PERF_EVENT_IOC_INJECT		_IO ('$', 7)
 
 enum perf_event_ioc_flags {
 	PERF_IOC_FLAG_GROUP		= 1U << 0,
@@ -413,10 +414,22 @@ enum perf_event_type {
 	 * };
 	 */
 	PERF_RECORD_SAMPLE		= 9,
+	/*
+	 * struct {
+	 *	struct perf_event_header	header;
+	 *	u32 inject_event_id;
+	 * 	u64 value;
+	 * };
+	 */
+	PERF_RECORD_INJECT		= 10,
 
 	PERF_RECORD_MAX,			/* non-ABI */
 };
 
+enum perf_inject_event {
+	PERF_INJECT_HZ = 0x01,
+};
+
 enum perf_callchain_context {
 	PERF_CONTEXT_HV			= (__u64)-32,
 	PERF_CONTEXT_KERNEL		= (__u64)-128,
diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 8984afd..9343c6c 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -2012,6 +2012,50 @@ unlock:
 	return ret;
 }
 
+static int perf_inject_get_hz(u64 *hz)
+{
+	*hz = HZ;
+	return 0;
+}
+
+static int perf_inject_event(struct perf_event *event, u32 inject_event_id,
+			     int (*get_value)(u64 *))
+{
+	struct perf_output_handle handle;
+	struct perf_inject_event {
+		struct perf_event_header header;
+		u32 inject_event_id;
+		u64 value;
+	} inject_event;
+	int ret = 0;
+
+	inject_event.header.type = PERF_RECORD_INJECT;
+	inject_event.header.misc = 0;
+	inject_event.header.size = sizeof(inject_event);
+	inject_event.inject_event_id = inject_event_id;
+
+	ret = get_value(&inject_event.value);
+	if (ret)
+		goto exit;
+
+	ret = perf_output_begin(&handle, event, inject_event.header.size, 0, 0);
+	if (ret)
+		goto exit;
+
+	perf_output_put(&handle, inject_event);
+	perf_output_end(&handle);
+exit:
+	return ret;
+}
+
+static int perf_inject_ioctl(struct perf_event *event, unsigned int arg)
+{
+	if (!arg || arg & ~PERF_INJECT_HZ)
+		return -EINVAL;
+
+	return perf_inject_event(event, PERF_INJECT_HZ, perf_inject_get_hz);
+}
+
 static int perf_event_set_output(struct perf_event *event, int output_fd);
 static int perf_event_set_filter(struct perf_event *event, void __user *arg);
 
@@ -2044,6 +2088,9 @@ static long perf_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case PERF_EVENT_IOC_SET_FILTER:
 		return perf_event_set_filter(event, (void __user *)arg);
 
+	case PERF_EVENT_IOC_INJECT:
+		return perf_inject_ioctl(event, arg);
+
 	default:
 		return -ENOTTY;
 	}
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 2654253..d13601d 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -65,6 +65,8 @@ static int			file_new			=      1;
 
 static struct perf_session	*session;
 
+u32 inject_events;
+
 struct mmap_data {
 	int			counter;
 	void			*base;
@@ -381,6 +383,17 @@ try_again:
 		}
 	}
 
+	if (inject_events) {
+		ret = ioctl(fd[nr_cpu][counter], PERF_EVENT_IOC_INJECT,
+			    inject_events);
+		if (ret) {
+			error("failed to inject event(%u) with %d (%s)\n",
+			      inject_events, errno, strerror(errno));
+			exit(-1);
+		}
+		inject_events = 0;
+	}
+
 	ioctl(fd[nr_cpu][counter], PERF_EVENT_IOC_ENABLE);
 }
 
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 7f0537d..74f43af 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -179,6 +179,8 @@ static void perf_event_ops__fill_defaults(struct perf_event_ops *handler)
 		handler->throttle = process_event_stub;
 	if (handler->unthrottle == NULL)
 		handler->unthrottle = process_event_stub;
+	if (handler->inject == NULL)
+		handler->inject = process_event_stub;
 }
 
 static const char *event__name[] = {
@@ -192,6 +194,7 @@ static const char *event__name[] = {
 	[PERF_RECORD_FORK]	 = "FORK",
 	[PERF_RECORD_READ]	 = "READ",
 	[PERF_RECORD_SAMPLE]	 = "SAMPLE",
+	[PERF_RECORD_INJECT]	 = "INJECT",
 };
 
 unsigned long event__total[PERF_RECORD_MAX];
@@ -239,6 +242,8 @@ static int perf_session__process_event(struct perf_session *self,
 		return ops->throttle(event, self);
 	case PERF_RECORD_UNTHROTTLE:
 		return ops->unthrottle(event, self);
+	case PERF_RECORD_INJECT:
+		return ops->inject(event, self);
 	default:
 		self->unknown_events++;
 		return -1;
diff --git a/tools/perf/util/session.h b/tools/perf/util/session.h
index 77c5ee2..8742354 100644
--- a/tools/perf/util/session.h
+++ b/tools/perf/util/session.h
@@ -40,7 +40,8 @@ struct perf_event_ops {
 		 lost,
 		 read,
 		 throttle,
-		 unthrottle;
+		 unthrottle,
+		 inject;
 };
 
 struct perf_session *perf_session__new(const char *filename, int mode, bool force);
-- 
1.6.1.2

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

[Peter Zijlstra]

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.

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

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> 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?

	Ingo

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

[Peter Zijlstra]

On Wed, 2009-12-30 at 10:28 +0100, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> 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.

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

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, 2009-12-30 at 10:28 +0100, Ingo Molnar wrote:
> > * Peter Zijlstra <peterz@infradead.org> 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.

Nah. HZ might be static here but we'll have more dynamic parameters so the 
sysconf API is rather inflexible there.

I was thinking of something that fits into existing perf API schemes, to keep 
it simple and self-sufficient, and to allow for extensions/etc.

	Ingo

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

[Peter Zijlstra]

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 <peterz@infradead.org> 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.

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

[Ingo Molnar]

* Peter Zijlstra <peterz@infradead.org> 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

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

[Xiao Guangrong]

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

Yes, it like write and it writes something form kernel to perf.
And, i think this name is suitable for it's doing that injects an
'artificial' event, it's well if you have other name for it :-)

> This just seems like a very weird way of getting data out. A kind of
> like sysconf() but done very strange.
> 

It's since some parameter is only used by perf, suce as HZ in this patch and
'relocation offset' in my other patchset, it also well if has better way to
do it :-)

Thanks,
Xiao

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

[Peter Zijlstra]

On Wed, 2009-12-30 at 17:37 +0800, Xiao Guangrong wrote:
> > 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.
> > 
> 
> Yes, it like write and it writes something form kernel to perf.
> And, i think this name is suitable for it's doing that injects an
> 'artificial' event, it's well if you have other name for it :-)

No, what I means is that you cannot actually inject an arbitrary event.
Sure it injects something, but that's pretty limited.

> > This just seems like a very weird way of getting data out. A kind of
> > like sysconf() but done very strange.
> > 
> 
> It's since some parameter is only used by perf, suce as HZ in this patch and
> 'relocation offset' in my other patchset, it also well if has better way to
> do it :-)

The thing is, this proposed interface is very much tied to perf and
makes it basically impossible for !perf to use. Whereas I can imagine
esp. the relocation offset to be interesting for other things. Basically
everything needing to resolve a kernel symbol.