From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756744AbdKNVCt (ORCPT ); Tue, 14 Nov 2017 16:02:49 -0500 Received: from mail.efficios.com ([167.114.142.141]:57951 "EHLO mail.efficios.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756615AbdKNVCl (ORCPT ); Tue, 14 Nov 2017 16:02:41 -0500 Date: Tue, 14 Nov 2017 21:03:24 +0000 (UTC) From: Mathieu Desnoyers To: Ben Maurer 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 , rostedt , Josh Triplett , Linus Torvalds , Catalin Marinas , Will Deacon , Michael Kerrisk , Alexander Viro Message-ID: <1641747353.15177.1510693404572.JavaMail.zimbra@efficios.com> In-Reply-To: References: <20171114200414.2188-1-mathieu.desnoyers@efficios.com> <20171114200414.2188-2-mathieu.desnoyers@efficios.com> Subject: Re: [RFC PATCH v11 for 4.15 01/24] Restartable sequences system call 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: Restartable sequences system call Thread-Index: AQHTXYPhmIqd6HY5PUe1T5SV8sIkp6MUVpZlgAAB7cASFntJhQ== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- On Nov 14, 2017, at 3:49 PM, Ben Maurer bmaurer@fb.com wrote: > (apologies for the duplicate email, the previous one bounced as it was > accidentally using HTML formatting) > > If I understand correctly this is run on every context switch so we probably > want to make it really fast Yes, more precisely, it runs on return to user-space, after every context switch going back to a registered rseq thread. > >> +static int rseq_need_restart(struct task_struct *t, uint32_t cs_flags) >> +{ >> + bool need_restart = false; >> + uint32_t flags; >> + >> + /* Get thread flags. */ >> + if (__get_user(flags, &t->rseq->flags)) >> + return -EFAULT; >> + >> + /* Take into account critical section flags. */ >> + flags |= cs_flags; >> + >> + /* >> + * Restart on signal can only be inhibited when restart on >> + * preempt and restart on migrate are inhibited too. Otherwise, >> + * a preempted signal handler could fail to restart the prior >> + * execution context on sigreturn. >> + */ >> + if (flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) { >> + if (!(flags & RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)) >> + return -EINVAL; >> + if (!(flags & RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)) >> + return -EINVAL; >> + } > > How does this error even get to userspace? Is it worth doing this switch on > every execution? If we detect this situation, the rseq_need_restart caller will end up sending a SIGSEGV signal to user-space. Note that the two nested if() checks are only executing in the unlikely case where the NO_RESTART_ON_SIGNAL flag is set. > > >> + if (t->rseq_migrate >> + && !(flags & RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE)) >> + need_restart = true; >> + else if (t->rseq_preempt >> + && !(flags & RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT)) >> + need_restart = true; >> + else if (t->rseq_signal >> + && !(flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL)) >> + need_restart = true; > > This could potentially be sped up by having the rseq_* fields in t use a single > bitmask with the same bit offsets as RSEQ_CS_FLAG_NO_* then using bit > operations to check the appropriate overlap. Given that those are not requests impacting the ABI presented to user-space, I'm tempted to keep these optimizations for the following 4.16 merge window. Is that ok with you ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com