On 11/21/2017 10:05 AM, Mathieu Desnoyers wrote: > ----- On Nov 21, 2017, at 10:34 AM, shuah [email protected] 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
