Re: [PATCH 2/3] perf bench: Add new files for futex performance test

[Darren Hart <dvhltc@us.ibm.com>]

Hitoshi Mitake wrote:
> This patch adds two new files.
> 

Hi Hitoshi-san,

 > futextest.h provides general wrappers for futex() system call.
 > This patch containts the line:
 > typedef volatile u_int32_t futex_t;
 > I know that new typedef is not a thing to welcome,
 > but this is useful thing.
 >
 > futex-wait.c is a new suite to test performance of FUTEX_WAIT.
 >
 > These files are from Darren Hart's futex test,
 > and futex-wait.c is based on the program originally
 > written by Michel Lespinasse.

Thanks for looking at getting the futex performance tests into perf. 
Just wanted to make sure you are aware that there will likely be several 
more futextest/performance tests in the near future as the project is 
under early active development. I see you have merged harness.h into 
futex-wait.c, which is fine, but I will likely be adding a new 
include/locking.h to add things like barrier, lock, and lock_pi locking 
primitives to the futextest testsuite. futex-wait.c would then be 
updated to use those directly if feasible and remove the custom locking 
primitives in the test itself. I mention this so you are aware and perf 
futex benchmarks don't get too far out of sync with futextest.

I'd be interested in any ideas you have to make futextest/performance/* 
tests integrate more easily into perf as I'd like to include each new 
test into perf as well.

Couple of nits below:


> diff --git a/tools/perf/bench/futextest.h b/tools/perf/bench/futextest.h
> new file mode 100644
> index 0000000..09d8b94
> --- /dev/null
> +++ b/tools/perf/bench/futextest.h
> @@ -0,0 +1,280 @@
> +/******************************************************************************
> + *
> + *   Copyright B) International Business Machines  Corp., 2009


Copyright character issue...

> + * HISTORY
> + *      2009-Nov-24:
> + *       Ported to perf by Hitoshi Mitake <mitake@dcl.info.waseda.ac.jp>
> + *      2009-Nov-06: Initial version by Darren Hart <dvhltc@us.ibm.com>

I usually add the dates in ascending chronological order.

> + *
> + *****************************************************************************/
> +
> +#ifndef _FUTEXTEST_H
> +#define _FUTEXTEST_H
> +
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +#include <sys/types.h>
> +#include <linux/futex.h>
> +
> +typedef volatile u_int32_t futex_t;

I have been considering making this into a val wrapped in a struct like 
the atomic_t as that will make adding things like flags to the futex_t 
easier. Again, just a head's up that it may be changing in the near future.

Thanks,

-- 
Darren Hart
IBM Linux Technology Center
Real-Time Linux Team