Re: [Linux-kselftest-mirror] [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests

List overview All Threads
Download

newer

older

[Linux-kselftest-mirror] [PATCH...

[Linux-kselftest-mirror] [PATCH]...

Mathieu Desnoyers

21 Nov 2017 21 Nov '17

5:05 p.m.

----- On Nov 21, 2017, at 10:34 AM, shuah shuah@kernel.org wrote:

[...]

...

...

MAINTAINERS | 1 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/rseq/.gitignore | 4 +

Thanks for the .gitignore files. It is commonly missed change, I end up adding one to clean things up after tests get in.

I'm used to receive patches where contributors forget to add new files to gitignore within my own projects, which may contribute to my awareness of this pain point. :)

[...]

...

...
+void *test_percpu_inc_thread(void *arg) +{
struct inc_thread_test_data *thread_data = arg;

struct inc_test_data *data = thread_data->data;

long long i, reps;

if (!opt_disable_rseq && thread_data->reg
	&& rseq_register_current_thread())
abort();
reps = thread_data->reps;

for (i = 0; i < reps; i++) {
int cpu, ret;
+#ifndef SKIP_FASTPATH
/* Try fast path. */
cpu = rseq_cpu_start();
ret = rseq_addv(&data->c[cpu].count, 1, cpu);
if (likely(!ret))
	goto next;
+#endif
So the test needs to compiled with this enabled? I think it would be better to make this an argument to be abel to select at test start time as opposed to making this compile time option. Remember that these tests get run in automated test rings. Making this a compile time otpion pertty much ensures that this path will not be tested.

So I would reccommend adding a paratemer.

...
slowpath:
__attribute__((unused));
for (;;) {
	/* Fallback on cpu_opv system call. */
	cpu = rseq_current_cpu();
	ret = cpu_op_addv(&data->c[cpu].count, 1, cpu);
	if (likely(!ret))
		break;
	assert(ret >= 0 || errno == EAGAIN);
}
next:
__attribute__((unused));
+#ifndef BENCHMARK
if (i != 0 && !(i % (reps / 10)))
	printf_verbose("tid %d: count %lld\n", (int) gettid(), i);
+#endif
Same comment as before. Avoid compile time options.

The goal of those compiler define are to generate the altered code without adding branches into the fast-paths.

Here is an alternative solution that should take care of your concern: I'll build multiple targets for param_test.c:

param_test param_test_skip_fastpath (built with -DSKIP_FASTPATH) param_test_benchmark (build with -DBENCHMARK)

I'll update run_param_test.sh to run both param_test and param_test_skip_fastpath.

Note that "param_test_benchmark" is only useful for benchmarking, so I don't plan to run it from run_param_test.sh which is meant to track regressions.

Is that approach OK with you ?

Thanks,

Mathieu

-- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com -- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Show replies by date

Shuah Khan

21 Nov 21 Nov

5:40 p.m.

New subject: [Linux-kselftest-mirror] [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests

On 11/21/2017 10:05 AM, Mathieu Desnoyers wrote:

...

----- On Nov 21, 2017, at 10:34 AM, shuah shuah@kernel.org wrote:

[...]

...
...

MAINTAINERS | 1 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/rseq/.gitignore | 4 +

Thanks for the .gitignore files. It is commonly missed change, I end up adding one to clean things up after tests get in.

I'm used to receive patches where contributors forget to add new files to gitignore within my own projects, which may contribute to my awareness of this pain point. :)

[...]

...
...
+void *test_percpu_inc_thread(void *arg) +{
struct inc_thread_test_data *thread_data = arg;

struct inc_test_data *data = thread_data->data;

long long i, reps;

if (!opt_disable_rseq && thread_data->reg
	&& rseq_register_current_thread())
abort();
reps = thread_data->reps;

for (i = 0; i < reps; i++) {
int cpu, ret;
+#ifndef SKIP_FASTPATH
/* Try fast path. */
cpu = rseq_cpu_start();
ret = rseq_addv(&data->c[cpu].count, 1, cpu);
if (likely(!ret))
	goto next;
+#endif
So the test needs to compiled with this enabled? I think it would be better to make this an argument to be abel to select at test start time as opposed to making this compile time option. Remember that these tests get run in automated test rings. Making this a compile time otpion pertty much ensures that this path will not be tested.

So I would reccommend adding a paratemer.

...
slowpath:
__attribute__((unused));
for (;;) {
	/* Fallback on cpu_opv system call. */
	cpu = rseq_current_cpu();
	ret = cpu_op_addv(&data->c[cpu].count, 1, cpu);
	if (likely(!ret))
		break;
	assert(ret >= 0 || errno == EAGAIN);
}
next:
__attribute__((unused));
+#ifndef BENCHMARK
if (i != 0 && !(i % (reps / 10)))
	printf_verbose("tid %d: count %lld\n", (int) gettid(), i);
+#endif
Same comment as before. Avoid compile time options.
The goal of those compiler define are to generate the altered code without adding branches into the fast-paths.

That makes sense. You are looking to not add any overhead.

...

Here is an alternative solution that should take care of your concern: I'll build multiple targets for param_test.c:

param_test param_test_skip_fastpath (built with -DSKIP_FASTPATH) param_test_benchmark (build with -DBENCHMARK)

I'll update run_param_test.sh to run both param_test and param_test_skip_fastpath.

Note that "param_test_benchmark" is only useful for benchmarking, so I don't plan to run it from run_param_test.sh which is meant to track regressions.

Is that approach OK with you ?

Yes. This approach addresses my concern about coverage for both paths.

thanks, -- Shuah

-- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Mathieu Desnoyers

9:22 p.m.

New subject: [Linux-kselftest-mirror] [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests

----- On Nov 21, 2017, at 12:40 PM, shuah shuah@kernel.org wrote:

...

On 11/21/2017 10:05 AM, Mathieu Desnoyers wrote:

...
----- On Nov 21, 2017, at 10:34 AM, shuah shuah@kernel.org wrote:

[...]

...
...

MAINTAINERS | 1 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/rseq/.gitignore | 4 +

Thanks for the .gitignore files. It is commonly missed change, I end up adding one to clean things up after tests get in.

I'm used to receive patches where contributors forget to add new files to gitignore within my own projects, which may contribute to my awareness of this pain point. :)

[...]

...
...
+void *test_percpu_inc_thread(void *arg) +{
struct inc_thread_test_data *thread_data = arg;

struct inc_test_data *data = thread_data->data;

long long i, reps;

if (!opt_disable_rseq && thread_data->reg
	&& rseq_register_current_thread())
abort();
reps = thread_data->reps;

for (i = 0; i < reps; i++) {
int cpu, ret;
+#ifndef SKIP_FASTPATH
/* Try fast path. */
cpu = rseq_cpu_start();
ret = rseq_addv(&data->c[cpu].count, 1, cpu);
if (likely(!ret))
	goto next;
+#endif
So the test needs to compiled with this enabled? I think it would be better to make this an argument to be abel to select at test start time as opposed to making this compile time option. Remember that these tests get run in automated test rings. Making this a compile time otpion pertty much ensures that this path will not be tested.

So I would reccommend adding a paratemer.

...
slowpath:
__attribute__((unused));
for (;;) {
	/* Fallback on cpu_opv system call. */
	cpu = rseq_current_cpu();
	ret = cpu_op_addv(&data->c[cpu].count, 1, cpu);
	if (likely(!ret))
		break;
	assert(ret >= 0 || errno == EAGAIN);
}
next:
__attribute__((unused));
+#ifndef BENCHMARK
if (i != 0 && !(i % (reps / 10)))
	printf_verbose("tid %d: count %lld\n", (int) gettid(), i);
+#endif
Same comment as before. Avoid compile time options.
The goal of those compiler define are to generate the altered code without adding branches into the fast-paths.
That makes sense. You are looking to not add any overhead.

...
Here is an alternative solution that should take care of your concern: I'll build multiple targets for param_test.c:

param_test param_test_skip_fastpath (built with -DSKIP_FASTPATH) param_test_benchmark (build with -DBENCHMARK)

I'll update run_param_test.sh to run both param_test and param_test_skip_fastpath.

Note that "param_test_benchmark" is only useful for benchmarking, so I don't plan to run it from run_param_test.sh which is meant to track regressions.

Is that approach OK with you ?

Yes. This approach addresses my concern about coverage for both paths.

fyi, the updated patches can be found here:

https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?... "selftests: lib.mk: Introduce OVERRIDE_TARGETS"

https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?... "cpu_opv: selftests: Implement selftests (v4)"

https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?... "rseq: selftests: Provide self-tests (v4)"

Thanks for the feedback!

Mathieu

...

thanks, -- Shuah

Shuah Khan

9:24 p.m.

New subject: [Linux-kselftest-mirror] [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests

On 11/21/2017 02:22 PM, Mathieu Desnoyers wrote:

...

----- On Nov 21, 2017, at 12:40 PM, shuah shuah@kernel.org wrote:

...
On 11/21/2017 10:05 AM, Mathieu Desnoyers wrote:

...
----- On Nov 21, 2017, at 10:34 AM, shuah shuah@kernel.org wrote:

[...]

...
...

MAINTAINERS | 1 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/rseq/.gitignore | 4 +

Thanks for the .gitignore files. It is commonly missed change, I end up adding one to clean things up after tests get in.

I'm used to receive patches where contributors forget to add new files to gitignore within my own projects, which may contribute to my awareness of this pain point. :)

[...]

...
...
+void *test_percpu_inc_thread(void *arg) +{
struct inc_thread_test_data *thread_data = arg;

struct inc_test_data *data = thread_data->data;

long long i, reps;

if (!opt_disable_rseq && thread_data->reg
	&& rseq_register_current_thread())
abort();
reps = thread_data->reps;

for (i = 0; i < reps; i++) {
int cpu, ret;
+#ifndef SKIP_FASTPATH
/* Try fast path. */
cpu = rseq_cpu_start();
ret = rseq_addv(&data->c[cpu].count, 1, cpu);
if (likely(!ret))
	goto next;
+#endif
So the test needs to compiled with this enabled? I think it would be better to make this an argument to be abel to select at test start time as opposed to making this compile time option. Remember that these tests get run in automated test rings. Making this a compile time otpion pertty much ensures that this path will not be tested.

So I would reccommend adding a paratemer.

...
slowpath:
__attribute__((unused));
for (;;) {
	/* Fallback on cpu_opv system call. */
	cpu = rseq_current_cpu();
	ret = cpu_op_addv(&data->c[cpu].count, 1, cpu);
	if (likely(!ret))
		break;
	assert(ret >= 0 || errno == EAGAIN);
}
next:
__attribute__((unused));
+#ifndef BENCHMARK
if (i != 0 && !(i % (reps / 10)))
	printf_verbose("tid %d: count %lld\n", (int) gettid(), i);
+#endif
Same comment as before. Avoid compile time options.
The goal of those compiler define are to generate the altered code without adding branches into the fast-paths.
That makes sense. You are looking to not add any overhead.

...
Here is an alternative solution that should take care of your concern: I'll build multiple targets for param_test.c:

param_test param_test_skip_fastpath (built with -DSKIP_FASTPATH) param_test_benchmark (build with -DBENCHMARK)

I'll update run_param_test.sh to run both param_test and param_test_skip_fastpath.

Note that "param_test_benchmark" is only useful for benchmarking, so I don't plan to run it from run_param_test.sh which is meant to track regressions.

Is that approach OK with you ?

Yes. This approach addresses my concern about coverage for both paths.
fyi, the updated patches can be found here:

https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?... "selftests: lib.mk: Introduce OVERRIDE_TARGETS"

https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?... "cpu_opv: selftests: Implement selftests (v4)"

https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?... "rseq: selftests: Provide self-tests (v4)"

Thanks for the feedback!

Are you going to send these to the mailing list? That way I can do a final review and give my Ack if they look good.

thanks, -- Shuah

-- To unsubscribe from this list: send the line "unsubscribe linux-kselftest" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Mathieu Desnoyers

9:44 p.m.

New subject: [Linux-kselftest-mirror] [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests

----- On Nov 21, 2017, at 4:24 PM, Shuah Khan shuahkh@osg.samsung.com wrote:

...

On 11/21/2017 02:22 PM, Mathieu Desnoyers wrote:

...
----- On Nov 21, 2017, at 12:40 PM, shuah shuah@kernel.org wrote:

...
On 11/21/2017 10:05 AM, Mathieu Desnoyers wrote:

...
----- On Nov 21, 2017, at 10:34 AM, shuah shuah@kernel.org wrote:

[...]

...
...

MAINTAINERS | 1 + tools/testing/selftests/Makefile | 1 + tools/testing/selftests/rseq/.gitignore | 4 +

Thanks for the .gitignore files. It is commonly missed change, I end up adding one to clean things up after tests get in.

I'm used to receive patches where contributors forget to add new files to gitignore within my own projects, which may contribute to my awareness of this pain point. :)

[...]

...
...
+void *test_percpu_inc_thread(void *arg) +{
struct inc_thread_test_data *thread_data = arg;

struct inc_test_data *data = thread_data->data;

long long i, reps;

if (!opt_disable_rseq && thread_data->reg
	&& rseq_register_current_thread())
abort();
reps = thread_data->reps;

for (i = 0; i < reps; i++) {
int cpu, ret;
+#ifndef SKIP_FASTPATH
/* Try fast path. */
cpu = rseq_cpu_start();
ret = rseq_addv(&data->c[cpu].count, 1, cpu);
if (likely(!ret))
	goto next;
+#endif
So the test needs to compiled with this enabled? I think it would be better to make this an argument to be abel to select at test start time as opposed to making this compile time option. Remember that these tests get run in automated test rings. Making this a compile time otpion pertty much ensures that this path will not be tested.

So I would reccommend adding a paratemer.

...
slowpath:
__attribute__((unused));
for (;;) {
	/* Fallback on cpu_opv system call. */
	cpu = rseq_current_cpu();
	ret = cpu_op_addv(&data->c[cpu].count, 1, cpu);
	if (likely(!ret))
		break;
	assert(ret >= 0 || errno == EAGAIN);
}
next:
__attribute__((unused));
+#ifndef BENCHMARK
if (i != 0 && !(i % (reps / 10)))
	printf_verbose("tid %d: count %lld\n", (int) gettid(), i);
+#endif
Same comment as before. Avoid compile time options.
The goal of those compiler define are to generate the altered code without adding branches into the fast-paths.
That makes sense. You are looking to not add any overhead.

...
Here is an alternative solution that should take care of your concern: I'll build multiple targets for param_test.c:

param_test param_test_skip_fastpath (built with -DSKIP_FASTPATH) param_test_benchmark (build with -DBENCHMARK)

I'll update run_param_test.sh to run both param_test and param_test_skip_fastpath.

Note that "param_test_benchmark" is only useful for benchmarking, so I don't plan to run it from run_param_test.sh which is meant to track regressions.

Is that approach OK with you ?

Yes. This approach addresses my concern about coverage for both paths.
fyi, the updated patches can be found here:

https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?... "selftests: lib.mk: Introduce OVERRIDE_TARGETS"

https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?... "cpu_opv: selftests: Implement selftests (v4)"

https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?... "rseq: selftests: Provide self-tests (v4)"

Thanks for the feedback!
Are you going to send these to the mailing list? That way I can do a final review and give my Ack if they look good.

Sure, I can do one hopefully last round of RFC with those selftests updates.

Thanks,

Mathieu

2587

days inactive

2587

days old

linux-kselftest-mirror@lists.linaro.org

4 comments

participants

tags (0)

participants (3)

Mathieu Desnoyers
Shuah Khan
Shuah Khan