linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] perf bench: fix assert when NDEBUG is defined
@ 2012-09-03  0:04 Irina Tirdea
  2012-09-03  1:45 ` Namhyung Kim
  2012-09-05 11:00 ` Peter Zijlstra
  0 siblings, 2 replies; 5+ messages in thread
From: Irina Tirdea @ 2012-09-03  0:04 UTC (permalink / raw)
  To: Steven Rostedt, Arnaldo Carvalho de Melo, Ingo Molnar
  Cc: LKML, Namhyung Kim, Peter Zijlstra, Frederic Weisbecker

From: Irina Tirdea <irina.tirdea@intel.com>

When NDEBUG is defined, the assert macro will be expanded to nothing.
Some assert calls used in perf are also including some functionality
(e.g. system calls), not only validity checks. Therefore, if NDEBUG is
defined, these functionality will be removed along with the assert.

The functionality of the program needs to be separated from the assert checks.
In perf, BUG_ON is also defined on assert, so we need to fix these statements
too.

Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
 tools/perf/bench/mem-memcpy.c |    8 +++++---
 tools/perf/bench/mem-memset.c |    8 +++++---
 tools/perf/bench/sched-pipe.c |    6 ++++--
 3 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/tools/perf/bench/mem-memcpy.c b/tools/perf/bench/mem-memcpy.c
index 02dad5d..bccb783 100644
--- a/tools/perf/bench/mem-memcpy.c
+++ b/tools/perf/bench/mem-memcpy.c
@@ -144,17 +144,19 @@ static double do_memcpy_gettimeofday(memcpy_t
fn, size_t len, bool prefault)
 {
 	struct timeval tv_start, tv_end, tv_diff;
 	void *src = NULL, *dst = NULL;
-	int i;
+	int i, ret;

 	alloc_mem(&src, &dst, len);

 	if (prefault)
 		fn(dst, src, len);

-	BUG_ON(gettimeofday(&tv_start, NULL));
+	ret = gettimeofday(&tv_start, NULL);
+	BUG_ON(ret);
 	for (i = 0; i < iterations; ++i)
 		fn(dst, src, len);
-	BUG_ON(gettimeofday(&tv_end, NULL));
+	ret = gettimeofday(&tv_end, NULL);
+	BUG_ON(ret);

 	timersub(&tv_end, &tv_start, &tv_diff);

diff --git a/tools/perf/bench/mem-memset.c b/tools/perf/bench/mem-memset.c
index 350cc95..e0702d2 100644
--- a/tools/perf/bench/mem-memset.c
+++ b/tools/perf/bench/mem-memset.c
@@ -139,17 +139,19 @@ static double do_memset_gettimeofday(memset_t
fn, size_t len, bool prefault)
 {
 	struct timeval tv_start, tv_end, tv_diff;
 	void *dst = NULL;
-	int i;
+	int i, ret;

 	alloc_mem(&dst, len);

 	if (prefault)
 		fn(dst, -1, len);

-	BUG_ON(gettimeofday(&tv_start, NULL));
+	ret = gettimeofday(&tv_start, NULL);
+	BUG_ON(ret);
 	for (i = 0; i < iterations; ++i)
 		fn(dst, i, len);
-	BUG_ON(gettimeofday(&tv_end, NULL));
+	ret = gettimeofday(&tv_end, NULL);
+	BUG_ON(ret);

 	timersub(&tv_end, &tv_start, &tv_diff);

diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
index 0c7454f..b35c94b 100644
--- a/tools/perf/bench/sched-pipe.c
+++ b/tools/perf/bench/sched-pipe.c
@@ -61,8 +61,10 @@ int bench_sched_pipe(int argc, const char **argv,
 	argc = parse_options(argc, argv, options,
 			     bench_sched_pipe_usage, 0);

-	assert(!pipe(pipe_1));
-	assert(!pipe(pipe_2));
+	ret = pipe(pipe_1);
+	assert(!ret);
+	ret = !pipe(pipe_2);
+	assert(!ret);

 	pid = fork();
 	assert(pid >= 0);
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf bench: fix assert when NDEBUG is defined
  2012-09-03  0:04 [PATCH] perf bench: fix assert when NDEBUG is defined Irina Tirdea
@ 2012-09-03  1:45 ` Namhyung Kim
  2012-09-03  5:21   ` Pekka Enberg
  2012-09-05 11:00 ` Peter Zijlstra
  1 sibling, 1 reply; 5+ messages in thread
From: Namhyung Kim @ 2012-09-03  1:45 UTC (permalink / raw)
  To: Irina Tirdea
  Cc: Steven Rostedt, Arnaldo Carvalho de Melo, Ingo Molnar, LKML,
	Peter Zijlstra, Frederic Weisbecker

On Mon, 3 Sep 2012 03:04:32 +0300, Irina Tirdea wrote:
> From: Irina Tirdea <irina.tirdea@intel.com>
>
> When NDEBUG is defined, the assert macro will be expanded to nothing.
> Some assert calls used in perf are also including some functionality
> (e.g. system calls), not only validity checks. Therefore, if NDEBUG is
> defined, these functionality will be removed along with the assert.
>
> The functionality of the program needs to be separated from the assert checks.
> In perf, BUG_ON is also defined on assert, so we need to fix these statements
> too.
>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
> ---
>  tools/perf/bench/mem-memcpy.c |    8 +++++---
>  tools/perf/bench/mem-memset.c |    8 +++++---
>  tools/perf/bench/sched-pipe.c |    6 ++++--
>  3 files changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/tools/perf/bench/mem-memcpy.c b/tools/perf/bench/mem-memcpy.c
> index 02dad5d..bccb783 100644
> --- a/tools/perf/bench/mem-memcpy.c
> +++ b/tools/perf/bench/mem-memcpy.c
> @@ -144,17 +144,19 @@ static double do_memcpy_gettimeofday(memcpy_t
> fn, size_t len, bool prefault)
>  {
>  	struct timeval tv_start, tv_end, tv_diff;
>  	void *src = NULL, *dst = NULL;
> -	int i;
> +	int i, ret;
>
>  	alloc_mem(&src, &dst, len);
>
>  	if (prefault)
>  		fn(dst, src, len);
>
> -	BUG_ON(gettimeofday(&tv_start, NULL));
> +	ret = gettimeofday(&tv_start, NULL);
> +	BUG_ON(ret);

I think one of good thing of assert is that it outputs the exact failure
condition when it fails.  So with patch, it will convert

  Assertion `gettimeofday(&tv_start, NULL)' failed.

into

  Assertion `ret' failed.

which is not so informative.

So I'd rather suggest using more descriptive names like ret_gtod ?


>  	for (i = 0; i < iterations; ++i)
>  		fn(dst, src, len);
> -	BUG_ON(gettimeofday(&tv_end, NULL));
> +	ret = gettimeofday(&tv_end, NULL);
> +	BUG_ON(ret);
>
>  	timersub(&tv_end, &tv_start, &tv_diff);
>
> diff --git a/tools/perf/bench/mem-memset.c b/tools/perf/bench/mem-memset.c
> index 350cc95..e0702d2 100644
> --- a/tools/perf/bench/mem-memset.c
> +++ b/tools/perf/bench/mem-memset.c
> @@ -139,17 +139,19 @@ static double do_memset_gettimeofday(memset_t
> fn, size_t len, bool prefault)
>  {
>  	struct timeval tv_start, tv_end, tv_diff;
>  	void *dst = NULL;
> -	int i;
> +	int i, ret;
>
>  	alloc_mem(&dst, len);
>
>  	if (prefault)
>  		fn(dst, -1, len);
>
> -	BUG_ON(gettimeofday(&tv_start, NULL));
> +	ret = gettimeofday(&tv_start, NULL);
> +	BUG_ON(ret);
>  	for (i = 0; i < iterations; ++i)
>  		fn(dst, i, len);
> -	BUG_ON(gettimeofday(&tv_end, NULL));
> +	ret = gettimeofday(&tv_end, NULL);
> +	BUG_ON(ret);

Ditto.


>
>  	timersub(&tv_end, &tv_start, &tv_diff);
>
> diff --git a/tools/perf/bench/sched-pipe.c b/tools/perf/bench/sched-pipe.c
> index 0c7454f..b35c94b 100644
> --- a/tools/perf/bench/sched-pipe.c
> +++ b/tools/perf/bench/sched-pipe.c
> @@ -61,8 +61,10 @@ int bench_sched_pipe(int argc, const char **argv,
>  	argc = parse_options(argc, argv, options,
>  			     bench_sched_pipe_usage, 0);
>
> -	assert(!pipe(pipe_1));
> -	assert(!pipe(pipe_2));
> +	ret = pipe(pipe_1);
> +	assert(!ret);
> +	ret = !pipe(pipe_2);
> +	assert(!ret);

What about converting these raw assert's to BUG_ONs (with negating the
conditions) for consistency?

Thanks,
Namhyung


>
>  	pid = fork();
>  	assert(pid >= 0);

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf bench: fix assert when NDEBUG is defined
  2012-09-03  1:45 ` Namhyung Kim
@ 2012-09-03  5:21   ` Pekka Enberg
  0 siblings, 0 replies; 5+ messages in thread
From: Pekka Enberg @ 2012-09-03  5:21 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Irina Tirdea, Steven Rostedt, Arnaldo Carvalho de Melo,
	Ingo Molnar, LKML, Peter Zijlstra, Frederic Weisbecker

On Mon, Sep 3, 2012 at 4:45 AM, Namhyung Kim <namhyung@kernel.org> wrote:
> On Mon, 3 Sep 2012 03:04:32 +0300, Irina Tirdea wrote:
>> From: Irina Tirdea <irina.tirdea@intel.com>
>>
>> When NDEBUG is defined, the assert macro will be expanded to nothing.
>> Some assert calls used in perf are also including some functionality
>> (e.g. system calls), not only validity checks. Therefore, if NDEBUG is
>> defined, these functionality will be removed along with the assert.
>>
>> The functionality of the program needs to be separated from the assert checks.
>> In perf, BUG_ON is also defined on assert, so we need to fix these statements
>> too.
>>
>> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
>> ---
>>  tools/perf/bench/mem-memcpy.c |    8 +++++---
>>  tools/perf/bench/mem-memset.c |    8 +++++---
>>  tools/perf/bench/sched-pipe.c |    6 ++++--
>>  3 files changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/perf/bench/mem-memcpy.c b/tools/perf/bench/mem-memcpy.c
>> index 02dad5d..bccb783 100644
>> --- a/tools/perf/bench/mem-memcpy.c
>> +++ b/tools/perf/bench/mem-memcpy.c
>> @@ -144,17 +144,19 @@ static double do_memcpy_gettimeofday(memcpy_t
>> fn, size_t len, bool prefault)
>>  {
>>       struct timeval tv_start, tv_end, tv_diff;
>>       void *src = NULL, *dst = NULL;
>> -     int i;
>> +     int i, ret;
>>
>>       alloc_mem(&src, &dst, len);
>>
>>       if (prefault)
>>               fn(dst, src, len);
>>
>> -     BUG_ON(gettimeofday(&tv_start, NULL));
>> +     ret = gettimeofday(&tv_start, NULL);
>> +     BUG_ON(ret);
>
> I think one of good thing of assert is that it outputs the exact failure
> condition when it fails.  So with patch, it will convert
>
>   Assertion `gettimeofday(&tv_start, NULL)' failed.
>
> into
>
>   Assertion `ret' failed.
>
> which is not so informative.
>
> So I'd rather suggest using more descriptive names like ret_gtod ?

No, please don't do that. That'll make the code ugly and it's really
just papering over the fact that the assertions should be converted to
proper error handling.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf bench: fix assert when NDEBUG is defined
  2012-09-03  0:04 [PATCH] perf bench: fix assert when NDEBUG is defined Irina Tirdea
  2012-09-03  1:45 ` Namhyung Kim
@ 2012-09-05 11:00 ` Peter Zijlstra
  2012-09-08  1:24   ` Irina Tirdea
  1 sibling, 1 reply; 5+ messages in thread
From: Peter Zijlstra @ 2012-09-05 11:00 UTC (permalink / raw)
  To: Irina Tirdea
  Cc: Steven Rostedt, Arnaldo Carvalho de Melo, Ingo Molnar, LKML,
	Namhyung Kim, Frederic Weisbecker

On Mon, 2012-09-03 at 03:04 +0300, Irina Tirdea wrote:
> -       BUG_ON(gettimeofday(&tv_start, NULL));
> +       ret = gettimeofday(&tv_start, NULL);
> +       BUG_ON(ret); 

Its valid (although admittedly dubious) to have BUG_ON with
side-effects.

The 'right' fix would be something like:

---
 tools/perf/util/include/linux/kernel.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/perf/util/include/linux/kernel.h b/tools/perf/util/include/linux/kernel.h
index b6842c1..a5efd924 100644
--- a/tools/perf/util/include/linux/kernel.h
+++ b/tools/perf/util/include/linux/kernel.h
@@ -47,8 +47,12 @@
 #endif
 
 #ifndef BUG_ON
+#ifdef NDEBUG
+#define BUG_ON(cond) do { if (cond) ; } while (0)
+#else
 #define BUG_ON(cond) assert(!(cond))
 #endif
+#endif
 
 /*
  * Both need more care to handle endianness



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] perf bench: fix assert when NDEBUG is defined
  2012-09-05 11:00 ` Peter Zijlstra
@ 2012-09-08  1:24   ` Irina Tirdea
  0 siblings, 0 replies; 5+ messages in thread
From: Irina Tirdea @ 2012-09-08  1:24 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Steven Rostedt, Arnaldo Carvalho de Melo, Ingo Molnar, LKML,
	Namhyung Kim, Frederic Weisbecker

> Its valid (although admittedly dubious) to have BUG_ON with
> side-effects.
>
> The 'right' fix would be something like:
>
> ---
>  #ifndef BUG_ON
> +#ifdef NDEBUG
> +#define BUG_ON(cond) do { if (cond) ; } while (0)
> +#else
>  #define BUG_ON(cond) assert(!(cond))
>  #endif
> +#endif
>

This is indeed the right fix. I will use this approach, test it and resubmit.

Thanks,
Irina

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-09-08  1:24 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-03  0:04 [PATCH] perf bench: fix assert when NDEBUG is defined Irina Tirdea
2012-09-03  1:45 ` Namhyung Kim
2012-09-03  5:21   ` Pekka Enberg
2012-09-05 11:00 ` Peter Zijlstra
2012-09-08  1:24   ` Irina Tirdea

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).