----- On Aug 9, 2016, at 5:33 PM, Peter Zijlstra peterz@xxxxxxxxxxxxx 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 -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html