From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mathieu Desnoyers Subject: Re: [RFC PATCH v11 for 4.15 01/24] Restartable sequences system call Date: Tue, 14 Nov 2017 21:03:24 +0000 (UTC) Message-ID: <1641747353.15177.1510693404572.JavaMail.zimbra@efficios.com> References: <20171114200414.2188-1-mathieu.desnoyers@efficios.com> <20171114200414.2188-2-mathieu.desnoyers@efficios.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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 List-Id: linux-api@vger.kernel.org ----- On Nov 14, 2017, at 3:49 PM, Ben Maurer bmaurer-b10kYP2dOMg@public.gmane.org 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