From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751500AbdKUVYg (ORCPT ); Tue, 21 Nov 2017 16:24:36 -0500 Received: from osg.samsung.com ([64.30.133.232]:50834 "EHLO osg.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751145AbdKUVYf (ORCPT ); Tue, 21 Nov 2017 16:24:35 -0500 Subject: Re: [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests To: Mathieu Desnoyers , 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 , Shuah Khan References: <20171121141900.18471-1-mathieu.desnoyers@efficios.com> <20171121141900.18471-16-mathieu.desnoyers@efficios.com> <1823512285.19395.1511283912405.JavaMail.zimbra@efficios.com> <5b5679fc-a807-8d14-85d0-41a1dd34214e@kernel.org> <911090321.19621.1511299340968.JavaMail.zimbra@efficios.com> From: Shuah Khan Message-ID: Date: Tue, 21 Nov 2017 14:24:30 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <911090321.19621.1511299340968.JavaMail.zimbra@efficios.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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/?h=rseq/dev&id=a0b8eb0eb5d4d8a280969370aa1dcf51801139c6 > "selftests: lib.mk: Introduce OVERRIDE_TARGETS" > > https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=4ef0214e19bb7415fe7aed6852859b8d66e09a45 > "cpu_opv: selftests: Implement selftests (v4)" > > https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=7d7530b843c7ecb50bea5a136c776cf3e9155d43 > "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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: shuahkh at osg.samsung.com (Shuah Khan) Date: Tue, 21 Nov 2017 14:24:30 -0700 Subject: [Linux-kselftest-mirror] [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests In-Reply-To: <911090321.19621.1511299340968.JavaMail.zimbra@efficios.com> References: <20171121141900.18471-1-mathieu.desnoyers@efficios.com> <20171121141900.18471-16-mathieu.desnoyers@efficios.com> <1823512285.19395.1511283912405.JavaMail.zimbra@efficios.com> <5b5679fc-a807-8d14-85d0-41a1dd34214e@kernel.org> <911090321.19621.1511299340968.JavaMail.zimbra@efficios.com> Message-ID: On 11/21/2017 02:22 PM, Mathieu Desnoyers wrote: > ----- On Nov 21, 2017, at 12:40 PM, shuah shuah at kernel.org wrote: > >> On 11/21/2017 10:05 AM, Mathieu Desnoyers wrote: >>> ----- On Nov 21, 2017, at 10:34 AM, shuah shuah at 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/?h=rseq/dev&id=a0b8eb0eb5d4d8a280969370aa1dcf51801139c6 > "selftests: lib.mk: Introduce OVERRIDE_TARGETS" > > https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=4ef0214e19bb7415fe7aed6852859b8d66e09a45 > "cpu_opv: selftests: Implement selftests (v4)" > > https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=7d7530b843c7ecb50bea5a136c776cf3e9155d43 > "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 at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: shuahkh@osg.samsung.com (Shuah Khan) Date: Tue, 21 Nov 2017 14:24:30 -0700 Subject: [Linux-kselftest-mirror] [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests In-Reply-To: <911090321.19621.1511299340968.JavaMail.zimbra@efficios.com> References: <20171121141900.18471-1-mathieu.desnoyers@efficios.com> <20171121141900.18471-16-mathieu.desnoyers@efficios.com> <1823512285.19395.1511283912405.JavaMail.zimbra@efficios.com> <5b5679fc-a807-8d14-85d0-41a1dd34214e@kernel.org> <911090321.19621.1511299340968.JavaMail.zimbra@efficios.com> Message-ID: Content-Type: text/plain; charset="UTF-8" Message-ID: <20171121212430.P7E9bI4iDTNQpN-IUP-qBIB8gDE4an3gmLNhFiVuVi4@z> On 11/21/2017 02:22 PM, Mathieu Desnoyers wrote: > ----- On Nov 21, 2017,@12:40 PM, shuah shuah@kernel.org wrote: > >> On 11/21/2017 10:05 AM, Mathieu Desnoyers wrote: >>> ----- On Nov 21, 2017,@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/?h=rseq/dev&id=a0b8eb0eb5d4d8a280969370aa1dcf51801139c6 > "selftests: lib.mk: Introduce OVERRIDE_TARGETS" > > https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=4ef0214e19bb7415fe7aed6852859b8d66e09a45 > "cpu_opv: selftests: Implement selftests (v4)" > > https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=7d7530b843c7ecb50bea5a136c776cf3e9155d43 > "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 at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shuah Khan Subject: Re: [RFC PATCH for 4.15 v3 15/22] rseq: selftests: Provide self-tests Date: Tue, 21 Nov 2017 14:24:30 -0700 Message-ID: References: <20171121141900.18471-1-mathieu.desnoyers@efficios.com> <20171121141900.18471-16-mathieu.desnoyers@efficios.com> <1823512285.19395.1511283912405.JavaMail.zimbra@efficios.com> <5b5679fc-a807-8d14-85d0-41a1dd34214e@kernel.org> <911090321.19621.1511299340968.JavaMail.zimbra@efficios.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <911090321.19621.1511299340968.JavaMail.zimbra-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org> Content-Language: en-US Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Mathieu Desnoyers , 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 List-Id: linux-api@vger.kernel.org On 11/21/2017 02:22 PM, Mathieu Desnoyers wrote: > ----- On Nov 21, 2017, at 12:40 PM, shuah shuah-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org wrote: > >> On 11/21/2017 10:05 AM, Mathieu Desnoyers wrote: >>> ----- On Nov 21, 2017, at 10:34 AM, shuah shuah-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.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/?h=rseq/dev&id=a0b8eb0eb5d4d8a280969370aa1dcf51801139c6 > "selftests: lib.mk: Introduce OVERRIDE_TARGETS" > > https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=4ef0214e19bb7415fe7aed6852859b8d66e09a45 > "cpu_opv: selftests: Implement selftests (v4)" > > https://git.kernel.org/pub/scm/linux/kernel/git/rseq/linux-rseq.git/commit/?h=rseq/dev&id=7d7530b843c7ecb50bea5a136c776cf3e9155d43 > "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