Re: [RFC PATCH v7 1/7] Restartable sequences system call

[Mathieu Desnoyers <mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org>]

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