2009-10-23 12:56:16

by Stephane Eranian

[permalink] [raw]
Subject: [PATCH] perf_events: fix default watermark calculation

This patch fixes the default watermark value for
the sampling buffer. With the existing calculation
(watermark = max(PAGE_SIZE, max_size / 2)), no
notification was ever received when the buffer was
exactly 1 page. This was because you would never
cross the threshold (there is no partial samples).

In certain configuration, there was no possibilty
detecting the problem because there was not enough
space left to store the LOST record.In fact, there
may be a more generic problem here. The kernel should
ensure that there is alaways enough space to store
one LOST record.

This patch sets the default watermark to half the
buffer size. With such limit, we are guaranteed to
get a notification even with a single page buffer
assuming no sample is bigger than a page.

Signed-off-by: Stephane Eranian <[email protected]>

---
kernel/perf_event.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index a69d4ed..e8ec4b7 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -2315,7 +2315,7 @@ perf_mmap_data_init(struct perf_event *event, struct perf_mmap_data *data)
}

if (!data->watermark)
- data->watermark = max_t(long, PAGE_SIZE, max_size / 2);
+ data->watermark = max_size / 2;


rcu_assign_pointer(event->data, data);


2009-11-18 14:35:41

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] perf_events: fix default watermark calculation

On Fri, 2009-10-23 at 14:56 +0200, Stephane Eranian wrote:
> This patch fixes the default watermark value for
> the sampling buffer. With the existing calculation
> (watermark = max(PAGE_SIZE, max_size / 2)), no
> notification was ever received when the buffer was
> exactly 1 page. This was because you would never
> cross the threshold (there is no partial samples).

Right, silly thinko, thanks for catching this.

> In certain configuration, there was no possibilty
> detecting the problem because there was not enough
> space left to store the LOST record.In fact, there
> may be a more generic problem here. The kernel should
> ensure that there is alaways enough space to store
> one LOST record.

It tries to prepend LOST records for each new event (when there is data
lost), so as soon as it manages to write a new event, it will include a
LOST record when appropriate.

> This patch sets the default watermark to half the
> buffer size. With such limit, we are guaranteed to
> get a notification even with a single page buffer
> assuming no sample is bigger than a page.
>
> Signed-off-by: Stephane Eranian <[email protected]>

Acked-by: Peter Zijlstra <[email protected]>

> ---
> kernel/perf_event.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/perf_event.c b/kernel/perf_event.c
> index a69d4ed..e8ec4b7 100644
> --- a/kernel/perf_event.c
> +++ b/kernel/perf_event.c
> @@ -2315,7 +2315,7 @@ perf_mmap_data_init(struct perf_event *event, struct perf_mmap_data *data)
> }
>
> if (!data->watermark)
> - data->watermark = max_t(long, PAGE_SIZE, max_size / 2);
> + data->watermark = max_size / 2;
>
>
> rcu_assign_pointer(event->data, data);

Subject: [tip:perf/core] perf_events: Fix default watermark calculation

Commit-ID: 8904b18046c2f050107f6449e887e7c1142b9ab9
Gitweb: http://git.kernel.org/tip/8904b18046c2f050107f6449e887e7c1142b9ab9
Author: Stephane Eranian <[email protected]>
AuthorDate: Fri, 20 Nov 2009 22:19:57 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Sat, 21 Nov 2009 14:11:41 +0100

perf_events: Fix default watermark calculation

This patch fixes the default watermark value for the sampling
buffer. With the existing calculation (watermark =
max(PAGE_SIZE, max_size / 2)), no notification was ever received
when the buffer was exactly 1 page. This was because you would
never cross the threshold (there is no partial samples).

In certain configuration, there was no possibilty detecting the
problem because there was not enough space left to store the
LOST record.In fact, there may be a more generic problem here.
The kernel should ensure that there is alaways enough space to
store one LOST record.

This patch sets the default watermark to half the buffer size.
With such limit, we are guaranteed to get a notification even
with a single page buffer assuming no sample is bigger than a
page.

Signed-off-by: Stephane Eranian <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Cc: Paul Mackerras <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
LKML-Reference: <[email protected]>
---
kernel/perf_event.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 3ede098..718fa93 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -2340,7 +2340,7 @@ perf_mmap_data_init(struct perf_event *event, struct perf_mmap_data *data)
}

if (!data->watermark)
- data->watermark = max_t(long, PAGE_SIZE, max_size / 2);
+ data->watermark = max_size / 2;


rcu_assign_pointer(event->data, data);