----- On Aug 9, 2016, at 5:33 PM, Peter Zijlstra peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org wrote:
On Tue, Aug 09, 2016 at 08:06:40PM +0000, Mathieu Desnoyers wrote:
quoted
quoted
quoted
+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;
+}
quoted
quoted
quoted
+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