Re: [RFC PATCH v11 for 4.15 01/24] Restartable sequences system call

[Mathieu Desnoyers]

----- On Nov 16, 2017, at 11:32 AM, Peter Zijlstra peterz@xxxxxxxxxxxxx wrote:

> On Thu, Nov 16, 2017 at 04:27:07PM +0000, Mathieu Desnoyers wrote:
>> ----- On Nov 16, 2017, at 11:18 AM, Peter Zijlstra peterz@xxxxxxxxxxxxx wrote:
>> 
>> > On Tue, Nov 14, 2017 at 03:03:51PM -0500, Mathieu Desnoyers wrote:
>> >> @@ -977,6 +978,13 @@ struct task_struct {
>> >>  	unsigned long			numa_pages_migrated;
>> >>  #endif /* CONFIG_NUMA_BALANCING */
>> >>  
>> >> +#ifdef CONFIG_RSEQ
>> >> +	struct rseq __user *rseq;
>> >> +	u32 rseq_len;
>> >> +	u32 rseq_sig;
>> >> +	bool rseq_preempt, rseq_signal, rseq_migrate;
>> > 
>> > No bool please. Use something that has a defined size in ILP32/LP64.
>> > _Bool makes it absolutely impossible to speculate on structure layout
>> > across architectures.
>> 
>> I should as well make all those a bitmask within a "u32 rseq_event_mask" then,
>> sounds fair ?
> 
> Sure, whatever works and isn't _Bool ;-)

So something along those lines should do the trick (including
the mask request from Ben Maurer):

diff --git a/include/linux/sched.h b/include/linux/sched.h
index b995a3b..44aef30 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -982,7 +982,7 @@ struct task_struct {
        struct rseq __user *rseq;
        u32 rseq_len;
        u32 rseq_sig;
-       bool rseq_preempt, rseq_signal, rseq_migrate;
+       u32 rseq_event_mask;
 #endif
 
        struct tlbflush_unmap_batch     tlb_ubc;
@@ -1676,6 +1676,16 @@ static inline void set_task_cpu(struct task_struct *p, un
 #endif
 
 #ifdef CONFIG_RSEQ
+/*
+ * Map the event mask on the user-space ABI enum rseq_cs_flags
+ * for direct mask checks.
+ */
+enum rseq_event_mask {
+       RSEQ_EVENT_PREEMPT      = RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT,
+       RSEQ_EVENT_SIGNAL       = RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL,
+       RSEQ_EVENT_MIGRATE      = RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE,
+};
+
 static inline void rseq_set_notify_resume(struct task_struct *t)
 {
        if (t->rseq)
@@ -1718,16 +1728,16 @@ static inline void rseq_sched_out(struct task_struct *t)
 }
 static inline void rseq_signal_deliver(struct pt_regs *regs)
 {
-       current->rseq_signal = true;
+       current->rseq_event_mask |= RSEQ_EVENT_SIGNAL;
        rseq_handle_notify_resume(regs);
 }
 static inline void rseq_preempt(struct task_struct *t)
 {
-       t->rseq_preempt = true;
+       t->rseq_event_mask |= RSEQ_EVENT_PREEMPT;
 }
 static inline void rseq_migrate(struct task_struct *t)
 {
-       t->rseq_migrate = true;
+       t->rseq_event_mask |= RSEQ_EVENT_MIGRATE;
 }
 #else
 static inline void rseq_set_notify_resume(struct task_struct *t)
diff --git a/kernel/rseq.c b/kernel/rseq.c
index 6f0d48c..d773003 100644
--- a/kernel/rseq.c
+++ b/kernel/rseq.c
@@ -159,7 +159,7 @@ static bool rseq_get_rseq_cs(struct task_struct *t,
 static int rseq_need_restart(struct task_struct *t, uint32_t cs_flags)
 {
        bool need_restart = false;
-       uint32_t flags;
+       uint32_t flags, event_mask;
 
        /* Get thread flags. */
        if (__get_user(flags, &t->rseq->flags))
@@ -174,26 +174,17 @@ static int rseq_need_restart(struct task_struct *t, uint32
         * a preempted signal handler could fail to restart the prior
         * execution context on sigreturn.
         */
-       if (flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL) {
-               if (!(flags & RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE))
-                       return -EINVAL;
-               if (!(flags & RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT))
+       if (unlikely(flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL)) {
+               if ((flags & (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
+                               | RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT))
+                       != (RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE
+                                | RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT))
                        return -EINVAL;
        }
-       if (t->rseq_migrate
-                       && !(flags & RSEQ_CS_FLAG_NO_RESTART_ON_MIGRATE))
-               need_restart = true;
-       else if (t->rseq_preempt
-                       && !(flags & RSEQ_CS_FLAG_NO_RESTART_ON_PREEMPT))
-               need_restart = true;
-       else if (t->rseq_signal
-                       && !(flags & RSEQ_CS_FLAG_NO_RESTART_ON_SIGNAL))
-               need_restart = true;
-
-       t->rseq_preempt = false;
-       t->rseq_signal = false;
-       t->rseq_migrate = false;
-       if (need_restart)
+       event_mask = t->rseq_event_mask;
+       t->rseq_event_mask = 0;
+       event_mask &= ~flags;
+       if (event_mask)
                return 1;
        return 0;
 }


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com