----- 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
