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

Darren Hart
2009-11-24T16:33:32Z

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