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

On Tue, Aug 09, 2016 at 08:06:40PM +0000, Mathieu Desnoyers wrote:

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

+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).