[RFC PATCH 0/6] Simplify setting thread flags to a particular value

[RFC PATCH 0/6] Simplify setting thread flags to a particular value

[Dave Martin]

There are a number of bits of code sprinkled around the kernel to
set a thread flag if a certain condition is true, and clear it
otherwise.

To help make those call sites terser and less cumbersome, this
patch adds a new family of thread flag manipulators

	update*_thread_flag([...,] flag, cond)

See patch 1 for details.

Reviewers please pay close attention to accidental conversion of
set_tsk_thread_flag() to set_thread_flag() for example, and accidental
inversion of conditions.  I'm pretty sure neither happens, but
build-testing can't pick these up.

Build-tested on x86 and the explicitly patched architectures (defconfig
and multi_v7_defconfig for arm, defconfig for arm64, mips, powerpc,
sparc).

Boot-tested on arm64, with some context-switch stress tests that depend
on correct maintenance of TIF_FOREIGN_FPSTATE.


Background rationale:

I've found myself coding this idiom a few times now and would like to
stop duplicating it.  Using an ungainly lash-up of sed and grep I tried
to find other users of the same idiom without too many false positives,
and there while there aren't a huge number it seemed enough to be worth
having a go at a simple cleanup.

The core changes are in the first patch; other changes are split
per-arch.


Dave Martin (6):
  thread_info: Add update_thread_flag() helpers
  ARM: Use update_thread_flag()
  arm64: Use update{,_tsk}_thread_flag()
  MIPS: Use update{,_tsk}_thread_flag()
  powerpc: Use update_thread_flag()
  sparc: Use update_thread_flag()

 arch/arm/kernel/elf.c           |  9 +++------
 arch/arm64/kernel/fpsimd.c      | 19 +++++++------------
 arch/mips/kernel/elf.c          | 10 ++--------
 arch/mips/kernel/process.c      | 15 ++++++---------
 arch/mips/kernel/ptrace.c       |  5 +----
 arch/mips/kernel/syscall.c      | 10 ++--------
 arch/powerpc/include/asm/elf.h  | 10 ++--------
 arch/sparc/include/asm/elf_64.h |  5 +----
 include/linux/sched.h           |  6 ++++++
 include/linux/thread_info.h     | 11 +++++++++++
 include/trace/syscall.h         |  6 ++----
 kernel/ptrace.c                 | 13 +++++--------
 12 files changed, 48 insertions(+), 71 deletions(-)

-- 
2.1.4

[RFC PATCH 1/6] thread_info: Add update_thread_flag() helpers

[Dave Martin]

There are a number of bits of code sprinkled around the kernel to
set a thread flag if a certain condition is true, and clear it
otherwise.

To help make those call sites terser and less cumbersome, this
patch adds a new family of thread flag manipulators

	update*_thread_flag([...,] flag, cond)

which do the equivalent of:

	if (cond)
		set*_thread_flag([...,] flag);
	else
		clear*_thread_flag([...,] flag);

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Oleg Nesterov <oleg@redhat.com>
---

Note, this patch assumes C99 _Bool semantics where casting a value to
_Bool is true is and only if the value compares != 0.  This would not
be true if bool where a typedef of unsigned char for example.

It appears that the kernel has always used the C99 semantics for bool
and relies on them elsewhere, so I don't believe this patch introduces
a new dependency.
---
 include/linux/sched.h       |  6 ++++++
 include/linux/thread_info.h | 11 +++++++++++
 include/trace/syscall.h     |  6 ++----
 kernel/ptrace.c             | 13 +++++--------
 4 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index b3d697f..c2c3051 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1578,6 +1578,12 @@ static inline void clear_tsk_thread_flag(struct task_struct *tsk, int flag)
 	clear_ti_thread_flag(task_thread_info(tsk), flag);
 }
 
+static inline void update_tsk_thread_flag(struct task_struct *tsk, int flag,
+					  bool value)
+{
+	update_ti_thread_flag(task_thread_info(tsk), flag, value);
+}
+
 static inline int test_and_set_tsk_thread_flag(struct task_struct *tsk, int flag)
 {
 	return test_and_set_ti_thread_flag(task_thread_info(tsk), flag);
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 34f053a..efd5ec4 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -64,6 +64,15 @@ static inline void clear_ti_thread_flag(struct thread_info *ti, int flag)
 	clear_bit(flag, (unsigned long *)&ti->flags);
 }
 
+static inline void update_ti_thread_flag(struct thread_info *ti, int flag,
+					 bool value)
+{
+	if (value)
+		set_ti_thread_flag(ti, flag);
+	else
+		clear_ti_thread_flag(ti, flag);
+}
+
 static inline int test_and_set_ti_thread_flag(struct thread_info *ti, int flag)
 {
 	return test_and_set_bit(flag, (unsigned long *)&ti->flags);
@@ -83,6 +92,8 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
 	set_ti_thread_flag(current_thread_info(), flag)
 #define clear_thread_flag(flag) \
 	clear_ti_thread_flag(current_thread_info(), flag)
+#define update_thread_flag(flag, value) \
+	update_ti_thread_flag(current_thread_info(), flag, value)
 #define test_and_set_thread_flag(flag) \
 	test_and_set_ti_thread_flag(current_thread_info(), flag)
 #define test_and_clear_thread_flag(flag) \
diff --git a/include/trace/syscall.h b/include/trace/syscall.h
index dc8ac27..dcc9bdf 100644
--- a/include/trace/syscall.h
+++ b/include/trace/syscall.h
@@ -37,10 +37,8 @@ struct syscall_metadata {
 #if defined(CONFIG_TRACEPOINTS) && defined(CONFIG_HAVE_SYSCALL_TRACEPOINTS)
 static inline void syscall_tracepoint_update(struct task_struct *p)
 {
-	if (test_thread_flag(TIF_SYSCALL_TRACEPOINT))
-		set_tsk_thread_flag(p, TIF_SYSCALL_TRACEPOINT);
-	else
-		clear_tsk_thread_flag(p, TIF_SYSCALL_TRACEPOINT);
+	update_tsk_thread_flag(p, TIF_SYSCALL_TRACEPOINT,
+			       test_thread_flag(TIF_SYSCALL_TRACEPOINT));
 }
 #else
 static inline void syscall_tracepoint_update(struct task_struct *p)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 21fec73..7a2bd8d 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -785,16 +785,13 @@ static int ptrace_resume(struct task_struct *child, long request,
 	if (!valid_signal(data))
 		return -EIO;
 
-	if (request == PTRACE_SYSCALL)
-		set_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
-	else
-		clear_tsk_thread_flag(child, TIF_SYSCALL_TRACE);
+	update_tsk_thread_flag(child, TIF_SYSCALL_TRACE,
+			       request == PTRACE_SYSCALL);
 
 #ifdef TIF_SYSCALL_EMU
-	if (request == PTRACE_SYSEMU || request == PTRACE_SYSEMU_SINGLESTEP)
-		set_tsk_thread_flag(child, TIF_SYSCALL_EMU);
-	else
-		clear_tsk_thread_flag(child, TIF_SYSCALL_EMU);
+	update_tsk_thread_flag(child, TIF_SYSCALL_EMU,
+			       request == PTRACE_SYSEMU ||
+			       request == PTRACE_SYSEMU_SINGLESTEP);
 #endif
 
 	if (is_singleblock(request)) {
-- 
2.1.4

[RFC PATCH 2/6] ARM: Use update_thread_flag()

[Dave Martin]

This patch uses the new update_thread_flag() helper to simplify an
if () set; else clear; construct.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
---
 arch/arm/kernel/elf.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/elf.c b/arch/arm/kernel/elf.c
index 1824229..aade393 100644
--- a/arch/arm/kernel/elf.c
+++ b/arch/arm/kernel/elf.c
@@ -68,12 +68,9 @@ void elf_set_personality(const struct elf32_hdr *x)
 	 * binary is EABI or softfloat (and thus, guaranteed not to use
 	 * FPA instructions.)
 	 */
-	if (elf_hwcap & HWCAP_IWMMXT &&
-	    eflags & (EF_ARM_EABI_MASK | EF_ARM_SOFT_FLOAT)) {
-		set_thread_flag(TIF_USING_IWMMXT);
-	} else {
-		clear_thread_flag(TIF_USING_IWMMXT);
-	}
+	update_thread_flag(TIF_USING_IWMMXT,
+			   elf_hwcap & HWCAP_IWMMXT &&
+			   eflags & (EF_ARM_EABI_MASK | EF_ARM_SOFT_FLOAT));
 }
 EXPORT_SYMBOL(elf_set_personality);
 
-- 
2.1.4

[RFC PATCH 3/6] arm64: Use update{,_tsk}_thread_flag()

[Dave Martin]

This patch uses the new update_thread_flag() helpers to simplify a
couple of if () set; else clear; constructs.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/fpsimd.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 87a3536..0c4e7e0 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -618,10 +618,8 @@ int sve_set_vector_length(struct task_struct *task,
 	task->thread.sve_vl = vl;
 
 out:
-	if (flags & PR_SVE_VL_INHERIT)
-		set_tsk_thread_flag(task, TIF_SVE_VL_INHERIT);
-	else
-		clear_tsk_thread_flag(task, TIF_SVE_VL_INHERIT);
+	update_tsk_thread_flag(task, TIF_SVE_VL_INHERIT,
+			       flags & PR_SVE_VL_INHERIT);
 
 	return 0;
 }
@@ -902,7 +900,7 @@ void fpsimd_thread_switch(struct task_struct *next)
 	if (current->mm)
 		task_fpsimd_save();
 
-	if (next->mm) {
+	if (next->mm)
 		/*
 		 * If we are switching to a task whose most recent userland
 		 * FPSIMD state is already in the registers of *this* cpu,
@@ -910,13 +908,10 @@ void fpsimd_thread_switch(struct task_struct *next)
 		 * the TIF_FOREIGN_FPSTATE flag so the state will be loaded
 		 * upon the next return to userland.
 		 */
-		if (__this_cpu_read(fpsimd_last_state.st) ==
-			&next->thread.uw.fpsimd_state
-		    && next->thread.fpsimd_cpu == smp_processor_id())
-			clear_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE);
-		else
-			set_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE);
-	}
+		update_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE,
+			__this_cpu_read(fpsimd_last_state.st) !=
+				&next->thread.uw.fpsimd_state ||
+			next->thread.fpsimd_cpu != smp_processor_id());
 }
 
 void fpsimd_flush_thread(void)
-- 
2.1.4

[RFC PATCH 4/6] MIPS: Use update{,_tsk}_thread_flag()

[Dave Martin]

This patch uses the new update_thread_flag() helpers to simplify a
couple of if () set; else clear; constructs.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: James Hogan <jhogan@kernel.org>
---
 arch/mips/kernel/elf.c     | 10 ++--------
 arch/mips/kernel/process.c | 15 ++++++---------
 arch/mips/kernel/ptrace.c  |  5 +----
 arch/mips/kernel/syscall.c | 10 ++--------
 4 files changed, 11 insertions(+), 29 deletions(-)

diff --git a/arch/mips/kernel/elf.c b/arch/mips/kernel/elf.c
index 731325a..2351509 100644
--- a/arch/mips/kernel/elf.c
+++ b/arch/mips/kernel/elf.c
@@ -267,14 +267,8 @@ int arch_check_elf(void *_ehdr, bool has_interpreter, void *_interp_ehdr,
 
 static inline void set_thread_fp_mode(int hybrid, int regs32)
 {
-	if (hybrid)
-		set_thread_flag(TIF_HYBRID_FPREGS);
-	else
-		clear_thread_flag(TIF_HYBRID_FPREGS);
-	if (regs32)
-		set_thread_flag(TIF_32BIT_FPREGS);
-	else
-		clear_thread_flag(TIF_32BIT_FPREGS);
+	update_thread_flag(TIF_HYBRID_FPREGS, hybrid);
+	update_thread_flag(TIF_32BIT_FPREGS, regs32);
 }
 
 void mips_set_personality_fp(struct arch_elf_state *state)
diff --git a/arch/mips/kernel/process.c b/arch/mips/kernel/process.c
index b9e9bf6..4bdecb6 100644
--- a/arch/mips/kernel/process.c
+++ b/arch/mips/kernel/process.c
@@ -763,18 +763,15 @@ int mips_set_process_fp_mode(struct task_struct *task, unsigned int value)
 	 */
 	for_each_thread(task, t) {
 		/* Update desired FP register width */
-		if (value & PR_FP_MODE_FR) {
-			clear_tsk_thread_flag(t, TIF_32BIT_FPREGS);
-		} else {
-			set_tsk_thread_flag(t, TIF_32BIT_FPREGS);
+		update_tsk_thread_flag(t, TIF_32BIT_REGS,
+				       !(value & PR_FP_MODE_FR));
+
+		if (!(value & PR_FP_MODE_FR))
 			clear_tsk_thread_flag(t, TIF_MSA_CTX_LIVE);
-		}
 
 		/* Update desired FP single layout */
-		if (value & PR_FP_MODE_FRE)
-			set_tsk_thread_flag(t, TIF_HYBRID_FPREGS);
-		else
-			clear_tsk_thread_flag(t, TIF_HYBRID_FPREGS);
+		update_tsk_thread_flag(t, TIF_HYBRID_FPREGS,
+				       value & PR_FP_MODE_FRE);
 	}
 
 	/* Allow threads to use FP again */
diff --git a/arch/mips/kernel/ptrace.c b/arch/mips/kernel/ptrace.c
index 0b23b1a..b218812 100644
--- a/arch/mips/kernel/ptrace.c
+++ b/arch/mips/kernel/ptrace.c
@@ -279,10 +279,7 @@ int ptrace_set_watch_regs(struct task_struct *child,
 		child->thread.watch.mips3264.watchhi[i] = ht[i];
 	}
 
-	if (watch_active)
-		set_tsk_thread_flag(child, TIF_LOAD_WATCH);
-	else
-		clear_tsk_thread_flag(child, TIF_LOAD_WATCH);
+	update_tsk_thread_flag(child, TIF_LOAD_WATCH, watch_active);
 
 	return 0;
 }
diff --git a/arch/mips/kernel/syscall.c b/arch/mips/kernel/syscall.c
index 69c17b5..e9fa130 100644
--- a/arch/mips/kernel/syscall.c
+++ b/arch/mips/kernel/syscall.c
@@ -209,14 +209,8 @@ SYSCALL_DEFINE3(sysmips, long, cmd, long, arg1, long, arg2)
 		if (arg1 & ~3)
 			return -EINVAL;
 
-		if (arg1 & 1)
-			set_thread_flag(TIF_FIXADE);
-		else
-			clear_thread_flag(TIF_FIXADE);
-		if (arg1 & 2)
-			set_thread_flag(TIF_LOGADE);
-		else
-			clear_thread_flag(TIF_LOGADE);
+		update_thread_flag(TIF_FIXADE, arg1 & 1);
+		update_thread_flag(TIF_LOGADE, arg1 & 2);
 
 		return 0;
 
-- 
2.1.4

[RFC PATCH 5/6] powerpc: Use update_thread_flag()

[Dave Martin]

This patch uses the new update_thread_flag() helper to simplify a
couple of if () set; else clear; constructs.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/elf.h | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h
index 548d9a4..136c9b1 100644
--- a/arch/powerpc/include/asm/elf.h
+++ b/arch/powerpc/include/asm/elf.h
@@ -88,14 +88,8 @@ typedef elf_vrregset_t elf_fpxregset_t;
 #ifdef __powerpc64__
 # define SET_PERSONALITY(ex)					\
 do {								\
-	if (((ex).e_flags & 0x3) == 2)				\
-		set_thread_flag(TIF_ELF2ABI);			\
-	else							\
-		clear_thread_flag(TIF_ELF2ABI);			\
-	if ((ex).e_ident[EI_CLASS] == ELFCLASS32)		\
-		set_thread_flag(TIF_32BIT);			\
-	else							\
-		clear_thread_flag(TIF_32BIT);			\
+	update_thread_flag(TIF_ELF2ABI, ((ex).e_flags & 0x3) == 2);	\
+	update_thread_flag(TIF_32BIT, (ex).e_ident[EI_CLASS] == ELFCLASS32); \
 	if (personality(current->personality) != PER_LINUX32)	\
 		set_personality(PER_LINUX |			\
 			(current->personality & (~PER_MASK)));	\
-- 
2.1.4

[RFC PATCH 6/6] sparc: Use update_thread_flag()

[Dave Martin]

This patch uses the new update_thread_flag() helper to simplify an
if () set; else clear; construct.

No functional change.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Cc: "David S. Miller" <davem@davemloft.net>
---
 arch/sparc/include/asm/elf_64.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/sparc/include/asm/elf_64.h b/arch/sparc/include/asm/elf_64.h
index 7e078bc..f1ef89c 100644
--- a/arch/sparc/include/asm/elf_64.h
+++ b/arch/sparc/include/asm/elf_64.h
@@ -202,10 +202,7 @@ extern unsigned long sparc64_elf_hwcap;
 #define ELF_PLATFORM	(NULL)
 
 #define SET_PERSONALITY(ex)				\
-do {	if ((ex).e_ident[EI_CLASS] == ELFCLASS32)	\
-		set_thread_flag(TIF_32BIT);		\
-	else						\
-		clear_thread_flag(TIF_32BIT);		\
+do {	update_thread_flag(TIF_32BIT, (ex).e_ident[EI_CLASS] == ELFCLASS32); \
 	/* flush_thread will update pgd cache */	\
 	if (personality(current->personality) != PER_LINUX32)	\
 		set_personality(PER_LINUX |		\
-- 
2.1.4

Re: [RFC PATCH 1/6] thread_info: Add update_thread_flag() helpers

[Will Deacon]

On Thu, Apr 19, 2018 at 11:58:43AM +0100, Dave Martin wrote:
> There are a number of bits of code sprinkled around the kernel to
> set a thread flag if a certain condition is true, and clear it
> otherwise.
> 
> To help make those call sites terser and less cumbersome, this
> patch adds a new family of thread flag manipulators
> 
> 	update*_thread_flag([...,] flag, cond)
> 
> which do the equivalent of:
> 
> 	if (cond)
> 		set*_thread_flag([...,] flag);
> 	else
> 		clear*_thread_flag([...,] flag);
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Oleg Nesterov <oleg@redhat.com>
> ---

Looks sensible to me:

Acked-by: Will Deacon <will.deacon@arm.com>

Will

Re: [RFC PATCH 3/6] arm64: Use update{,_tsk}_thread_flag()

[Will Deacon]

On Thu, Apr 19, 2018 at 11:58:45AM +0100, Dave Martin wrote:
> This patch uses the new update_thread_flag() helpers to simplify a
> couple of if () set; else clear; constructs.
> 
> No functional change.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/kernel/fpsimd.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 87a3536..0c4e7e0 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -618,10 +618,8 @@ int sve_set_vector_length(struct task_struct *task,
>  	task->thread.sve_vl = vl;
>  
>  out:
> -	if (flags & PR_SVE_VL_INHERIT)
> -		set_tsk_thread_flag(task, TIF_SVE_VL_INHERIT);
> -	else
> -		clear_tsk_thread_flag(task, TIF_SVE_VL_INHERIT);
> +	update_tsk_thread_flag(task, TIF_SVE_VL_INHERIT,
> +			       flags & PR_SVE_VL_INHERIT);
>  
>  	return 0;
>  }
> @@ -902,7 +900,7 @@ void fpsimd_thread_switch(struct task_struct *next)
>  	if (current->mm)
>  		task_fpsimd_save();
>  
> -	if (next->mm) {
> +	if (next->mm)
>  		/*
>  		 * If we are switching to a task whose most recent userland
>  		 * FPSIMD state is already in the registers of *this* cpu,
> @@ -910,13 +908,10 @@ void fpsimd_thread_switch(struct task_struct *next)
>  		 * the TIF_FOREIGN_FPSTATE flag so the state will be loaded
>  		 * upon the next return to userland.
>  		 */
> -		if (__this_cpu_read(fpsimd_last_state.st) ==
> -			&next->thread.uw.fpsimd_state
> -		    && next->thread.fpsimd_cpu == smp_processor_id())
> -			clear_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE);
> -		else
> -			set_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE);
> -	}
> +		update_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE,
> +			__this_cpu_read(fpsimd_last_state.st) !=
> +				&next->thread.uw.fpsimd_state ||
> +			next->thread.fpsimd_cpu != smp_processor_id());

This will conflict with some patches in-flight from a chap called Dave
Martin ;)

Other than that:

Acked-by: Will Deacon <will.deacon@arm.com>

Will