From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751356AbdKUREL (ORCPT ); Tue, 21 Nov 2017 12:04:11 -0500 Received: from mail.efficios.com ([167.114.142.141]:59012 "EHLO mail.efficios.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751191AbdKUREJ (ORCPT ); Tue, 21 Nov 2017 12:04:09 -0500 Date: Tue, 21 Nov 2017 17:05:12 +0000 (UTC) From: Mathieu Desnoyers To: shuah Cc: Peter Zijlstra , "Paul E. McKenney" , Boqun Feng , Andy Lutomirski , Dave Watson , linux-kernel , linux-api , Paul Turner , Andrew Morton , Russell King , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , Andrew Hunter , Andi Kleen , Chris Lameter , Ben Maurer , rostedt , Josh Triplett , Linus Torvalds , Catalin Marinas , Will Deacon , Michael Kerrisk , linux-kselftest , Shuah Khan Message-ID: <1823512285.19395.1511283912405.JavaMail.zimbra@efficios.com> In-Reply-To: References: <20171121141900.18471-1-mathieu.desnoyers@efficios.com> <20171121141900.18471-16-mathieu.desnoyers@efficios.com> Subject: Re: [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [167.114.142.141] X-Mailer: Zimbra 8.7.11_GA_1854 (ZimbraWebClient - FF52 (Linux)/8.7.11_GA_1854) Thread-Topic: rseq: selftests: Provide self-tests Thread-Index: nZqIn4RqLBXE7qrne69p/PhEziF0GA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- 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