lkml.org 
[lkml]   [2017]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests
    From
    Date
    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

    \
     
     \ /
      Last update: 2017-11-21 18:42    [W:4.596 / U:0.376 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site