From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932577AbcHIWl5 (ORCPT ); Tue, 9 Aug 2016 18:41:57 -0400 Received: from mail.efficios.com ([78.47.125.74]:49529 "EHLO mail.efficios.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932515AbcHIWly (ORCPT ); Tue, 9 Aug 2016 18:41:54 -0400 Date: Tue, 9 Aug 2016 22:41:47 +0000 (UTC) From: Mathieu Desnoyers To: Peter Zijlstra Cc: Andrew Morton , Russell King , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , linux-kernel , linux-api , Paul Turner , Andrew Hunter , Andy Lutomirski , Andi Kleen , Dave Watson , Chris Lameter , Ben Maurer , rostedt , "Paul E. McKenney" , Josh Triplett , Linus Torvalds , Catalin Marinas , Will Deacon , Michael Kerrisk , Boqun Feng Message-ID: <427989454.6732.1470782507345.JavaMail.zimbra@efficios.com> In-Reply-To: <20160809213314.GK30192@twins.programming.kicks-ass.net> References: <1469135662-31512-1-git-send-email-mathieu.desnoyers@efficios.com> <1469135662-31512-2-git-send-email-mathieu.desnoyers@efficios.com> <20160803131940.GM6862@twins.programming.kicks-ass.net> <656745027.6624.1470773200334.JavaMail.zimbra@efficios.com> <20160809213314.GK30192@twins.programming.kicks-ass.net> Subject: Re: [RFC PATCH v7 1/7] Restartable sequences system call MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [78.47.125.74] X-Mailer: Zimbra 8.7.0_GA_1659 (ZimbraWebClient - FF45 (Linux)/8.7.0_GA_1659) Thread-Topic: Restartable sequences system call Thread-Index: K5JH55ZYyg7FVeb/pLJUM7p5F2kOpg== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org ----- On Aug 9, 2016, at 5:33 PM, Peter Zijlstra peterz@infradead.org wrote: > On Tue, Aug 09, 2016 at 08:06:40PM +0000, Mathieu Desnoyers wrote: >> >> +static int rseq_increment_event_counter(struct task_struct *t) >> >> +{ >> >> + if (__put_user(++t->rseq_event_counter, >> >> + &t->rseq->u.e.event_counter)) >> >> + return -1; >> >> + return 0; >> >> +} > >> >> +void __rseq_handle_notify_resume(struct pt_regs *regs) >> >> +{ >> >> + struct task_struct *t = current; >> >> + >> >> + if (unlikely(t->flags & PF_EXITING)) >> >> + return; >> >> + if (!access_ok(VERIFY_WRITE, t->rseq, sizeof(*t->rseq))) >> >> + goto error; >> >> + if (__put_user(raw_smp_processor_id(), &t->rseq->u.e.cpu_id)) >> >> + goto error; >> >> + if (rseq_increment_event_counter(t)) >> > >> > It seems a shame to not use a single __put_user() here. You did the >> > layout to explicitly allow for this, but then you don't. >> >> The event counter increment needs to be performed at least once before >> returning to user-space whenever the thread is preempted or has a signal >> delivered. This counter increment needs to occur even if we are not nested >> over a restartable assembly block. (more detailed explanation about this >> follows at the end of this email) >> >> The rseq_ip_fixup only ever needs to update the rseq_cs pointer >> field if it preempts/delivers a signal over a restartable >> assembly block, which happens very rarely. >> >> Therefore, since the event counter increment is more frequent than >> setting rseq_cs ptr, I don't see much value in trying to combine >> those two into a single __put_user(). >> >> The reason why I combined both the cpu_id and event_counter >> fields into the same 64-bit integer is for user-space rseq_start() >> to be able to fetch them through a single load when the architecture >> allows it. > > I wasn't talking about the rseq_up_fixup(), I was talking about both > unconditional __put_user()'s on cpu_id and event_counter. > > These are 2 unconditinoal u32 stores that could very easily be done as a > single u64 store (on 64bit hardware). Gotcha. I'll therefore move the union outside of struct rseq in rseq.h so we can re-use it: union rseq_cpu_event { struct { /* * Restartable sequences cpu_id field. * Updated by the kernel, and read by user-space with * single-copy atomicity semantics. Aligned on 32-bit. * Negative values are reserved for user-space. */ int32_t cpu_id; /* * Restartable sequences event_counter field. * Updated by the kernel, and read by user-space with * single-copy atomicity semantics. Aligned on 32-bit. */ uint32_t event_counter; } e; /* * On architectures with 64-bit aligned reads, both cpu_id and * event_counter can be read with single-copy atomicity * semantics. */ uint64_t v; }; /* * struct rseq is aligned on 2 * 8 bytes to ensure it is always * contained within a single cache-line. */ struct rseq { union rseq_cpu_event u; /* * Restartable sequences rseq_cs field. * Contains NULL when no critical section is active for the * current thread, or holds a pointer to the currently active * struct rseq_cs. * Updated by user-space at the beginning and end of assembly * instruction sequence block, and by the kernel when it * restarts an assembly instruction sequence block. Read by the * kernel with single-copy atomicity semantics. Aligned on * 64-bit. */ RSEQ_FIELD_u32_u64(rseq_cs); } __attribute__((aligned(2 * sizeof(uint64_t)))); I'll replace the two updates by this call in __rseq_handle_notify_resume(): if (!rseq_update_cpu_id_event_counter(t)) goto error; And the given implementation: /* * The rseq_event_counter allow user-space to detect preemption and * signal delivery. It increments at least once before returning to * user-space if a thread is preempted or has a signal delivered. It is * not meant to be an exact counter of such events. * * Overflow of the event counter is not a problem in practice. It * increments at most once between each user-space thread instruction * executed, so we would need a thread to execute 2^32 instructions or * more between rseq_start() and rseq_finish(), while single-stepping, * for this to be an issue. * * On 64-bit architectures, both cpu_id and event_counter can be updated * with a single 64-bit store. On 32-bit architectures, we instead * perform two 32-bit single-copy stores, just in case the architecture * 64-bit __put_user() would fallback on a bytewise copy, which would * not guarantee single-copy atomicity semantics for other threads. */ #ifdef __LP64__ static bool rseq_update_cpu_id_event_counter(struct task_struct *t) { union rseq_cpu_event u; u.e.cpu_id = raw_smp_processor_id(); u.e.event_counter = ++t->rseq_event_counter; if (__put_user(u.v, &t->rseq->u.v)) return false; trace_rseq_inc(t->rseq_event_counter); return true; } #else /* #ifdef __LP64__ */ static bool rseq_update_cpu_id_event_counter(struct task_struct *t) { if (__put_user(raw_smp_processor_id(), &t->rseq->u.e.cpu_id)) return false; if (__put_user(++t->rseq_event_counter, &t->rseq->u.e.event_counter)) return false; trace_rseq_inc(t->rseq_event_counter); return true; } #endif /* #else #ifdef __LP64__ */ Let me know if I missed anything. Thanks! Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com