Scope-based Resource Management
Subsystems (19)
Peter Zijlstra
maintainerv3 cover Scope-based Resource Management
Hi,
After a wee bit of bike-shedding on the syntax/API of the thing I think we're in a reasonable shape.
There's still the no_free_ptr() vs take_ptr() thing, but that can be easily sorted with a little script over the patches if we reach consensus.
I've taken to converting kernel/sched/core.c and kernel/events/core.c to see how well this stuff actually works. Unlike last time, I've split them up into a gazillion little patches. Both for my sanity -- bisect is genius when you're trying to debug stuff in the middle of the night as well as reviewer sanity.
These are by no means 'complete' convertions, I've mostly focussed on functions that had 'goto' in them. Since that's a large part of why I started on all this.
x86_x64-defconfig boots and passes perf test. I'll go figure out how to point syzcaller at it.
The patches have gone through a few cycles of 0day over the weekend, and mostly builds fine now.
Dan Carpenter poked me about how sparse doesn't yet understand the __cleanup__ attribute and seems to suffer from decl-after-stmt issues, so that might be something that needs attention.
Anyway, does this look like something we can live with?
--- Makefile | 6 +- arch/arm64/kernel/vdso32/Makefile | 2 - drivers/dma/ioat/dma.c | 12 +- include/linux/cleanup.h | 167 ++++ include/linux/compiler-clang.h | 9 + include/linux/compiler_attributes.h | 6 + include/linux/cpu.h | 2 + include/linux/device.h | 7 + include/linux/file.h | 11 + include/linux/irqflags.h | 7 + include/linux/mutex.h | 4 + include/linux/percpu.h | 4 + include/linux/perf_event.h | 14 +- include/linux/preempt.h | 5 + include/linux/rcupdate.h | 3 + include/linux/rwsem.h | 8 + include/linux/sched/task.h | 2 + include/linux/slab.h | 3 + include/linux/spinlock.h | 31 + include/linux/srcu.h | 5 + kernel/events/core.c | 1642 +++++++++++++++-------------------- kernel/sched/core.c | 931 +++++++++----------- kernel/sched/sched.h | 40 + scripts/checkpatch.pl | 2 +- security/apparmor/include/lib.h | 6 +- 25 files changed, 1433 insertions(+), 1496 deletions(-)
Peter Zijlstra
maintainerPATCH v3 1/57 dmaengine: ioat: Free up __cleanup() name
In order to use __cleanup for __attribute__((__cleanup__(func))) the name must not be used for anything else. Avoid the conflict.
--- drivers/dma/ioat/dma.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/dma/ioat/dma.c +6 -6
@@ -584,11 +584,11 @@ desc_get_errstat(struct ioatdma_chan *io }/** -* __cleanup - reclaim used descriptors +* __ioat_cleanup - reclaim used descriptors * @ioat_chan: channel (ring) to clean * @phys_complete: zeroed (or not) completion address (from status) */ -static void __cleanup (struct ioatdma_chan *ioat_chan,dma_addr_t phys_complete) +static void __ioat_cleanup (struct ioatdma_chan *ioat_chan,dma_addr_t phys_complete) {struct ioatdma_device *ioat_dma = ioat_chan->ioat_dma;struct ioat_ring_ent *desc;
@@ -675,7 +675,7 @@ static void ioat_cleanup(struct ioatdma_spin_lock_bh (&ioat_chan->cleanup_lock);if (ioat_cleanup_preamble (ioat_chan, &phys_complete)) -__cleanup (ioat_chan, phys_complete); +__ioat_cleanup (ioat_chan, phys_complete);if (is_ioat_halted (*ioat_chan->completion)) {u32 chanerr =readl (ioat_chan->reg_base +IOAT_CHANERR_OFFSET );
@@ -712,7 +712,7 @@ static void ioat_restart_channel(structioat_quiesce (ioat_chan,0 );if (ioat_cleanup_preamble (ioat_chan, &phys_complete)) -__cleanup (ioat_chan, phys_complete); +__ioat_cleanup (ioat_chan, phys_complete);__ioat_restart_chan (ioat_chan); }
@@ -786,7 +786,7 @@ static void ioat_eh(struct ioatdma_chan/* cleanup so tail points to descriptor that caused the error */ if (ioat_cleanup_preamble (ioat_chan, &phys_complete)) -__cleanup (ioat_chan, phys_complete); +__ioat_cleanup (ioat_chan, phys_complete); chanerr =readl (ioat_chan->reg_base +IOAT_CHANERR_OFFSET );pci_read_config_dword (pdev,IOAT_PCI_CHANERR_INT_OFFSET , &chanerr_int);
@@ -943,7 +943,7 @@ void ioat_timer_event(struct timer_list/* timer restarted in ioat_cleanup_preamble * and IOAT_COMPLETION_ACK cleared */ -__cleanup (ioat_chan, phys_complete); +__ioat_cleanup (ioat_chan, phys_complete); goto unlock_out; }
Re: [PATCH v3 01/57] dmaengine: ioat: Free up __cleanup() name
Quoted 4 lines · Peter Zijlstra
In order to use
__cleanupfor __attribute__((__cleanup__(func))) the name must not be used for anything else. Avoid the conflict.Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Quoted 58 lines
--- drivers/dma/ioat/dma.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/dma/ioat/dma.c +6
-6@@ -584,11 +584,11 @@ desc_get_errstat(struct ioatdma_chan *io }/** -* __cleanup - reclaim used descriptors +* __ioat_cleanup - reclaim used descriptors * @ioat_chan: channel (ring) to clean * @phys_complete: zeroed (or not) completion address (from status) */ -static void __cleanup (struct ioatdma_chan *ioat_chan,dma_addr_t phys_complete) +static void __ioat_cleanup (struct ioatdma_chan *ioat_chan,dma_addr_t phys_complete) {struct ioatdma_device *ioat_dma = ioat_chan->ioat_dma;struct ioat_ring_ent *desc;@@ -675,7 +675,7 @@ static void ioat_cleanup(struct ioatdma_spin_lock_bh (&ioat_chan->cleanup_lock);if (ioat_cleanup_preamble (ioat_chan, &phys_complete)) -__cleanup (ioat_chan, phys_complete); +__ioat_cleanup (ioat_chan, phys_complete);if (is_ioat_halted (*ioat_chan->completion)) {u32 chanerr =readl (ioat_chan->reg_base +IOAT_CHANERR_OFFSET );@@ -712,7 +712,7 @@ static void ioat_restart_channel(structioat_quiesce (ioat_chan,0 );if (ioat_cleanup_preamble (ioat_chan, &phys_complete)) -__cleanup (ioat_chan, phys_complete); +__ioat_cleanup (ioat_chan, phys_complete);__ioat_restart_chan (ioat_chan); }@@ -786,7 +786,7 @@ static void ioat_eh(struct ioatdma_chan/* cleanup so tail points to descriptor that caused the error */ if (ioat_cleanup_preamble (ioat_chan, &phys_complete)) -__cleanup (ioat_chan, phys_complete); +__ioat_cleanup (ioat_chan, phys_complete); chanerr =readl (ioat_chan->reg_base +IOAT_CHANERR_OFFSET );pci_read_config_dword (pdev,IOAT_PCI_CHANERR_INT_OFFSET , &chanerr_int);@@ -943,7 +943,7 @@ void ioat_timer_event(struct timer_list/* timer restarted in ioat_cleanup_preamble * and IOAT_COMPLETION_ACK cleared */ -__cleanup (ioat_chan, phys_complete); +__ioat_cleanup (ioat_chan, phys_complete); goto unlock_out; }
Peter Zijlstra
maintainerPATCH v3 2/57 apparmor: Free up __cleanup() name
In reply to Peter ZijlstraIn order to use __cleanup for __attribute__((__cleanup__(func))) the name must not be used for anything else. Avoid the conflict.
--- security/apparmor/include/lib.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
security/apparmor/include/lib.h +3 -3
@@ -232,7 +232,7 @@ void aa_policy_destroy(struct aa_policy*/ #define fn_label_build (L ,P ,GFP ,FN ) \ ({ \ - __label__ __cleanup, __done; \ + __label__ __do_cleanup, __done; \ struct aa_label *__new_; \ \ if ((L)->size > 1) { \
@@ -250,7 +250,7 @@ void aa_policy_destroy(struct aa_policy __new_ = (FN ); \AA_BUG (!__new_); \if (IS_ERR (__new_)) \ - goto __cleanup; \ + goto __do_cleanup; \ __lvec[__j++] = __new_; \ } \for (__j = __count =0 ; __j < (L )->size; __j++) \
@@ -272,7 +272,7 @@ void aa_policy_destroy(struct aa_policyvec_cleanup (profile, __pvec, __count); \ }else \ __new_ =NULL ; \ -__cleanup: \ +__do_cleanup: \vec_cleanup (label, __lvec, (L )->size); \ }else { \ (P ) =labels_profile (L ); \
Re: [PATCH v3 02/57] apparmor: Free up __cleanup() name
Quoted 4 lines · Peter Zijlstra
In order to use
__cleanupfor __attribute__((__cleanup__(func))) the name must not be used for anything else. Avoid the conflict.Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Quoted 35 lines
--- security/apparmor/include/lib.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)security/apparmor/include/lib.h +3
-3@@ -232,7 +232,7 @@ void aa_policy_destroy(struct aa_policy*/ #define fn_label_build (L ,P ,GFP ,FN ) \ ({ \ - __label__ __cleanup, __done; \ + __label__ __do_cleanup, __done; \ struct aa_label *__new_; \ \ if ((L)->size > 1) { \@@ -250,7 +250,7 @@ void aa_policy_destroy(struct aa_policy __new_ = (FN ); \AA_BUG (!__new_); \if (IS_ERR (__new_)) \ - goto __cleanup; \ + goto __do_cleanup; \ __lvec[__j++] = __new_; \ } \for (__j = __count =0 ; __j < (L )->size; __j++) \@@ -272,7 +272,7 @@ void aa_policy_destroy(struct aa_policyvec_cleanup (profile, __pvec, __count); \ }else \ __new_ =NULL ; \ -__cleanup: \ +__do_cleanup: \vec_cleanup (label, __lvec, (L )->size); \ }else { \ (P ) =labels_profile (L ); \
John Johansen
maintainerRe: [PATCH v3 02/57] apparmor: Free up __cleanup() name
In reply to Peter ZijlstraQuoted 4 lines · Peter Zijlstra
In order to use
__cleanupfor __attribute__((__cleanup__(func))) the name must not be used for anything else. Avoid the conflict.Signed-off-by Peter Zijlstra (Intel) <[email protected]>
if you want, I can just pull this small change into the apparmor tree so you don't have to carry it anymore as part of the series.
Quoted 35 lines
--- security/apparmor/include/lib.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)security/apparmor/include/lib.h +3
-3@@ -232,7 +232,7 @@ void aa_policy_destroy(struct aa_policy*/ #define fn_label_build (L ,P ,GFP ,FN ) \ ({ \ - __label__ __cleanup, __done; \ + __label__ __do_cleanup, __done; \ struct aa_label *__new_; \ \ if ((L)->size > 1) { \@@ -250,7 +250,7 @@ void aa_policy_destroy(struct aa_policy __new_ = (FN ); \AA_BUG (!__new_); \if (IS_ERR (__new_)) \ - goto __cleanup; \ + goto __do_cleanup; \ __lvec[__j++] = __new_; \ } \for (__j = __count =0 ; __j < (L )->size; __j++) \@@ -272,7 +272,7 @@ void aa_policy_destroy(struct aa_policyvec_cleanup (profile, __pvec, __count); \ }else \ __new_ =NULL ; \ -__cleanup: \ +__do_cleanup: \vec_cleanup (label, __lvec, (L )->size); \ }else { \ (P ) =labels_profile (L ); \
Re: [PATCH v3 02/57] apparmor: Free up __cleanup() name
In reply to Peter ZijlstraQuoted 4 lines · Peter Zijlstra
In order to use
__cleanupfor __attribute__((__cleanup__(func))) the name must not be used for anything else. Avoid the conflict.Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Quoted 35 lines
--- security/apparmor/include/lib.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)security/apparmor/include/lib.h +3
-3@@ -232,7 +232,7 @@ void aa_policy_destroy(struct aa_policy*/ #define fn_label_build (L ,P ,GFP ,FN ) \ ({ \ - __label__ __cleanup, __done; \ + __label__ __do_cleanup, __done; \ struct aa_label *__new_; \ \ if ((L)->size > 1) { \@@ -250,7 +250,7 @@ void aa_policy_destroy(struct aa_policy __new_ = (FN ); \AA_BUG (!__new_); \if (IS_ERR (__new_)) \ - goto __cleanup; \ + goto __do_cleanup; \ __lvec[__j++] = __new_; \ } \for (__j = __count =0 ; __j < (L )->size; __j++) \@@ -272,7 +272,7 @@ void aa_policy_destroy(struct aa_policyvec_cleanup (profile, __pvec, __count); \ }else \ __new_ =NULL ; \ -__cleanup: \ +__do_cleanup: \vec_cleanup (label, __lvec, (L )->size); \ }else { \ (P ) =labels_profile (L ); \
Peter Zijlstra
maintainerPATCH v3 3/57 locking: Introduce __cleanup() based infrastructure
In reply to Peter ZijlstraUse __attribute__((__cleanup__(func))) to build:
- simple auto-release pointers using __free()
- 'classes' with constructor and destructor semantics for scope-based resource management.
- lock guards based on the above classes.
--- include/linux/cleanup.h | 167 ++++++++++++++++++++++++++++++++++++ include/linux/compiler-clang.h | 9 + include/linux/compiler_attributes.h | 6 + include/linux/device.h | 7 + include/linux/file.h | 6 + include/linux/irqflags.h | 7 + include/linux/mutex.h | 4 include/linux/percpu.h | 4 include/linux/preempt.h | 5 + include/linux/rcupdate.h | 3 include/linux/rwsem.h | 8 + include/linux/sched/task.h | 2 include/linux/slab.h | 3 include/linux/spinlock.h | 31 ++++++ include/linux/srcu.h | 5 + scripts/checkpatch.pl | 2 16 files changed, 268 insertions(+), 1 deletion(-)
include/linux/cleanup.h new file +167 -0
@@ -0,0 +1,167 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __LINUX_GUARDS_H +#define __LINUX_GUARDS_H + +#include <linux/compiler.h> + +/* +* DEFINE_FREE(name, type, free): +* simple helper macro that defines the required wrapper for a __free() +* based cleanup function. @free is an expression using '_T' to access +* the variable. +* +* __free(name): +* variable attribute to add a scoped based cleanup to the variable. +* +* return_ptr(p): +* returns p while inhibiting the __free(). +* +* Ex. +* +* DEFINE_FREE(kfree, void *, if (_T) kfree(_T)) +* +* struct obj *p = kmalloc(...); +* if (!p) +* return NULL; +* +* if (!init_obj(p)) +* return NULL; +* +* return_ptr(p); +*/ + +#define DEFINE_FREE (name, type, free) \ + static inline void __free_##name(void *p) { type _T = *(type *)p; free; } + +#define __free (name) __cleanup(__free_##name) + +#define no_free_ptr (p) \ + ({ __auto_type __ptr = (p); (p) = NULL; __ptr; }) + +#define return_ptr (p) return no_free_ptr(p) + + +/* +* DEFINE_CLASS(name, type, exit, init, init_args...): +* helper to define the destructor and constructor for a type. +* @exit is an expression using '_T' -- similar to FREE above. +* @init is an expression in @init_args resulting in @type +* +* EXTEND_CLASS(name, ext, init, init_args...): +* extends class @name to @name@ext with the new constructor +* +* CLASS(name, var)(args...): +* declare the variable @var as an instance of the named class +* +* Ex. +* +* DEFINE_CLASS(fdget, struct fd, fdput(_T), fdget(fd), int fd) +* +* CLASS(fdget, f)(fd); +* if (!f.file) +* return -EBADF; +* +* // use 'f' without concern +*/ + +#define DEFINE_CLASS(name, type, exit, init, init_args...) \ +typedef type class_##name##_t; \ +static inline void class_##name##_destructor(type *p) \ +{ type _T = *p; exit; } \ +static inline type class_##name##_constructor(init_args) \ +{ type t = init; return t; } + +#define EXTEND_CLASS(name, ext, init, init_args...) \ +typedef class_##name##_t class_##name##ext##_t; \ +static inline void class_##name##ext##_destructor(class_##name##_t *p) \ +{ class_##name##_destructor(p); } \ +static inline class_##name##_t class_##name##ext##_constructor(init_args) \ +{ class_##name##_t t = init; return t; } + +#define CLASS (name, var) \ + class_##name##_t var __cleanup(class_##name##_destructor) = \ + class_##name##_constructor + + +/* +* DEFINE_GUARD(name, type, lock, unlock): +* trivial wrapper around DEFINE_CLASS() above specifically +* for locks. +* +* guard(name): +* an anonymous instance of the (guard) class +* +* scoped_guard (name, args...) { }: +* similar to CLASS(name, scope)(args), except the variable (with the +* explicit name 'scope') is declard in a for-loop such that its scope is +* bound to the next (compound) statement. +* +*/ + +#define DEFINE_GUARD (name, type, lock, unlock) \ + DEFINE_CLASS(name, type, unlock, ({ lock; _T; }), type _T) + +#define guard (name) \ + CLASS(name, __UNIQUE_ID(guard)) + +#define scoped_guard(name, args...) \ + for (CLASS(name, scope)(args), \ + *done = NULL; !done; done = (void *)1) + +/* +* Additional helper macros for generating lock guards with types, either for +* locks that don't have a native type (eg. RCU, preempt) or those that need a +* 'fat' pointer (eg. spin_lock_irqsave). +* +* DEFINE_LOCK_GUARD_0(name, _lock, _unlock, ...) +* DEFINE_LOCK_GUARD_1(name, type, _lock, _unlock, ...) +* +* will result in the following type: +* +* typedef struct { +* type *lock; // 'type := void' for the _0 variant +* __VA_ARGS__; +* } class_##name##_t; +* +* As above, both _lock and _unlock are statements, except this time '_T' will +* be a pointer to the above struct. +*/ + +#define __DEFINE_UNLOCK_GUARD (name, type, _unlock, ...) \ +typedef struct { \ + type *lock; \ + __VA_ARGS__; \ +} class_##name##_t; \ + \ +static inline void class_##name##_destructor(class_##name##_t *_T) \ +{ \ + if (_T->lock) { _unlock; } \ +} + + +#define __DEFINE_LOCK_GUARD_1 (name, type, _lock) \ +static inline class_##name##_t class_##name##_constructor(type *l) \ +{ \ + class_##name##_t _t = { .lock = l }, *_T = &_t; \ + _lock; \ + return _t; \ +} + +#define __DEFINE_LOCK_GUARD_0 (name, _lock) \ +static inline class_##name##_t class_##name##_constructor(void) \ +{ \ + class_##name##_t _t = { .lock = (void*)1 }, \ + *_T __maybe_unused = &_t; \ + _lock; \ + return _t; \ +} + +#define DEFINE_LOCK_GUARD_1 (name, type, _lock, _unlock, ...) \ +__DEFINE_UNLOCK_GUARD(name, type, _unlock, __VA_ARGS__) \ +__DEFINE_LOCK_GUARD_1(name, type, _lock) + +#define DEFINE_LOCK_GUARD_0 (name, _lock, _unlock, ...) \ +__DEFINE_UNLOCK_GUARD(name, void, _unlock, __VA_ARGS__) \ +__DEFINE_LOCK_GUARD_0(name, _lock) + +#endif /* __LINUX_GUARDS_H */
include/linux/compiler-clang.h +9 -0
@@ -5,6 +5,15 @@/* Compiler specific definitions for Clang compiler */ +/* +* Clang prior to 17 is being silly and considers many __cleanup() variables +* as unused (because they are, their sole purpose is to go out of scope). +* +* https://reviews.llvm.org/D152180 +*/ +#undef __cleanup +#define __cleanup (func) __maybe_unused __attribute__((__cleanup__(func))) +/* same as gcc, this was present in clang-2.6 so we can assume it works * with any version that can compile the kernel */
include/linux/compiler_attributes.h +6 -0
@@ -77,6 +77,12 @@#define __attribute_const__ __attribute__((__const__))/* +* gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-cleanup-variable-attribute +* clang: https://clang.llvm.org/docs/AttributeReference.html#cleanup +*/ +#define __cleanup (func) __attribute__((__cleanup__(func))) + +/* * Optional: only supported since gcc >= 9 * Optional: not supported by clang *
include/linux/device.h +7 -0
@@ -30,6 +30,7 @@#include <linux/device/bus.h> #include <linux/device/class.h> #include <linux/device/driver.h> +#include <linux/cleanup.h> #include <asm/device.h> struct device ;
@@ -899,6 +900,9 @@ void device_unregister(struct device *devoid device_initialize (struct device *dev);int __must_check device_add (struct device *dev);void device_del (struct device *dev); + +DEFINE_FREE(device_del, struct device *, if (_T) device_del(_T)) +int device_for_each_child (struct device *dev,void *data,int (*fn)(struct device *dev,void *data)); int device_for_each_child_reverse(struct device *dev, void *data,
@@ -1066,6 +1070,9 @@ extern int (*platform_notify_remove)(str*/ struct device *get_device (struct device *dev);void put_device (struct device *dev); + +DEFINE_FREE(put_device, struct device *, if (_T) put_device(_T)) +bool kill_device (struct device *dev);#ifdef CONFIG_DEVTMPFS
include/linux/file.h +6 -0
@@ -10,6 +10,7 @@#include <linux/types.h> #include <linux/posix_types.h> #include <linux/errno.h> +#include <linux/cleanup.h> struct file ;
@@ -80,6 +81,8 @@ static inline void fdput_pos(struct fd ffdput (f); } +DEFINE_CLASS(fd, struct fd, fdput(_T), fdget(fd), int fd) +extern int f_dupfd (unsigned int from,struct file *file,unsigned flags);extern int replace_fd (unsigned fd,struct file *file,unsigned flags);extern void set_close_on_exec (unsigned int fd,int flag);
@@ -88,6 +91,9 @@ extern int __get_unused_fd_flags(unsigneextern int get_unused_fd_flags (unsigned flags);extern void put_unused_fd (unsigned int fd); +DEFINE_CLASS(get_unused_fd, int, if (_T >= 0) put_unused_fd(_T), + get_unused_fd_flags(flags), unsigned flags) +extern void fd_install (unsigned int fd,struct file *file); extern int __receive_fd(struct file *file, int __user *ufd,
include/linux/irqflags.h +7 -0
@@ -13,6 +13,7 @@#define _LINUX_TRACE_IRQFLAGS_H#include <linux/typecheck.h> +#include <linux/cleanup.h> #include <asm/irqflags.h> #include <asm/percpu.h>
@@ -267,4 +268,10 @@ extern void warn_bogus_irq_restore(void)#define irqs_disabled_flags (flags) raw_irqs_disabled_flags(flags) +DEFINE_LOCK_GUARD_0(irq, local_irq_disable(), local_irq_enable()) +DEFINE_LOCK_GUARD_0 (irqsave, +local_irq_save (_T->flags), +local_irq_restore (_T->flags), + unsigned long flags) +#endif
include/linux/mutex.h +4 -0
@@ -19,6 +19,7 @@#include <asm/processor.h> #include <linux/osq_lock.h> #include <linux/debug_locks.h> +#include <linux/cleanup.h> #ifdef CONFIG_DEBUG_LOCK_ALLOC # define __DEP_MAP_MUTEX_INITIALIZER (lockname) \
@@ -219,4 +220,7 @@ extern void mutex_unlock(struct mutex *lextern int atomic_dec_and_mutex_lock (atomic_t *cnt,struct mutex *lock); +DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T)) +DEFINE_FREE(mutex, struct mutex *, if (_T) mutex_unlock(_T)) +#endif /* __LINUX_MUTEX_H */
include/linux/percpu.h +4 -0
@@ -8,6 +8,7 @@#include <linux/cpumask.h> #include <linux/pfn.h> #include <linux/init.h> +#include <linux/cleanup.h> #include <asm/percpu.h>
@@ -127,6 +128,9 @@ extern void __init setup_per_cpu_areas(vextern void __percpu *__alloc_percpu_gfp (size_t size,size_t align,gfp_t gfp)__alloc_size(1) ;extern void __percpu *__alloc_percpu (size_t size,size_t align)__alloc_size(1) ;extern void free_percpu (void __percpu *__pdata); + +DEFINE_FREE(free_percpu, void __percpu *, free_percpu(_T)) +extern phys_addr_t per_cpu_ptr_to_phys (void *addr);#define alloc_percpu_gfp (type, gfp) \
include/linux/preempt.h +5 -0
@@ -8,6 +8,7 @@*/ #include <linux/linkage.h> +#include <linux/cleanup.h> #include <linux/list.h> /*
@@ -463,4 +464,8 @@ static __always_inline void preempt_enabpreempt_enable (); } +DEFINE_LOCK_GUARD_0(preempt, preempt_disable(), preempt_enable()) +DEFINE_LOCK_GUARD_0(preempt_notrace, preempt_disable_notrace(), preempt_enable_notrace()) +DEFINE_LOCK_GUARD_0(migrate, migrate_disable(), migrate_enable()) +#endif /* __LINUX_PREEMPT_H */
include/linux/rcupdate.h +3 -0
@@ -27,6 +27,7 @@#include <linux/preempt.h> #include <linux/bottom_half.h> #include <linux/lockdep.h> +#include <linux/cleanup.h> #include <asm/processor.h> #include <linux/cpumask.h> #include <linux/context_tracking_irq.h>
@@ -1095,4 +1096,6 @@ rcu_head_after_call_rcu(struct rcu_headextern int rcu_expedited;extern int rcu_normal; +DEFINE_LOCK_GUARD_0(rcu, rcu_read_lock(), rcu_read_unlock()) +#endif /* __LINUX_RCUPDATE_H */
include/linux/rwsem.h +8 -0
@@ -15,6 +15,7 @@#include <linux/spinlock.h> #include <linux/atomic.h> #include <linux/err.h> +#include <linux/cleanup.h> #ifdef CONFIG_DEBUG_LOCK_ALLOC # define __RWSEM_DEP_MAP_INIT (lockname) \
@@ -201,6 +202,13 @@ extern void up_read(struct rw_semaphore*/ extern void up_write (struct rw_semaphore *sem); +DEFINE_GUARD(rwsem_read, struct rw_semaphore *, down_read(_T), up_read(_T)) +DEFINE_GUARD(rwsem_write, struct rw_semaphore *, down_write(_T), up_write(_T)) + +DEFINE_FREE(up_read, struct rw_semaphore *, if (_T) up_read(_T)) +DEFINE_FREE(up_write, struct rw_semaphore *, if (_T) up_write(_T)) + +/* * downgrade write lock to read lock */
include/linux/sched/task.h +2 -0
@@ -126,6 +126,8 @@ static inline void put_task_struct(struc__put_task_struct (t); } +DEFINE_FREE(put_task, struct task_struct *, if (_T) put_task_struct(_T)) +static inline void put_task_struct_many (struct task_struct *t,int nr) {if (refcount_sub_and_test (nr, &t->usage))
include/linux/slab.h +3 -0
@@ -17,6 +17,7 @@#include <linux/types.h> #include <linux/workqueue.h> #include <linux/percpu-refcount.h> +#include <linux/cleanup.h> /*
@@ -211,6 +212,8 @@ void kfree(const void *objp);void kfree_sensitive (const void *objp);size_t __ksize (const void *objp); +DEFINE_FREE(kfree, void *, if (_T) kfree(_T)) +/** * ksize - Report actual allocation size of associated object *
include/linux/spinlock.h +31 -0
@@ -61,6 +61,7 @@#include <linux/stringify.h> #include <linux/bottom_half.h> #include <linux/lockdep.h> +#include <linux/cleanup.h> #include <asm/barrier.h> #include <asm/mmiowb.h>
@@ -502,5 +503,35 @@ int __alloc_bucket_spinlocks(spinlock_tvoid free_bucket_spinlocks (spinlock_t *locks); +DEFINE_LOCK_GUARD_1 (raw_spinlock, raw_spinlock_t, +raw_spin_lock (_T->lock), + raw_spin_unlock(_T->lock)) + +DEFINE_LOCK_GUARD_1 (raw_spinlock_nested, raw_spinlock_t, +raw_spin_lock_nested (_T->lock,SINGLE_DEPTH_NESTING ), + raw_spin_unlock(_T->lock)) + +DEFINE_LOCK_GUARD_1 (raw_spinlock_irq, raw_spinlock_t, +raw_spin_lock_irq (_T->lock), + raw_spin_unlock_irq(_T->lock)) + +DEFINE_LOCK_GUARD_1 (raw_spinlock_irqsave, raw_spinlock_t, +raw_spin_lock_irqsave (_T->lock, _T->flags), +raw_spin_unlock_irqrestore (_T->lock, _T->flags), + unsigned long flags) + +DEFINE_LOCK_GUARD_1 (spinlock, spinlock_t, +spin_lock (_T->lock), + spin_unlock(_T->lock)) + +DEFINE_LOCK_GUARD_1 (spinlock_irq, spinlock_t, +spin_lock_irq (_T->lock), + spin_unlock_irq(_T->lock)) + +DEFINE_LOCK_GUARD_1 (spinlock_irqsave, spinlock_t, +spin_lock_irqsave (_T->lock, _T->flags), +spin_unlock_irqrestore (_T->lock, _T->flags), + unsigned long flags) +#undef __LINUX_INSIDE_SPINLOCK_H#endif /* __LINUX_SPINLOCK_H */
include/linux/srcu.h +5 -0
@@ -343,4 +343,9 @@ static inline void smp_mb__after_srcu_re/* __srcu_read_unlock has smp_mb() internally so nothing to do here. */ } +DEFINE_LOCK_GUARD_1(srcu, struct srcu_struct, + _T->idx = srcu_read_lock(_T->lock), + srcu_read_unlock(_T->lock, _T->idx), + int idx) +#endif
scripts/checkpatch.pl +1 -1
@@ -5046,7 +5046,7 @@ sub process { if|for|while|switch|return|case| volatile|__volatile__| __attribute__|format|__extension__| - asm|__asm__)$/x) + asm|__asm__|scoped_guard)$/x) { # cpp #define statements have non-optional spaces, ie # if there is a space between the name and the open
Peter Zijlstra
maintainerRe: [PATCH v3 03/57] locking: Introduce __cleanup() based infrastructure
Quoted 8 lines · Peter Zijlstra
include/linux/cleanup.h new file +5
-0@@ -0,0 +1,167 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __LINUX_GUARDS_H +#define __LINUX_GUARDS_H + +#include <linux/compiler.h>
include/linux/cleanup.h line 15
Quoted 10 lines
include/linux/cleanup.h new file +10
-0+ +/* +* DEFINE_FREE(name, type, free): +* simple helper macro that defines the required wrapper for a __free() +* based cleanup function. @free is an expression using '_T' to access +* the variable. +* +* __free(name): +* variable attribute to add a scoped based cleanup to the variable. +*
no_free_ptr(var):
like a non-atomic xchg(var, NULL), such that the cleanup function will be inhibited -- provided it sanely deals with a NULL value.
include/linux/cleanup.h line 23
Quoted 8 lines
include/linux/cleanup.h new file +8
-0+* return_ptr(p): +* returns p while inhibiting the __free(). +* +* Ex. +* +* DEFINE_FREE(kfree, void *, if (_T) kfree(_T)) +* +* struct obj *p = kmalloc(...);
That should obviously have been:
struct obj *p __free(kfree) = kmalloc(...);
Quoted 18 lines
include/linux/cleanup.h new file +18
-0+* if (!p) +* return NULL; +* +* if (!init_obj(p)) +* return NULL; +* +* return_ptr(p); +*/ + +#define DEFINE_FREE (name, type, free) \ + static inline void __free_##name(void *p) { type _T = *(type *)p; free; } + +#define __free (name) __cleanup(__free_##name) + +#define no_free_ptr (p) \ + ({ __auto_type __ptr = (p); (p) = NULL; __ptr; }) + +#define return_ptr (p) return no_free_ptr(p)
Re: [PATCH v3 03/57] locking: Introduce __cleanup() based infrastructure
Quoted 35 lines · Peter Zijlstra
Quoted 8 lines · Peter Zijlstra
include/linux/cleanup.h new file +5
-0@@ -0,0 +1,167 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __LINUX_GUARDS_H +#define __LINUX_GUARDS_H + +#include <linux/compiler.h> include/linux/cleanup.h line 15
Quoted 10 lines
include/linux/cleanup.h new file +10
-0+ +/* +* DEFINE_FREE(name, type, free): +* simple helper macro that defines the required wrapper for a __free() +* based cleanup function. @free is an expression using '_T' to access +* the variable. +* +* __free(name): +* variable attribute to add a scoped based cleanup to the variable. +*
no_free_ptr(var):
like a non-atomicxchg(var, NULL), such that the cleanup function will be inhibited -- provided it sanely deals with a NULL value.include/linux/cleanup.h line 23
Quoted 8 lines
include/linux/cleanup.h new file +8
-0+* return_ptr(p): +* returns p while inhibiting the __free(). +* +* Ex. +* +* DEFINE_FREE(kfree, void *, if (_T) kfree(_T)) +* +* struct obj *p = kmalloc(...);
Quoted 3 lines
That should obviously have been:
struct obj *p __free(kfree) = kmalloc(...);
Okay, good. I noticed that too. :)
I also didn't see any users of the kfree helpers (maybe I missed them?) Maybe add at least one conversion just to have an example conversion for it too?
-Kees
Signature · 1 line
Kees Cook
Re: [PATCH v3 03/57] locking: Introduce __cleanup() based infrastructure
In reply to Peter ZijlstraQuoted 6 lines · Peter Zijlstra
include/linux/compiler_attributes.h +0
-0@@ -77,6 +77,12 @@#define __attribute_const__ __attribute__((__const__))
include/linux/compiler_attributes.h line 88
Quoted 10 lines
include/linux/compiler_attributes.h +6
-0/* +* gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-cleanup-variable-attribute +* clang: https://clang.llvm.org/docs/AttributeReference.html#cleanup +*/ +#define __cleanup (func) __attribute__((__cleanup__(func))) + +/* * Optional: only supported since gcc >= 9 * Optional: not supported by clang *
cleanup goes before const, but apart from that, for compiler attributes:
Thanks for bringing RAII/SBRM to the kernel!
Rust also behaves like the __cleanup__ attribute, e.g. iterating on a loop destroys and constructs the variables defined inside the loop. Also breaking to an outer label/scope.
By the way, maybe some of the macros in cleanup.h read a bit better applying clang-format to it.
Cheers,
Miguel
Re: [PATCH v3 03/57] locking: Introduce __cleanup() based infrastructure
In reply to Peter ZijlstraQuoted 1 line · Peter Zijlstra
[snip]
Quoted 11 lines
include/linux/slab.h +1
-0@@ -17,6 +17,7 @@#include <linux/types.h> #include <linux/workqueue.h> #include <linux/percpu-refcount.h> +#include <linux/cleanup.h> /*
include/linux/slab.h void kfree(const void *objp); · line 216
Quoted 5 lines
include/linux/slab.h +2
-0@@ -211,6 +212,8 @@ void kfree(const void *objp);void kfree_sensitive (const void *objp);size_t __ksize (const void *objp); +DEFINE_FREE(kfree, void *, if (_T) kfree(_T)) +
Peter,
Yuri Norov pointed out to me (under a different cleanup patch) that kfree() handles NULL-pointers and there's no reason to check it again in DEFINE_FREE() macros. It seems right to me but I wanted to run it by you and check if there is maybe some reason I'm not seeing to doing it?
Bartosz
[snip]
Re: [PATCH v3 03/57] locking: Introduce __cleanup() based infrastructure
In reply to Peter ZijlstraHi Peter,
include/linux/mutex.h extern void mutex_unlock(struct mutex *l · line 224
Quoted 3 lines · Peter Zijlstra
include/linux/mutex.h +2
-0+DEFINE_GUARD(mutex, struct mutex *, mutex_lock(_T), mutex_unlock(_T)) +DEFINE_FREE(mutex, struct mutex *, if (_T) mutex_unlock(_T))
I got excited with the ability to have mutexes released automatically, however the code I work with typically uses mutex_lock_interruptible() and friends, where resource acquisition may fail. Additionally, the return values are integers and not pointers, which makes them unsuitable for something like
struct mutex *guard __free(mutex) = mutex_lock_interruptible(...);
I guess we could make wrappers around mutex_lock...() family to return either the mutex or ERR_PTR-encoded error, but that feels quite ugly.
Do you have any suggestions here?
Thanks.
Signature · 1 line
Dmitry
Peter Zijlstra
maintainerPATCH v3 4/57 kbuild: Drop -Wdeclaration-after-statement
In reply to Peter ZijlstraWith the advent on scope-based resource management it comes really tedious to abide by the contraints of -Wdeclaration-after-statement.
It will still be recommeneded to place declarations at the start of a scope where possible, but it will no longer be enforced.
--- Makefile | 6 +----- arch/arm64/kernel/vdso32/Makefile | 2 -- 2 files changed, 1 insertion(+), 7 deletions(-)
Makefile +1 -5
@@ -447,8 +447,7 @@ HOSTRUSTC = rustc HOSTPKG_CONFIG = pkg-config KBUILD_USERHOSTCFLAGS := -Wall -Wmissing-prototypes -Wstrict-prototypes \ - -O2 -fomit-frame-pointer -std=gnu11 \ - -Wdeclaration-after-statement + -O2 -fomit-frame-pointer -std=gnu11 KBUILD_USERCFLAGS := $(KBUILD_USERHOSTCFLAGS) $(USERCFLAGS) KBUILD_USERLDFLAGS := $(USERLDFLAGS)
@@ -1012,9 +1011,6 @@ endif # arch Makefile may override CC so keep this after arch Makefile is included NOSTDINC_FLAGS += -nostdinc -# warn about C99 declaration after statement -KBUILD_CFLAGS += -Wdeclaration-after-statement - # Variable Length Arrays (VLAs) should not be used anywhere in the kernel KBUILD_CFLAGS += -Wvla
arch/arm64/kernel/vdso32/Makefile +0 -2
@@ -65,11 +65,9 @@ VDSO_CFLAGS += -Wall -Wundef -Wstrict-pr -fno-strict-aliasing -fno-common \ -Werror-implicit-function-declaration \ -Wno-format-security \ - -Wdeclaration-after-statement \ -std=gnu11 VDSO_CFLAGS += -O2 # Some useful compiler-dependent flags from top-level Makefile -VDSO_CFLAGS += $(call cc32-option,-Wdeclaration-after-statement,) VDSO_CFLAGS += $(call cc32-option,-Wno-pointer-sign) VDSO_CFLAGS += -fno-strict-overflow VDSO_CFLAGS += $(call cc32-option,-Werror=strict-prototypes)
Re: [PATCH v3 04/57] kbuild: Drop -Wdeclaration-after-statement
Quoted 3 lines · Peter Zijlstra
With the advent on scope-based resource management it comes really tedious to abide by the contraints of -Wdeclaration-after-statement.
Where is the context of Linus' suggested-by?
I do not know where this came from.
I suddenly got a huge v3 in my mailbox.
I see an equivalent patch submitted last year:
https://lore.kernel.org/lkml/Y1w031iI6Ld29IVT@p183/
Linus rejected it. Did he change his mind?
Quoted 2 lines
It will still be recommeneded to place declarations at the start of a scope where possible, but it will no longer be enforced.
If you remove the warning, we will not be able to detect code that opts out the recommendation for no good reason.
Signature · 2 lines
Best Regards Masahiro Yamada
Peter Zijlstra
maintainerRe: [PATCH v3 04/57] kbuild: Drop -Wdeclaration-after-statement
Quoted 7 lines · Masahiro Yamada
Quoted 3 lines · Peter Zijlstra
With the advent on scope-based resource management it comes really tedious to abide by the contraints of -Wdeclaration-after-statement.
Where is the context of Linus' suggested-by?
Quoted 8 lines
I do not know where this came from.
I suddenly got a huge v3 in my mailbox.I see an equivalent patch submitted last year:
https://lore.kernel.org/lkml/Y1w031iI6Ld29IVT@p183/Linus rejected it. Did he change his mind?
https://lkml.kernel.org/r/CAHk-%3Dwi-RyoUhbChiVaJZoZXheAwnJ7OO%3DGxe85BkPAd93TwDA%40mail.gmail.com
I'll add it as a Link tag to the Changelog.
Quoted 7 lines
Quoted 3 lines
It will still be recommeneded to place declarations at the start of a scope where possible, but it will no longer be enforced.
If you remove the warning, we will not be able to detect code that opts out the recommendation for no good reason.
Yeah, so per that thread linked above I tried to play clever games with _Pragma() to get around this, but GCC hates on it (works fine with Clang though).
Linus said to just give up and scrap the whole
-Wdeclaration-after-statement thing.
I suppose it'll be up to reviewers and perhaps checkpatch like things to 'enforce' the rules.
Peter Zijlstra
maintainerPATCH v3 5/57 sched: Simplify get_nohz_timer_target()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)
kernel/sched/core.c +6 -9
@@ -1097,25 +1097,22 @@ int get_nohz_timer_target(void) hk_mask =housekeeping_cpumask (HK_TYPE_TIMER ); -rcu_read_lock (); +guard (rcu)(); +for_each_domain (cpu, sd) {for_each_cpu_and (i,sched_domain_span (sd), hk_mask) {if (cpu == i)continue ; -if (!idle_cpu (i)) { - cpu = i; - goto unlock; - } +if (!idle_cpu (i)) +return i; } }if (default_cpu ==-1 ) default_cpu =housekeeping_any_cpu (HK_TYPE_TIMER ); - cpu = default_cpu; -unlock: -rcu_read_unlock (); -return cpu; + +return default_cpu; }/*
Peter Zijlstra
maintainerPATCH v3 6/57 sched: Simplify sysctl_sched_uclamp_handler()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)
kernel/sched/core.c +4 -7
@@ -1801,7 +1801,8 @@ static int sysctl_sched_uclamp_handler(sint old_min, old_max, old_min_rt;int result; -mutex_lock (&uclamp_mutex); +guard (mutex)(&uclamp_mutex); + old_min = sysctl_sched_uclamp_util_min; old_max = sysctl_sched_uclamp_util_max; old_min_rt = sysctl_sched_uclamp_util_min_rt_default;
@@ -1810,7 +1811,7 @@ static int sysctl_sched_uclamp_handler(sif (result) goto undo;if (!write) - goto done; +return result; if (sysctl_sched_uclamp_util_min > sysctl_sched_uclamp_util_max || sysctl_sched_uclamp_util_max > SCHED_CAPACITY_SCALE ||
@@ -1846,16 +1847,12 @@ static int sysctl_sched_uclamp_handler(s* Otherwise, keep it simple and do just a lazy update at each next * task enqueue time. */ - - goto done; +return result; undo: sysctl_sched_uclamp_util_min = old_min; sysctl_sched_uclamp_util_max = old_max; sysctl_sched_uclamp_util_min_rt_default = old_min_rt; -done: -mutex_unlock (&uclamp_mutex); - return result; }#endif
Re: [PATCH v3 06/57] sched: Simplify sysctl_sched_uclamp_handler()
kernel/sched/core.c static int sysctl_sched_uclamp_handler(s · line 1814
Quoted 6 lines · Peter Zijlstra
kernel/sched/core.c +1
-1@@ -1810,7 +1811,7 @@ static int sysctl_sched_uclamp_handler(sif (result) goto undo;if (!write) - goto done; +return result;
This would be nicer as a "return 0;"
kernel/sched/core.c static int sysctl_sched_uclamp_handler(s · line 1850
Quoted 10 lines
kernel/sched/core.c +1
-2if (sysctl_sched_uclamp_util_min > sysctl_sched_uclamp_util_max || sysctl_sched_uclamp_util_max > SCHED_CAPACITY_SCALE ||@@ -1846,16 +1847,12 @@ static int sysctl_sched_uclamp_handler(s* Otherwise, keep it simple and do just a lazy update at each next * task enqueue time. */ - - goto done; +return result;
This is a return 0 as well.
regards,
dan carpenter
Quoted 10 lines
kernel/sched/core.c +0
-3undo: sysctl_sched_uclamp_util_min = old_min; sysctl_sched_uclamp_util_max = old_max; sysctl_sched_uclamp_util_min_rt_default = old_min_rt; -done: -mutex_unlock (&uclamp_mutex); -return result; }
Peter Zijlstra
maintainerPATCH v3 7/57 sched: Simplify: migrate_swap_stop()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 23 +++++++---------------- kernel/sched/sched.h | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 16 deletions(-)
kernel/sched/core.c +7 -16
@@ -3258,7 +3258,6 @@ static int migrate_swap_stop(void *data) {struct migration_swap_arg *arg = data;struct rq *src_rq, *dst_rq; -int ret = -EAGAIN ;if (!cpu_active (arg->src_cpu) || !cpu_active (arg->dst_cpu))return -EAGAIN ;
@@ -3266,33 +3265,25 @@ static int migrate_swap_stop(void *data) src_rq =cpu_rq (arg->src_cpu); dst_rq =cpu_rq (arg->dst_cpu); -double_raw_lock (&arg->src_task->pi_lock, - &arg->dst_task->pi_lock); -double_rq_lock (src_rq, dst_rq); +guard (double_raw_spinlock)(&arg->src_task->pi_lock, &arg->dst_task->pi_lock); +guard (double_rq_lock)(src_rq, dst_rq);if (task_cpu (arg->dst_task) != arg->dst_cpu) - goto unlock; +return -EAGAIN ;if (task_cpu (arg->src_task) != arg->src_cpu) - goto unlock; +return -EAGAIN ;if (!cpumask_test_cpu (arg->dst_cpu, arg->src_task->cpus_ptr)) - goto unlock; +return -EAGAIN ;if (!cpumask_test_cpu (arg->src_cpu, arg->dst_task->cpus_ptr)) - goto unlock; +return -EAGAIN ;__migrate_swap_task (arg->src_task, arg->dst_cpu);__migrate_swap_task (arg->dst_task, arg->src_cpu); - ret =0 ; - -unlock: -double_rq_unlock (src_rq, dst_rq); -raw_spin_unlock (&arg->dst_task->pi_lock); -raw_spin_unlock (&arg->src_task->pi_lock); - -return ret; +return 0 ; }/*
kernel/sched/sched.h +20 -0
@@ -2572,6 +2572,12 @@ static inline void double_rq_clock_clearstatic inline void double_rq_clock_clear_update (struct rq *rq1,struct rq *rq2) {}#endif +#define DEFINE_LOCK_GUARD_2 (name, type, _lock, _unlock, ...) \ +__DEFINE_UNLOCK_GUARD(name, type, _unlock, type *lock2; __VA_ARGS__) \ +static inline class_##name##_t class_##name##_constructor(type *lock, type *lock2) \ +{ class_##name##_t _t = { .lock = lock, .lock2 = lock2 }, *_T = &_t; \ + _lock; return _t; } +#ifdef CONFIG_SMP static inline bool rq_order_less (struct rq *rq1,struct rq *rq2)
@@ -2701,6 +2707,16 @@ static inline void double_raw_lock(raw_sraw_spin_lock_nested (l2,SINGLE_DEPTH_NESTING ); } +static inline void double_raw_unlock (raw_spinlock_t *l1,raw_spinlock_t *l2) +{ +raw_spin_unlock (l1); +raw_spin_unlock (l2); +} + +DEFINE_LOCK_GUARD_2 (double_raw_spinlock, raw_spinlock_t, +double_raw_lock (_T->lock, _T->lock2), +double_raw_unlock (_T->lock, _T->lock2)) +/* * double_rq_unlock - safely unlock two runqueues *
@@ -2758,6 +2774,10 @@ static inline void double_rq_unlock(stru#endif +DEFINE_LOCK_GUARD_2(double_rq_lock, struct rq, +double_rq_lock (_T->lock, _T->lock2), + double_rq_unlock(_T->lock, _T->lock2)) +extern struct sched_entity *__pick_first_entity (struct cfs_rq *cfs_rq);extern struct sched_entity *__pick_last_entity (struct cfs_rq *cfs_rq);
Peter Zijlstra
maintainerPATCH v3 8/57 sched: Simplify wake_up_if_idle()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 20 ++++++-------------- kernel/sched/sched.h | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 14 deletions(-)
kernel/sched/core.c +6 -14
@@ -3872,21 +3872,13 @@ static void __ttwu_queue_wakelist(structvoid wake_up_if_idle (int cpu) {struct rq *rq =cpu_rq (cpu); -struct rq_flags rf; -rcu_read_lock (); - -if (!is_idle_task (rcu_dereference (rq->curr))) - goto out; - -rq_lock_irqsave (rq, &rf); -if (is_idle_task (rq->curr)) -resched_curr (rq); -/* Else CPU is not idle, do nothing here: */ -rq_unlock_irqrestore (rq, &rf); - -out: -rcu_read_unlock (); +guard (rcu)(); +if (is_idle_task (rcu_dereference (rq->curr))) { +guard (rq_lock)(rq); +if (is_idle_task (rq->curr)) +resched_curr (rq); + } }bool cpus_share_cache (int this_cpu,int that_cpu)
kernel/sched/sched.h +15 -0
@@ -1678,6 +1678,21 @@ rq_unlock(struct rq *rq, struct rq_flagsraw_spin_rq_unlock (rq); } +DEFINE_LOCK_GUARD_1(rq_lock, struct rq, +rq_lock (_T->lock, &_T->rf), +rq_unlock (_T->lock, &_T->rf), + struct rq_flags rf) + +DEFINE_LOCK_GUARD_1(rq_lock_irq, struct rq, +rq_lock_irq (_T->lock, &_T->rf), +rq_unlock_irq (_T->lock, &_T->rf), + struct rq_flags rf) + +DEFINE_LOCK_GUARD_1(rq_lock_irqsave, struct rq, +rq_lock_irqsave (_T->lock, &_T->rf), +rq_unlock_irqrestore (_T->lock, &_T->rf), + struct rq_flags rf) +static inline struct rq *this_rq_lock_irq (struct rq_flags *rf) __acquires(rq->lock)
Re: [PATCH v3 08/57] sched: Simplify wake_up_if_idle()
Quoted 22 lines · Peter Zijlstra
Use guards to reduce gotos and simplify control flow.
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/sched/core.c | 20 ++++++-------------- kernel/sched/sched.h | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 14 deletions(-)kernel/sched/core.c +0
-7@@ -3872,21 +3872,13 @@ static void __ttwu_queue_wakelist(structvoid wake_up_if_idle (int cpu) {struct rq *rq =cpu_rq (cpu); -struct rq_flags rf; -rcu_read_lock (); - -if (!is_idle_task (rcu_dereference (rq->curr))) - goto out; - -rq_lock_irqsave (rq, &rf);
kernel/sched/core.c static void __ttwu_queue_wakelist(struct · line 3878
Quoted 10 lines
kernel/sched/core.c +3
-7-if (is_idle_task (rq->curr)) -resched_curr (rq); -/* Else CPU is not idle, do nothing here: */ -rq_unlock_irqrestore (rq, &rf); - -out: -rcu_read_unlock (); +guard (rcu)(); +if (is_idle_task (rcu_dereference (rq->curr))) { +guard (rq_lock)(rq);
We assume that irq must be disabled when this function called? Otherwise, I don't understand why this is not
guard(rq_lock_irqsave)(rq);
?
Regards,
Boqun
Quoted 32 lines
kernel/sched/core.c +3
-0+if (is_idle_task (rq->curr)) +resched_curr (rq); + } }bool cpus_share_cache (int this_cpu,int that_cpu)kernel/sched/sched.h +15
-0@@ -1678,6 +1678,21 @@ rq_unlock(struct rq *rq, struct rq_flagsraw_spin_rq_unlock (rq); } +DEFINE_LOCK_GUARD_1(rq_lock, struct rq, +rq_lock (_T->lock, &_T->rf), +rq_unlock (_T->lock, &_T->rf), + struct rq_flags rf) + +DEFINE_LOCK_GUARD_1(rq_lock_irq, struct rq, +rq_lock_irq (_T->lock, &_T->rf), +rq_unlock_irq (_T->lock, &_T->rf), + struct rq_flags rf) + +DEFINE_LOCK_GUARD_1(rq_lock_irqsave, struct rq, +rq_lock_irqsave (_T->lock, &_T->rf), +rq_unlock_irqrestore (_T->lock, &_T->rf), + struct rq_flags rf) +static inline struct rq *this_rq_lock_irq (struct rq_flags *rf) __acquires(rq->lock)
Peter Zijlstra
maintainerRe: [PATCH v3 08/57] sched: Simplify wake_up_if_idle()
Quoted 37 lines · Boqun Feng
Quoted 22 lines · Peter Zijlstra
Use guards to reduce gotos and simplify control flow.
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/sched/core.c | 20 ++++++-------------- kernel/sched/sched.h | 15 +++++++++++++++ 2 files changed, 21 insertions(+), 14 deletions(-)kernel/sched/core.c +0
-7@@ -3872,21 +3872,13 @@ static void __ttwu_queue_wakelist(structvoid wake_up_if_idle (int cpu) {struct rq *rq =cpu_rq (cpu); -struct rq_flags rf; -rcu_read_lock (); - -if (!is_idle_task (rcu_dereference (rq->curr))) - goto out; - -rq_lock_irqsave (rq, &rf);kernel/sched/core.c static void __ttwu_queue_wakelist(struct · line 3878
Quoted 10 lines
kernel/sched/core.c +3
-7-if (is_idle_task (rq->curr)) -resched_curr (rq); -/* Else CPU is not idle, do nothing here: */ -rq_unlock_irqrestore (rq, &rf); - -out: -rcu_read_unlock (); +guard (rcu)(); +if (is_idle_task (rcu_dereference (rq->curr))) { +guard (rq_lock)(rq);We assume that irq must be disabled when this function called? Otherwise, I don't understand why this is not
Quoted 2 lines
guard(rq_lock_irqsave)(rq);
You're quite right, I messed that up.
Peter Zijlstra
maintainerPATCH v3 9/57 sched: Simplify ttwu()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 220 +++++++++++++++++++++++++--------------------------- 1 file changed, 108 insertions(+), 112 deletions(-)
kernel/sched/core.c +108 -112
@@ -3664,16 +3664,15 @@ ttwu_stat(struct task_struct *p, int cpu__schedstat_inc (p->stats.nr_wakeups_local); }else {struct sched_domain *sd; +guard (rcu)();__schedstat_inc (p->stats.nr_wakeups_remote); -rcu_read_lock ();for_each_domain (rq->cpu, sd) {if (cpumask_test_cpu (cpu,sched_domain_span (sd))) {__schedstat_inc (sd->ttwu_wake_remote);break ; } } -rcu_read_unlock (); }if (wake_flags &WF_MIGRATED )
@@ -4135,10 +4134,9 @@ bool ttwu_state_match(struct task_structstatic int try_to_wake_up (struct task_struct *p,unsigned int state,int wake_flags) { -unsigned long flags; +guard (preempt)();int cpu, success =0 ; -preempt_disable ();if (p == current) {/* * We're waking current, this means 'p->on_rq' and 'task_cpu(p)
@@ -4165,129 +4163,127 @@ try_to_wake_up(struct task_struct *p, un* reordered with p->state check below. This pairs with smp_store_mb() * in set_current_state() that the waiting thread does. */ -raw_spin_lock_irqsave (&p->pi_lock, flags); -smp_mb__after_spinlock (); -if (!ttwu_state_match (p, state, &success)) - goto unlock; + scoped_guard (raw_spinlock_irqsave, &p->pi_lock) { +smp_mb__after_spinlock (); +if (!ttwu_state_match (p, state, &success)) +break ; -trace_sched_waking (p); +trace_sched_waking (p); -/* -* Ensure we load p->on_rq _after_ p->state, otherwise it would -* be possible to, falsely, observe p->on_rq == 0 and get stuck -* in smp_cond_load_acquire() below. -* -* sched_ttwu_pending() try_to_wake_up() -* STORE p->on_rq = 1 LOAD p->state -* UNLOCK rq->lock -* -* __schedule() (switch to task 'p') -* LOCK rq->lock smp_rmb(); -* smp_mb__after_spinlock(); -* UNLOCK rq->lock -* -* [task p] -* STORE p->state = UNINTERRUPTIBLE LOAD p->on_rq -* -* Pairs with the LOCK+smp_mb__after_spinlock() on rq->lock in -* __schedule(). See the comment for smp_mb__after_spinlock(). -* -* A similar smb_rmb() lives in try_invoke_on_locked_down_task(). -*/ -smp_rmb (); -if (READ_ONCE (p->on_rq) &&ttwu_runnable (p, wake_flags)) - goto unlock; +/* +* Ensure we load p->on_rq _after_ p->state, otherwise it would +* be possible to, falsely, observe p->on_rq == 0 and get stuck +* in smp_cond_load_acquire() below. +* +* sched_ttwu_pending() try_to_wake_up() +* STORE p->on_rq = 1 LOAD p->state +* UNLOCK rq->lock +* +* __schedule() (switch to task 'p') +* LOCK rq->lock smp_rmb(); +* smp_mb__after_spinlock(); +* UNLOCK rq->lock +* +* [task p] +* STORE p->state = UNINTERRUPTIBLE LOAD p->on_rq +* +* Pairs with the LOCK+smp_mb__after_spinlock() on rq->lock in +* __schedule(). See the comment for smp_mb__after_spinlock(). +* +* A similar smb_rmb() lives in try_invoke_on_locked_down_task(). +*/ +smp_rmb (); +if (READ_ONCE (p->on_rq) &&ttwu_runnable (p, wake_flags)) +break ;#ifdef CONFIG_SMP -/* -* Ensure we load p->on_cpu _after_ p->on_rq, otherwise it would be -* possible to, falsely, observe p->on_cpu == 0. -* -* One must be running (->on_cpu == 1) in order to remove oneself -* from the runqueue. -* -* __schedule() (switch to task 'p') try_to_wake_up() -* STORE p->on_cpu = 1 LOAD p->on_rq -* UNLOCK rq->lock -* -* __schedule() (put 'p' to sleep) -* LOCK rq->lock smp_rmb(); -* smp_mb__after_spinlock(); -* STORE p->on_rq = 0 LOAD p->on_cpu -* -* Pairs with the LOCK+smp_mb__after_spinlock() on rq->lock in -* __schedule(). See the comment for smp_mb__after_spinlock(). -* -* Form a control-dep-acquire with p->on_rq == 0 above, to ensure -* schedule()'s deactivate_task() has 'happened' and p will no longer -* care about it's own p->state. See the comment in __schedule(). -*/ -smp_acquire__after_ctrl_dep (); +/* +* Ensure we load p->on_cpu _after_ p->on_rq, otherwise it would be +* possible to, falsely, observe p->on_cpu == 0. +* +* One must be running (->on_cpu == 1) in order to remove oneself +* from the runqueue. +* +* __schedule() (switch to task 'p') try_to_wake_up() +* STORE p->on_cpu = 1 LOAD p->on_rq +* UNLOCK rq->lock +* +* __schedule() (put 'p' to sleep) +* LOCK rq->lock smp_rmb(); +* smp_mb__after_spinlock(); +* STORE p->on_rq = 0 LOAD p->on_cpu +* +* Pairs with the LOCK+smp_mb__after_spinlock() on rq->lock in +* __schedule(). See the comment for smp_mb__after_spinlock(). +* +* Form a control-dep-acquire with p->on_rq == 0 above, to ensure +* schedule()'s deactivate_task() has 'happened' and p will no longer +* care about it's own p->state. See the comment in __schedule(). +*/ +smp_acquire__after_ctrl_dep (); -/* -* We're doing the wakeup (@success == 1), they did a dequeue (p->on_rq -* == 0), which means we need to do an enqueue, change p->state to -* TASK_WAKING such that we can unlock p->pi_lock before doing the -* enqueue, such as ttwu_queue_wakelist(). -*/ -WRITE_ONCE (p->__state,TASK_WAKING ); +/* +* We're doing the wakeup (@success == 1), they did a dequeue (p->on_rq +* == 0), which means we need to do an enqueue, change p->state to +* TASK_WAKING such that we can unlock p->pi_lock before doing the +* enqueue, such as ttwu_queue_wakelist(). +*/ +WRITE_ONCE (p->__state,TASK_WAKING ); -/* -* If the owning (remote) CPU is still in the middle of schedule() with -* this task as prev, considering queueing p on the remote CPUs wake_list -* which potentially sends an IPI instead of spinning on p->on_cpu to -* let the waker make forward progress. This is safe because IRQs are -* disabled and the IPI will deliver after on_cpu is cleared. -* -* Ensure we load task_cpu(p) after p->on_cpu: -* -* set_task_cpu(p, cpu); -* STORE p->cpu = @cpu -* __schedule() (switch to task 'p') -* LOCK rq->lock -* smp_mb__after_spin_lock() smp_cond_load_acquire(&p->on_cpu) -* STORE p->on_cpu = 1 LOAD p->cpu -* -* to ensure we observe the correct CPU on which the task is currently -* scheduling. -*/ -if (smp_load_acquire (&p->on_cpu) && -ttwu_queue_wakelist (p,task_cpu (p), wake_flags)) - goto unlock; +/* +* If the owning (remote) CPU is still in the middle of schedule() with +* this task as prev, considering queueing p on the remote CPUs wake_list +* which potentially sends an IPI instead of spinning on p->on_cpu to +* let the waker make forward progress. This is safe because IRQs are +* disabled and the IPI will deliver after on_cpu is cleared. +* +* Ensure we load task_cpu(p) after p->on_cpu: +* +* set_task_cpu(p, cpu); +* STORE p->cpu = @cpu +* __schedule() (switch to task 'p') +* LOCK rq->lock +* smp_mb__after_spin_lock() smp_cond_load_acquire(&p->on_cpu) +* STORE p->on_cpu = 1 LOAD p->cpu +* +* to ensure we observe the correct CPU on which the task is currently +* scheduling. +*/ +if (smp_load_acquire (&p->on_cpu) && +ttwu_queue_wakelist (p,task_cpu (p), wake_flags)) +break ; -/* -* If the owning (remote) CPU is still in the middle of schedule() with -* this task as prev, wait until it's done referencing the task. -* -* Pairs with the smp_store_release() in finish_task(). -* -* This ensures that tasks getting woken will be fully ordered against -* their previous state and preserve Program Order. -*/ -smp_cond_load_acquire (&p->on_cpu, !VAL ); +/* +* If the owning (remote) CPU is still in the middle of schedule() with +* this task as prev, wait until it's done referencing the task. +* +* Pairs with the smp_store_release() in finish_task(). +* +* This ensures that tasks getting woken will be fully ordered against +* their previous state and preserve Program Order. +*/ +smp_cond_load_acquire (&p->on_cpu, !VAL ); - cpu =select_task_rq (p, p->wake_cpu, wake_flags |WF_TTWU ); -if (task_cpu (p) != cpu) { -if (p->in_iowait) { -delayacct_blkio_end (p); -atomic_dec (&task_rq (p)->nr_iowait); - } + cpu =select_task_rq (p, p->wake_cpu, wake_flags |WF_TTWU ); +if (task_cpu (p) != cpu) { +if (p->in_iowait) { +delayacct_blkio_end (p); +atomic_dec (&task_rq (p)->nr_iowait); + } - wake_flags |=WF_MIGRATED ; -psi_ttwu_dequeue (p); -set_task_cpu (p, cpu); - } + wake_flags |=WF_MIGRATED ; +psi_ttwu_dequeue (p); +set_task_cpu (p, cpu); + }#else - cpu =task_cpu (p); + cpu =task_cpu (p);#endif /* CONFIG_SMP */ -ttwu_queue (p, cpu, wake_flags); -unlock: -raw_spin_unlock_irqrestore (&p->pi_lock, flags); +ttwu_queue (p, cpu, wake_flags); + } out:if (success)ttwu_stat (p,task_cpu (p), wake_flags); -preempt_enable ();return success; }
Re: [PATCH v3 09/57] sched: Simplify ttwu()
kernel/sched/core.c ttwu_stat(struct task_struct *p, int cpu · line 3669
Quoted 10 lines · Peter Zijlstra
kernel/sched/core.c +1
-1@@ -3664,16 +3664,15 @@ ttwu_stat(struct task_struct *p, int cpu__schedstat_inc (p->stats.nr_wakeups_local); }else {struct sched_domain *sd; +guard (rcu)();__schedstat_inc (p->stats.nr_wakeups_remote); -rcu_read_lock ();
We can't put the guard(rcu)(); here? I have unpublished static analysis which assumes that the first and last statements guarded by a lock are important. But if we always put it at the top of the scope then we lose that information.
kernel/sched/core.c ttwu_stat(struct task_struct *p, int cpu · line 3676
Quoted 8 lines
kernel/sched/core.c +0
-1for_each_domain (rq->cpu, sd) {if (cpumask_test_cpu (cpu,sched_domain_span (sd))) {__schedstat_inc (sd->ttwu_wake_remote);break ; } } -rcu_read_unlock (); }
regards,
dan carpenter
Peter Zijlstra
maintainerRe: [PATCH v3 09/57] sched: Simplify ttwu()
Quoted 9 lines · Dan Carpenter
Quoted 8 lines · Peter Zijlstra
kernel/sched/core.c +1
-0@@ -3664,16 +3664,15 @@ ttwu_stat(struct task_struct *p, int cpu__schedstat_inc (p->stats.nr_wakeups_local); }else {struct sched_domain *sd; +guard (rcu)();
Quoted 7 lines
kernel/sched/core.c ttwu_stat(struct task_struct *p, int cpu · line 3669
Quoted 2 lines
kernel/sched/core.c +0
-1__schedstat_inc (p->stats.nr_wakeups_remote); -rcu_read_lock ();We can't put the guard(rcu)(); here? I have unpublished static analysis which assumes that the first and last statements guarded by a lock are important. But if we always put it at the top of the scope then we lose that information.
we can definitely put it there. that one schedstat doesn't matter either way around.
Quoted 8 lines
Quoted 8 lines
kernel/sched/core.c +0
-1for_each_domain (rq->cpu, sd) {if (cpumask_test_cpu (cpu,sched_domain_span (sd))) {__schedstat_inc (sd->ttwu_wake_remote);break ; } } -rcu_read_unlock (); }
Peter Zijlstra
maintainerPATCH v3 10/57 sched: Simplify sched_exec()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
kernel/sched/core.c +9 -12
@@ -5431,23 +5431,20 @@ unsigned int nr_iowait(void)void sched_exec (void ) {struct task_struct *p = current; -unsigned long flags; +struct migration_arg arg;int dest_cpu; -raw_spin_lock_irqsave (&p->pi_lock, flags); - dest_cpu = p->sched_class->select_task_rq (p,task_cpu (p),WF_EXEC ); -if (dest_cpu ==smp_processor_id ()) - goto unlock; + scoped_guard (raw_spinlock_irqsave, &p->pi_lock) { + dest_cpu = p->sched_class->select_task_rq (p,task_cpu (p),WF_EXEC ); +if (dest_cpu ==smp_processor_id ()) +return ; -if (likely (cpu_active (dest_cpu))) { -struct migration_arg arg = { p, dest_cpu }; +if (unlikely (!cpu_active (dest_cpu))) +return ; -raw_spin_unlock_irqrestore (&p->pi_lock, flags); -stop_one_cpu (task_cpu (p), migration_cpu_stop, &arg); -return ; + arg = (struct migration_arg ){ p, dest_cpu }; } -unlock: -raw_spin_unlock_irqrestore (&p->pi_lock, flags); +stop_one_cpu (task_cpu (p), migration_cpu_stop, &arg); }#endif
Peter Zijlstra
maintainerPATCH v3 11/57 sched: Simplify sched_tick_remote()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 43 ++++++++++++++++++------------------------- 1 file changed, 18 insertions(+), 25 deletions(-)
kernel/sched/core.c +18 -25
@@ -5651,9 +5651,6 @@ static void sched_tick_remote(struct wor struct tick_work *twork = container_of(dwork, struct tick_work, work);int cpu = twork->cpu;struct rq *rq =cpu_rq (cpu); -struct task_struct *curr; -struct rq_flags rf; -u64 delta;int os;/*
@@ -5663,30 +5660,26 @@ static void sched_tick_remote(struct wor* statistics and checks timeslices in a time-independent way, regardless * of when exactly it is running. */ -if (!tick_nohz_tick_stopped_cpu (cpu)) - goto out_requeue; +if (tick_nohz_tick_stopped_cpu (cpu)) { +guard (rq_lock_irq)(rq); +struct task_struct *curr = rq->curr; + +if (cpu_online (cpu)) { +update_rq_clock (rq); + +if (!is_idle_task (curr)) { +/* +* Make sure the next tick runs within a +* reasonable amount of time. +*/ +u64 delta =rq_clock_task (rq) - curr->se.exec_start; +WARN_ON_ONCE (delta > (u64 )NSEC_PER_SEC *3 ); + } + curr->sched_class->task_tick (rq, curr,0 ); -rq_lock_irq (rq, &rf); - curr = rq->curr; -if (cpu_is_offline (cpu)) - goto out_unlock; - -update_rq_clock (rq); - -if (!is_idle_task (curr)) { -/* -* Make sure the next tick runs within a reasonable -* amount of time. -*/ - delta =rq_clock_task (rq) - curr->se.exec_start; -WARN_ON_ONCE (delta > (u64 )NSEC_PER_SEC *3 ); +calc_load_nohz_remote (rq); + } } - curr->sched_class->task_tick (rq, curr,0 ); - -calc_load_nohz_remote (rq); -out_unlock: -rq_unlock_irq (rq, &rf); -out_requeue:/* * Run the remote tick once per second (1Hz). This arbitrary
Peter Zijlstra
maintainerPATCH v3 12/57 sched: Simplify try_steal_cookie()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-)
kernel/sched/core.c +9 -12
@@ -6229,19 +6229,19 @@ static bool try_steal_cookie(int this, iunsigned long cookie;bool success = false; -local_irq_disable (); -double_rq_lock (dst, src); +guard (irq)(); +guard (double_rq_lock)(dst, src); cookie = dst->core->core_cookie;if (!cookie) - goto unlock; +return false;if (dst->curr != dst->idle) - goto unlock; +return false; p =sched_core_find (src, cookie);if (!p) - goto unlock; +return false; do { if (p == src->core_pick || p == src->curr)
@@ -6253,9 +6253,10 @@ static bool try_steal_cookie(int this, iif (p->core_occupation > dst->idle->core_occupation) goto next;/* -* sched_core_find() and sched_core_next() will ensure that task @p -* is not throttled now, we also need to check whether the runqueue -* of the destination CPU is being throttled. +* sched_core_find() and sched_core_next() will ensure +* that task @p is not throttled now, we also need to +* check whether the runqueue of the destination CPU is +* being throttled. */ if (sched_task_is_throttled (p, this)) goto next;
@@ -6273,10 +6274,6 @@ static bool try_steal_cookie(int this, i p =sched_core_next (p, cookie); }while (p); -unlock: -double_rq_unlock (dst, src); -local_irq_enable (); -return success; }
Peter Zijlstra
maintainerPATCH v3 13/57 sched: Simplify sched_core_cpu_{starting,deactivate}()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-)
kernel/sched/core.c +12 -15
@@ -6331,20 +6331,24 @@ static void queue_core_balance(struct rqqueue_balance_callback (rq, &per_cpu (core_balance_head, rq->cpu), sched_core_balance); } +DEFINE_LOCK_GUARD_1 (core_lock, int, +sched_core_lock (*_T->lock, &_T->flags), +sched_core_unlock (*_T->lock, &_T->flags), + unsigned long flags) +static void sched_core_cpu_starting (unsigned int cpu) {const struct cpumask *smt_mask =cpu_smt_mask (cpu);struct rq *rq =cpu_rq (cpu), *core_rq =NULL ; -unsigned long flags;int t; -sched_core_lock (cpu, &flags); +guard (core_lock)(&cpu);WARN_ON_ONCE (rq->core != rq);/* if we're the first, we'll be our own leader */ if (cpumask_weight (smt_mask) ==1 ) - goto unlock; +return ;/* find the leader */ for_each_cpu (t, smt_mask) {
@@ -6358,7 +6362,7 @@ static void sched_core_cpu_starting(unsi }if (WARN_ON_ONCE (!core_rq))/* whoopsie */ - goto unlock; +return ;/* install and validate core_rq */ for_each_cpu (t, smt_mask) {
@@ -6369,29 +6373,25 @@ static void sched_core_cpu_starting(unsiWARN_ON_ONCE (rq->core != core_rq); } - -unlock: -sched_core_unlock (cpu, &flags); }static void sched_core_cpu_deactivate (unsigned int cpu) {const struct cpumask *smt_mask =cpu_smt_mask (cpu);struct rq *rq =cpu_rq (cpu), *core_rq =NULL ; -unsigned long flags;int t; -sched_core_lock (cpu, &flags); +guard (core_lock)(&cpu);/* if we're the last man standing, nothing to do */ if (cpumask_weight (smt_mask) ==1 ) {WARN_ON_ONCE (rq->core != rq); - goto unlock; +return ; }/* if we're not the leader, nothing to do */ if (rq->core != rq) - goto unlock; +return ;/* find a new leader */ for_each_cpu (t, smt_mask) {
@@ -6402,7 +6402,7 @@ static void sched_core_cpu_deactivate(un }if (WARN_ON_ONCE (!core_rq))/* impossible */ - goto unlock; +return ;/* copy the shared state to the new leader */ core_rq->core_task_seq = rq->core_task_seq;
@@ -6424,9 +6424,6 @@ static void sched_core_cpu_deactivate(un rq =cpu_rq (t); rq->core = core_rq; } - -unlock: -sched_core_unlock (cpu, &flags); }static inline void sched_core_cpu_dying (unsigned int cpu)
Peter Zijlstra
maintainerPATCH v3 14/57 sched: Simplify set_user_nice()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 13 ++++++------- kernel/sched/sched.h | 5 +++++ 2 files changed, 11 insertions(+), 7 deletions(-)
kernel/sched/core.c +6 -7
@@ -7119,9 +7119,8 @@ static inline int rt_effective_prio(struvoid set_user_nice (struct task_struct *p,long nice) {bool queued, running; -int old_prio; -struct rq_flags rf;struct rq *rq; +int old_prio;if (task_nice (p) == nice || nice <MIN_NICE || nice >MAX_NICE )return ;
@@ -7129,7 +7128,9 @@ void set_user_nice(struct task_struct *p* We have to be careful, if called from sys_setpriority(), * the task might be in the middle of scheduling on another CPU. */ - rq =task_rq_lock (p, &rf); +CLASS (task_rq_lock, rq_guard)(p); + rq = rq_guard.rq; +update_rq_clock (rq);/*
@@ -7140,8 +7141,9 @@ void set_user_nice(struct task_struct *p*/ if (task_has_dl_policy (p) ||task_has_rt_policy (p)) { p->static_prio =NICE_TO_PRIO (nice); - goto out_unlock; +return ; } + queued =task_on_rq_queued (p); running =task_current (rq, p);if (queued)
@@ -7164,9 +7166,6 @@ void set_user_nice(struct task_struct *p* lowered its priority, then reschedule its CPU: */ p->sched_class->prio_changed (rq, p, old_prio); - -out_unlock: -task_rq_unlock (rq, p, &rf); }EXPORT_SYMBOL (set_user_nice);
kernel/sched/sched.h +5 -0
@@ -1630,6 +1630,11 @@ task_rq_unlock(struct rq *rq, struct tasraw_spin_unlock_irqrestore (&p->pi_lock, rf->flags); } +DEFINE_LOCK_GUARD_1(task_rq_lock, struct task_struct, + _T->rq =task_rq_lock (_T->lock, &_T->rf), +task_rq_unlock (_T->rq, _T->lock, &_T->rf), + struct rq *rq; struct rq_flags rf) + static inline void rq_lock_irqsave(struct rq *rq, struct rq_flags *rf) __acquires(rq->lock)
Peter Zijlstra
maintainerPATCH v3 15/57 sched: Simplify syscalls
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 154 ++++++++++++++++++++++------------------------------ 1 file changed, 68 insertions(+), 86 deletions(-)
kernel/sched/core.c +68 -86
@@ -7425,6 +7425,21 @@ static struct task_struct *find_process_return pid ?find_task_by_vpid (pid) : current; } +static struct task_struct *find_get_task (pid_t pid) +{ +struct task_struct *p; +guard (rcu)(); + + p =find_process_by_pid (pid); +if (likely (p)) +get_task_struct (p); + +return p; +} + +DEFINE_CLASS(find_get_task, struct task_struct *, if (_T) put_task_struct(_T), + find_get_task(pid), pid_t pid) +/* * sched_setparam() passes in -1 for its policy, to let the functions * it calls know not to change it.
@@ -7462,14 +7477,11 @@ static void __setscheduler_params(structstatic bool check_same_owner (struct task_struct *p) {const struct cred *cred =current_cred (), *pcred; -bool match; +guard (rcu)(); -rcu_read_lock (); pcred =__task_cred (p); - match = (uid_eq (cred->euid, pcred->euid) || -uid_eq (cred->euid, pcred->uid)); -rcu_read_unlock (); -return match; +return (uid_eq (cred->euid, pcred->euid) || +uid_eq (cred->euid, pcred->uid)); }/*
@@ -7873,27 +7885,17 @@ static int do_sched_setscheduler(pid_t pid, int policy, struct sched_param __user *param) { struct sched_param lparam; -struct task_struct *p; -int retval;if (!param || pid <0 )return -EINVAL ;if (copy_from_user (&lparam, param,sizeof (struct sched_param )))return -EFAULT ; -rcu_read_lock (); - retval = -ESRCH ; - p =find_process_by_pid (pid); -if (likely (p)) -get_task_struct (p); -rcu_read_unlock (); - -if (likely (p)) { - retval =sched_setscheduler (p, policy, &lparam); -put_task_struct (p); - } +CLASS (find_get_task, p)(pid); +if (!p) +return -ESRCH ; -return retval; +return sched_setscheduler (p, policy, &lparam); }/*
@@ -7989,7 +7991,6 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, pi unsigned int, flags) {struct sched_attr attr; -struct task_struct *p;int retval;if (!uattr || pid <0 || flags)
@@ -8004,21 +8005,14 @@ SYSCALL_DEFINE3(sched_setattr, pid_t, piif (attr.sched_flags &SCHED_FLAG_KEEP_POLICY ) attr.sched_policy =SETPARAM_POLICY ; -rcu_read_lock (); - retval = -ESRCH ; - p =find_process_by_pid (pid); -if (likely (p)) -get_task_struct (p); -rcu_read_unlock (); +CLASS (find_get_task, p)(pid); +if (!p) +return -ESRCH ; -if (likely (p)) { -if (attr.sched_flags &SCHED_FLAG_KEEP_PARAMS ) -get_params (p, &attr); - retval =sched_setattr (p, &attr); -put_task_struct (p); - } +if (attr.sched_flags &SCHED_FLAG_KEEP_PARAMS ) +get_params (p, &attr); -return retval; +return sched_setattr (p, &attr); }/**
@@ -8036,16 +8030,17 @@ SYSCALL_DEFINE1(sched_getscheduler, pid_if (pid <0 )return -EINVAL ; - retval = -ESRCH ; -rcu_read_lock (); +guard (rcu)(); p =find_process_by_pid (pid); -if (p) { - retval =security_task_getscheduler (p); -if (!retval) - retval = p->policy - | (p->sched_reset_on_fork ?SCHED_RESET_ON_FORK :0 ); +if (!p) +return -ESRCH ; + + retval =security_task_getscheduler (p); +if (!retval) { + retval = p->policy; +if (p->sched_reset_on_fork) + retval |=SCHED_RESET_ON_FORK ; } -rcu_read_unlock ();return retval; }
@@ -8066,30 +8061,23 @@ SYSCALL_DEFINE2(sched_getparam, pid_t, pif (!param || pid <0 )return -EINVAL ; -rcu_read_lock (); - p =find_process_by_pid (pid); - retval = -ESRCH ; -if (!p) - goto out_unlock; +scoped_guard (rcu) { + p =find_process_by_pid (pid); +if (!p) +return -ESRCH ; - retval =security_task_getscheduler (p); -if (retval) - goto out_unlock; + retval =security_task_getscheduler (p); +if (retval) +return retval; -if (task_has_rt_policy (p)) - lp.sched_priority = p->rt_priority; -rcu_read_unlock (); +if (task_has_rt_policy (p)) + lp.sched_priority = p->rt_priority; + }/* * This one might sleep, we cannot do it with a spinlock held ... */ - retval =copy_to_user (param, &lp,sizeof (*param)) ? -EFAULT :0 ; - -return retval; - -out_unlock: -rcu_read_unlock (); -return retval; +return copy_to_user (param, &lp,sizeof (*param)) ? -EFAULT :0 ; }/*
@@ -8149,39 +8137,33 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pi usize < SCHED_ATTR_SIZE_VER0 || flags)return -EINVAL ; -rcu_read_lock (); - p =find_process_by_pid (pid); - retval = -ESRCH ; -if (!p) - goto out_unlock; +scoped_guard (rcu) { + p =find_process_by_pid (pid); +if (!p) +return -ESRCH ; - retval =security_task_getscheduler (p); -if (retval) - goto out_unlock; + retval =security_task_getscheduler (p); +if (retval) +return retval; - kattr.sched_policy = p->policy; -if (p->sched_reset_on_fork) - kattr.sched_flags |=SCHED_FLAG_RESET_ON_FORK ; -get_params (p, &kattr); - kattr.sched_flags &=SCHED_FLAG_ALL ; + kattr.sched_policy = p->policy; +if (p->sched_reset_on_fork) + kattr.sched_flags |=SCHED_FLAG_RESET_ON_FORK ; +get_params (p, &kattr); + kattr.sched_flags &=SCHED_FLAG_ALL ;#ifdef CONFIG_UCLAMP_TASK -/* -* This could race with another potential updater, but this is fine -* because it'll correctly read the old or the new value. We don't need -* to guarantee who wins the race as long as it doesn't return garbage. -*/ - kattr.sched_util_min = p->uclamp_req[UCLAMP_MIN ].value; - kattr.sched_util_max = p->uclamp_req[UCLAMP_MAX ].value; +/* +* This could race with another potential updater, but this is fine +* because it'll correctly read the old or the new value. We don't need +* to guarantee who wins the race as long as it doesn't return garbage. +*/ + kattr.sched_util_min = p->uclamp_req[UCLAMP_MIN ].value; + kattr.sched_util_max = p->uclamp_req[UCLAMP_MAX ].value;#endif - -rcu_read_unlock (); + }return sched_attr_copy_to_user (uattr, &kattr, usize); - -out_unlock: -rcu_read_unlock (); -return retval; }#ifdef CONFIG_SMP
Peter Zijlstra
maintainerPATCH v3 16/57 sched: Simplify sched_{set,get}affinity()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 53 +++++++++++++--------------------------------------- 1 file changed, 14 insertions(+), 39 deletions(-)
kernel/sched/core.c +14 -39
@@ -8258,39 +8258,24 @@ long sched_setaffinity(pid_t pid, const {struct affinity_context ac;struct cpumask *user_mask; -struct task_struct *p;int retval; -rcu_read_lock (); - - p =find_process_by_pid (pid); -if (!p) { -rcu_read_unlock (); +CLASS (find_get_task, p)(pid); +if (!p)return -ESRCH ; - } -/* Prevent p going away */ -get_task_struct (p); -rcu_read_unlock (); - -if (p->flags &PF_NO_SETAFFINITY ) { - retval = -EINVAL ; - goto out_put_task; - } +if (p->flags &PF_NO_SETAFFINITY ) +return -EINVAL ;if (!check_same_owner (p)) { -rcu_read_lock (); -if (!ns_capable (__task_cred (p)->user_ns,CAP_SYS_NICE )) { -rcu_read_unlock (); - retval = -EPERM ; - goto out_put_task; - } -rcu_read_unlock (); +guard (rcu)(); +if (!ns_capable (__task_cred (p)->user_ns,CAP_SYS_NICE )) +return -EPERM ; } retval =security_task_setscheduler (p);if (retval) - goto out_put_task; +return retval;/* * With non-SMP configs, user_cpus_ptr/user_mask isn't used and
@@ -8300,8 +8285,7 @@ long sched_setaffinity(pid_t pid, constif (user_mask) {cpumask_copy (user_mask, in_mask); }else if (IS_ENABLED (CONFIG_SMP )) { - retval = -ENOMEM ; - goto out_put_task; +return -ENOMEM ; } ac = (struct affinity_context ){
@@ -8313,8 +8297,6 @@ long sched_setaffinity(pid_t pid, const retval =__sched_setaffinity (p, &ac);kfree (ac.user_mask); -out_put_task: -put_task_struct (p);return retval; }
@@ -8356,28 +8338,21 @@ SYSCALL_DEFINE3(sched_setaffinity, pid_tlong sched_getaffinity (pid_t pid,struct cpumask *mask) {struct task_struct *p; -unsigned long flags;int retval; -rcu_read_lock (); - - retval = -ESRCH ; +guard (rcu)(); p =find_process_by_pid (pid);if (!p) - goto out_unlock; +return -ESRCH ; retval =security_task_getscheduler (p);if (retval) - goto out_unlock; +return retval; -raw_spin_lock_irqsave (&p->pi_lock, flags); +guard (raw_spinlock_irqsave)(&p->pi_lock);cpumask_and (mask, &p->cpus_mask, cpu_active_mask); -raw_spin_unlock_irqrestore (&p->pi_lock, flags); -out_unlock: -rcu_read_unlock (); - -return retval; +return 0 ; }/**
Peter Zijlstra
maintainerPATCH v3 17/57 sched: Simplify yield_to()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 73 ++++++++++++++++++++++------------------------------ 1 file changed, 32 insertions(+), 41 deletions(-)
kernel/sched/core.c +32 -41
@@ -8799,55 +8799,46 @@ int __sched yield_to(struct task_struct {struct task_struct *curr = current;struct rq *rq, *p_rq; -unsigned long flags;int yielded =0 ; -local_irq_save (flags); - rq =this_rq (); +scoped_guard (irqsave) { + rq =this_rq (); again: - p_rq =task_rq (p); -/* -* If we're the only runnable task on the rq and target rq also -* has only one task, there's absolutely no point in yielding. -*/ -if (rq->nr_running ==1 && p_rq->nr_running ==1 ) { - yielded = -ESRCH ; - goto out_irq; - } - -double_rq_lock (rq, p_rq); -if (task_rq (p) != p_rq) { -double_rq_unlock (rq, p_rq); - goto again; - } - -if (!curr->sched_class->yield_to_task) - goto out_unlock; - -if (curr->sched_class != p->sched_class) - goto out_unlock; - -if (task_on_cpu (p_rq, p) || !task_is_running (p)) - goto out_unlock; - - yielded = curr->sched_class->yield_to_task (rq, p); -if (yielded) { -schedstat_inc (rq->yld_count); + p_rq =task_rq (p);/* -* Make p's CPU reschedule; pick_next_entity takes care of -* fairness. +* If we're the only runnable task on the rq and target rq also +* has only one task, there's absolutely no point in yielding. */ -if (preempt && rq != p_rq) -resched_curr (p_rq); - } +if (rq->nr_running ==1 && p_rq->nr_running ==1 ) +return -ESRCH ; -out_unlock: -double_rq_unlock (rq, p_rq); -out_irq: -local_irq_restore (flags); +guard (double_rq_lock)(rq, p_rq); +if (task_rq (p) != p_rq) + goto again; + +if (!curr->sched_class->yield_to_task) +return 0 ; + +if (curr->sched_class != p->sched_class) +return 0 ; + +if (task_on_cpu (p_rq, p) || !task_is_running (p)) +return 0 ; + + yielded = curr->sched_class->yield_to_task (rq, p); +if (yielded) { +schedstat_inc (rq->yld_count); +/* +* Make p's CPU reschedule; pick_next_entity +* takes care of fairness. +*/ +if (preempt && rq != p_rq) +resched_curr (p_rq); + } + } -if (yielded >0 ) +if (yielded)schedule ();return yielded;
Peter Zijlstra
maintainerPATCH v3 18/57 sched: Simplify sched_rr_get_interval()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-)
kernel/sched/core.c +16 -24
@@ -8941,38 +8941,30 @@ SYSCALL_DEFINE1(sched_get_priority_min,static int sched_rr_get_interval (pid_t pid,struct timespec64 *t) { -struct task_struct *p; -unsigned int time_slice; -struct rq_flags rf; -struct rq *rq; +unsigned int time_slice =0 ;int retval;if (pid <0 )return -EINVAL ; - retval = -ESRCH ; -rcu_read_lock (); - p =find_process_by_pid (pid); -if (!p) - goto out_unlock; - - retval =security_task_getscheduler (p); -if (retval) - goto out_unlock; - - rq =task_rq_lock (p, &rf); - time_slice =0 ; -if (p->sched_class->get_rr_interval) - time_slice = p->sched_class->get_rr_interval (rq, p); -task_rq_unlock (rq, p, &rf); +scoped_guard (rcu) { +struct task_struct *p =find_process_by_pid (pid); +if (!p) +return -ESRCH ; + + retval =security_task_getscheduler (p); +if (retval) +return retval; + + scoped_guard (task_rq_lock, p) { +struct rq *rq = scope.rq; +if (p->sched_class->get_rr_interval) + time_slice = p->sched_class->get_rr_interval (rq, p); + } + } -rcu_read_unlock ();jiffies_to_timespec64 (time_slice, t);return 0 ; - -out_unlock: -rcu_read_unlock (); -return retval; }/**
Peter Zijlstra
maintainerPATCH v3 19/57 sched: Simplify sched_move_task()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/sched/core.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
kernel/sched/core.c +4 -6
@@ -10361,17 +10361,18 @@ void sched_move_task(struct task_structint queued, running, queue_flags =DEQUEUE_SAVE |DEQUEUE_MOVE |DEQUEUE_NOCLOCK ;struct task_group *group; -struct rq_flags rf;struct rq *rq; - rq =task_rq_lock (tsk, &rf); +CLASS (task_rq_lock, rq_guard)(tsk); + rq = rq_guard.rq; +/* * Esp. with SCHED_AUTOGROUP enabled it is possible to get superfluous * group changes. */ group =sched_get_task_group (tsk);if (group == tsk->sched_task_group) - goto unlock; +return ;update_rq_clock (rq);
@@ -10396,9 +10397,6 @@ void sched_move_task(struct task_struct*/ resched_curr (rq); } - -unlock: -task_rq_unlock (rq, tsk, &rf); }static inline struct task_group *css_tg (struct cgroup_subsys_state *css)
Peter Zijlstra
maintainerPATCH v3 20/57 sched: Simplify tg_set_cfs_bandwidth()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- include/linux/cpu.h | 2 ++ kernel/sched/core.c | 42 +++++++++++++++++++++--------------------- 2 files changed, 23 insertions(+), 21 deletions(-)
include/linux/cpu.h +2 -0
@@ -148,6 +148,8 @@ static inline int remove_cpu(unsigned instatic inline void smp_shutdown_nonboot_cpus (unsigned int primary_cpu) { }#endif /* !CONFIG_HOTPLUG_CPU */ +DEFINE_LOCK_GUARD_0(cpus_read_lock, cpus_read_lock(), cpus_read_unlock()) +#ifdef CONFIG_PM_SLEEP_SMP extern int freeze_secondary_cpus (int primary);extern void thaw_secondary_cpus (void );
kernel/sched/core.c +21 -21
71 lines not on this page · raw message
Peter Zijlstra
maintainerPATCH v3 21/57 sched: Misc cleanups
In reply to Peter ZijlstraRandom remaining guard use...
--- kernel/sched/core.c | 163 ++++++++++++++++++++-------------------------------- 1 file changed, 63 insertions(+), 100 deletions(-)
kernel/sched/core.c +63 -100
@@ -1454,16 +1454,12 @@ static void __uclamp_update_util_min_rt_static void uclamp_update_util_min_rt_default (struct task_struct *p) { -struct rq_flags rf; -struct rq *rq; -if (!rt_task (p))return ;/* Protect updates to p->uclamp_* */ - rq =task_rq_lock (p, &rf); +guard (task_rq_lock)(p);__uclamp_update_util_min_rt_default (p); -task_rq_unlock (rq, p, &rf); } static inline struct uclamp_se
@@ -1759,9 +1755,8 @@ static void uclamp_update_root_tg(void)uclamp_se_set (&tg->uclamp_req[UCLAMP_MAX ], sysctl_sched_uclamp_util_max, false); -rcu_read_lock (); +guard (rcu)(); cpu_util_update_eff(&root_task_group.css); - rcu_read_unlock(); }#else static void uclamp_update_root_tg (void ) { }
@@ -1788,10 +1783,9 @@ static void uclamp_sync_util_min_rt_defasmp_mb__after_spinlock ();read_unlock (&tasklist_lock); -rcu_read_lock (); +guard (rcu)();for_each_process_thread (g, p)uclamp_update_util_min_rt_default (p); -rcu_read_unlock (); } static int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
@@ -2243,10 +2237,9 @@ void migrate_disable(void)return ; } -preempt_disable (); +guard (preempt)();this_rq ()->nr_pinned++; p->migration_disabled =1 ; -preempt_enable (); }EXPORT_SYMBOL_GPL (migrate_disable);
@@ -2270,7 +2263,7 @@ void migrate_enable(void)* Ensure stop_task runs either before or after this, and that * __set_cpus_allowed_ptr(SCA_MIGRATE_ENABLE) doesn't schedule(). */ -preempt_disable (); +guard (preempt)();if (p->cpus_ptr != &p->cpus_mask)__set_cpus_allowed_ptr (p, &ac);/*
@@ -2281,7 +2274,6 @@ void migrate_enable(void)barrier (); p->migration_disabled =0 ;this_rq ()->nr_pinned--; -preempt_enable (); }EXPORT_SYMBOL_GPL (migrate_enable);
@@ -3449,13 +3441,11 @@ unsigned long wait_task_inactive(struct*/ void kick_process (struct task_struct *p) { -int cpu; +guard (preempt)(); +int cpu =task_cpu (p); -preempt_disable (); - cpu =task_cpu (p);if ((cpu !=smp_processor_id ()) &&task_curr (p))smp_send_reschedule (cpu); -preempt_enable (); }EXPORT_SYMBOL_GPL (kick_process);
@@ -6300,8 +6290,9 @@ static void sched_core_balance(struct rqstruct sched_domain *sd;int cpu =cpu_of (rq); -preempt_disable (); -rcu_read_lock (); +guard (preempt)(); +guard (rcu)(); +raw_spin_rq_unlock_irq (rq);for_each_domain (cpu, sd) {if (need_resched ())
@@ -6311,8 +6302,6 @@ static void sched_core_balance(struct rqbreak ; }raw_spin_rq_lock_irq (rq); -rcu_read_unlock (); -preempt_enable (); } static DEFINE_PER_CPU(struct balance_callback, core_balance_head);
@@ -8169,8 +8158,6 @@ SYSCALL_DEFINE4(sched_getattr, pid_t, pi#ifdef CONFIG_SMP int dl_task_check_affinity (struct task_struct *p,const struct cpumask *mask) { -int ret =0 ; -/* * If the task isn't a deadline task or admission control is * disabled then we don't care about affinity changes.
@@ -8184,11 +8171,11 @@ int dl_task_check_affinity(struct task_s* tasks allowed to run on all the CPUs in the task's * root_domain. */ -rcu_read_lock (); +guard (rcu)();if (!cpumask_subset (task_rq (p)->rd->span, mask)) - ret = -EBUSY ; -rcu_read_unlock (); - return ret; +return -EBUSY ; + + return 0; }#endif
@@ -9197,10 +9184,8 @@ int task_can_attach(struct task_struct ** success of set_cpus_allowed_ptr() on all attached tasks * before cpus_mask may be changed. */ -if (p->flags &PF_NO_SETAFFINITY ) { - ret = -EINVAL ; - goto out; - } +if (p->flags &PF_NO_SETAFFINITY ) +return -EINVAL ;if (dl_task (p) && !cpumask_intersects (task_rq (p)->rd->span, cs_effective_cpus)) {
@@ -9211,7 +9196,6 @@ int task_can_attach(struct task_struct * ret =dl_cpu_busy (cpu, p); } -out:return ret; }
@@ -10433,11 +10417,9 @@ static int cpu_cgroup_css_online(struct#ifdef CONFIG_UCLAMP_TASK_GROUP /* Propagate the effective uclamp value for the new group */ -mutex_lock (&uclamp_mutex); -rcu_read_lock (); +guard (mutex)(&uclamp_mutex); +guard (rcu)();cpu_util_update_eff (css); -rcu_read_unlock (); -mutex_unlock (&uclamp_mutex);#endif return 0 ;
@@ -10588,8 +10570,8 @@ static ssize_t cpu_uclamp_write(struct kstatic_branch_enable (&sched_uclamp_used); -mutex_lock (&uclamp_mutex); -rcu_read_lock (); +guard (mutex)(&uclamp_mutex); +guard (rcu)(); tg =css_tg (of_css (of));if (tg->uclamp_req[clamp_id].value != req.util)
@@ -10604,9 +10586,6 @@ static ssize_t cpu_uclamp_write(struct k/* Update effective clamps to track the most restrictive value */ cpu_util_update_eff (of_css (of)); -rcu_read_unlock (); -mutex_unlock (&uclamp_mutex); -return nbytes; }
@@ -10632,10 +10611,10 @@ static inline void cpu_uclamp_print(struu64 percent;u32 rem; -rcu_read_lock (); - tg =css_tg (seq_css (sf)); - util_clamp = tg->uclamp_req[clamp_id].value; -rcu_read_unlock (); +scoped_guard (rcu) { + tg =css_tg (seq_css (sf)); + util_clamp = tg->uclamp_req[clamp_id].value; + }if (util_clamp ==SCHED_CAPACITY_SCALE ) {seq_puts (sf,"max\n" );
@@ -10952,7 +10931,6 @@ static int tg_cfs_schedulable_down(strucstatic int __cfs_schedulable (struct task_group *tg,u64 period,u64 quota) { -int ret;struct cfs_schedulable_data data = { .tg = tg, .period = period,
@@ -10964,11 +10942,8 @@ static int __cfs_schedulable(struct taskdo_div (data.quota,NSEC_PER_USEC ); } -rcu_read_lock (); - ret =walk_tg_tree (tg_cfs_schedulable_down, tg_nop, &data); -rcu_read_unlock (); - -return ret; +guard (rcu)(); +return walk_tg_tree (tg_cfs_schedulable_down, tg_nop, &data); }static int cpu_cfs_stat_show (struct seq_file *sf,void *v)
@@ -11529,14 +11504,12 @@ int __sched_mm_cid_migrate_from_fetch_ci* are not the last task to be migrated from this cpu for this mm, so * there is no need to move src_cid to the destination cpu. */ -rcu_read_lock (); +guard (rcu)(); src_task =rcu_dereference (src_rq->curr);if (READ_ONCE (src_task->mm_cid_active) && src_task->mm == mm) { -rcu_read_unlock (); t->last_mm_cid =-1 ;return -1 ; } -rcu_read_unlock ();return src_cid; }
@@ -11580,18 +11553,17 @@ int __sched_mm_cid_migrate_from_try_stea* the lazy-put flag, this task will be responsible for transitioning * from lazy-put flag set to MM_CID_UNSET. */ -rcu_read_lock (); - src_task =rcu_dereference (src_rq->curr); -if (READ_ONCE (src_task->mm_cid_active) && src_task->mm == mm) { -rcu_read_unlock (); -/* -* We observed an active task for this mm, there is therefore -* no point in moving this cid to the destination cpu. -*/ - t->last_mm_cid =-1 ; -return -1 ; +scoped_guard (rcu) { + src_task =rcu_dereference (src_rq->curr); +if (READ_ONCE (src_task->mm_cid_active) && src_task->mm == mm) { +/* +* We observed an active task for this mm, there is therefore +* no point in moving this cid to the destination cpu. +*/ + t->last_mm_cid =-1 ; +return -1 ; + } } -rcu_read_unlock ();/* * The src_cid is unused, so it can be unset.
@@ -11664,7 +11636,6 @@ static void sched_mm_cid_remote_clear(st {struct rq *rq =cpu_rq (cpu);struct task_struct *t; -unsigned long flags;int cid, lazy_cid; cid =READ_ONCE (pcpu_cid->cid);
@@ -11699,23 +11670,21 @@ static void sched_mm_cid_remote_clear(st* the lazy-put flag, that task will be responsible for transitioning * from lazy-put flag set to MM_CID_UNSET. */ -rcu_read_lock (); - t =rcu_dereference (rq->curr); -if (READ_ONCE (t->mm_cid_active) && t->mm == mm) { -rcu_read_unlock (); -return ; +scoped_guard (rcu) { + t =rcu_dereference (rq->curr); +if (READ_ONCE (t->mm_cid_active) && t->mm == mm) +return ; } -rcu_read_unlock ();/* * The cid is unused, so it can be unset. * Disable interrupts to keep the window of cid ownership without rq * lock small. */ -local_irq_save (flags); -if (try_cmpxchg (&pcpu_cid->cid, &lazy_cid,MM_CID_UNSET )) -__mm_cid_put (mm, cid); -local_irq_restore (flags); +scoped_guard (irqsave) { +if (try_cmpxchg (&pcpu_cid->cid, &lazy_cid,MM_CID_UNSET )) +__mm_cid_put (mm, cid); + } }static void sched_mm_cid_remote_clear_old (struct mm_struct *mm,int cpu)
@@ -11737,14 +11706,13 @@ static void sched_mm_cid_remote_clear_ol* snapshot associated with this cid if an active task using the mm is * observed on this rq. */ -rcu_read_lock (); - curr =rcu_dereference (rq->curr); -if (READ_ONCE (curr->mm_cid_active) && curr->mm == mm) { -WRITE_ONCE (pcpu_cid->time, rq_clock); -rcu_read_unlock (); -return ; +scoped_guard (rcu) { + curr =rcu_dereference (rq->curr); +if (READ_ONCE (curr->mm_cid_active) && curr->mm == mm) { +WRITE_ONCE (pcpu_cid->time, rq_clock); +return ; + } } -rcu_read_unlock ();if (rq_clock < pcpu_cid->time +SCHED_MM_CID_PERIOD_NS )return ;
@@ -11838,7 +11806,6 @@ void task_tick_mm_cid(struct rq *rq, strvoid sched_mm_cid_exit_signals (struct task_struct *t) {struct mm_struct *mm = t->mm; -struct rq_flags rf;struct rq *rq;if (!mm)
@@ -11846,7 +11813,7 @@ void sched_mm_cid_exit_signals(struct tapreempt_disable (); rq =this_rq (); -rq_lock_irqsave (rq, &rf); +guard (rq_lock_irqsave)(rq);preempt_enable_no_resched ();/* holding spinlock */ WRITE_ONCE (t->mm_cid_active,0 );/*
@@ -11856,13 +11823,11 @@ void sched_mm_cid_exit_signals(struct tasmp_mb ();mm_cid_put (mm); t->last_mm_cid = t->mm_cid =-1 ; -rq_unlock_irqrestore (rq, &rf); }void sched_mm_cid_before_execve (struct task_struct *t) {struct mm_struct *mm = t->mm; -struct rq_flags rf;struct rq *rq;if (!mm)
@@ -11870,7 +11835,7 @@ void sched_mm_cid_before_execve(struct tpreempt_disable (); rq =this_rq (); -rq_lock_irqsave (rq, &rf); +guard (rq_lock_irqsave)(rq);preempt_enable_no_resched ();/* holding spinlock */ WRITE_ONCE (t->mm_cid_active,0 );/*
@@ -11880,13 +11845,11 @@ void sched_mm_cid_before_execve(struct tsmp_mb ();mm_cid_put (mm); t->last_mm_cid = t->mm_cid =-1 ; -rq_unlock_irqrestore (rq, &rf); }void sched_mm_cid_after_execve (struct task_struct *t) {struct mm_struct *mm = t->mm; -struct rq_flags rf;struct rq *rq;if (!mm)
@@ -11894,16 +11857,16 @@ void sched_mm_cid_after_execve(struct tapreempt_disable (); rq =this_rq (); -rq_lock_irqsave (rq, &rf); -preempt_enable_no_resched ();/* holding spinlock */ -WRITE_ONCE (t->mm_cid_active,1 ); -/* -* Store t->mm_cid_active before loading per-mm/cpu cid. -* Matches barrier in sched_mm_cid_remote_clear_old(). -*/ -smp_mb (); - t->last_mm_cid = t->mm_cid =mm_cid_get (rq, mm); -rq_unlock_irqrestore (rq, &rf); + scoped_guard (rq_lock_irqsave, rq) { +preempt_enable_no_resched ();/* holding spinlock */ +WRITE_ONCE (t->mm_cid_active,1 ); +/* +* Store t->mm_cid_active before loading per-mm/cpu cid. +* Matches barrier in sched_mm_cid_remote_clear_old(). +*/ +smp_mb (); + t->last_mm_cid = t->mm_cid =mm_cid_get (rq, mm); + }rseq_set_notify_resume (t); }
Peter Zijlstra
maintainerPATCH v3 22/57 perf: Fix cpuctx refcounting
In reply to Peter Zijlstra--- include/linux/perf_event.h | 13 ++++++++----- kernel/events/core.c | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-)
include/linux/perf_event.h +8 -5
@@ -841,11 +841,11 @@ struct perf_event { };/* -* ,-----------------------[1:n]----------------------. -* V V -* perf_event_context <-[1:n]-> perf_event_pmu_context <--- perf_event -* ^ ^ | | -* `--------[1:n]---------' `-[n:1]-> pmu <-[1:n]-' +* ,-----------------------[1:n]------------------------. +* V V +* perf_event_context <-[1:n]-> perf_event_pmu_context <-[1:n]- perf_event +* | | +* `--[n:1]-> pmu <-[1:n]--' * * * struct perf_event_pmu_context lifetime is refcount based and RCU freed
@@ -863,6 +863,9 @@ struct perf_event {* ctx->mutex pinning the configuration. Since we hold a reference on * group_leader (through the filedesc) it can't go away, therefore it's * associated pmu_ctx must exist and cannot change due to ctx->mutex. +* +* perf_event holds a refcount on perf_event_context +* perf_event holds a refcount on perf_event_pmu_context */ struct perf_event_pmu_context {struct pmu *pmu;
kernel/events/core.c +16 -0
@@ -4809,6 +4809,11 @@ find_get_pmu_context(struct pmu *pmu, stvoid *task_ctx_data =NULL ;if (!ctx->task) { +/* +* perf_pmu_migrate_context() / __perf_pmu_install_event() +* relies on the fact that find_get_pmu_context() cannot fail +* for CPU contexts. +*/ struct perf_cpu_pmu_context *cpc; cpc =per_cpu_ptr (pmu->cpu_pmu_context, event->cpu);
@@ -12832,6 +12837,13 @@ static void __perf_pmu_install_event(str {struct perf_event_pmu_context *epc; +/* +* Now that the events are unused, put their old ctx and grab a +* reference on the new context. +*/ +put_ctx (event->ctx); +get_ctx (ctx); + event->cpu = cpu; epc =find_get_pmu_context (pmu, ctx, event); event->pmu_ctx = epc;
@@ -12877,6 +12889,10 @@ void perf_pmu_migrate_context(struct pmustruct perf_event_context *src_ctx, *dst_ctx;LIST_HEAD (events); +/* +* Since per-cpu context is persistent, no need to grab an extra +* reference. +*/ src_ctx = &per_cpu_ptr (&perf_cpu_context, src_cpu)->ctx; dst_ctx = &per_cpu_ptr (&perf_cpu_context, dst_cpu)->ctx;
[tip: perf/urgent] perf: Fix cpuctx refcounting
The following commit has been merged into the perf/urgent branch of tip:
The following commit has been merged into the perf/urgent branch of tip:
Commit-ID: cfaf1b99ac72d339c586d053885554dbe169d276 Gitweb: https://git.kernel.org/tip/cfaf1b99ac72d339c586d053885554dbe169d276 Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 09 Jun 2023 12:34:46 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Tue, 14 Nov 2023 22:27:28 +01:00
perf: Fix cpuctx refcounting
Audit of the refcounting turned up that perf_pmu_migrate_context() fails to migrate the ctx refcount.
--- include/linux/perf_event.h | 13 ++++++++----- kernel/events/core.c | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 5 deletions(-)
include/linux/perf_event.h +8 -5
@@ -843,11 +843,11 @@ struct perf_event { };/* -* ,-----------------------[1:n]----------------------. -* V V -* perf_event_context <-[1:n]-> perf_event_pmu_context <--- perf_event -* ^ ^ | | -* `--------[1:n]---------' `-[n:1]-> pmu <-[1:n]-' +* ,-----------------------[1:n]------------------------. +* V V +* perf_event_context <-[1:n]-> perf_event_pmu_context <-[1:n]- perf_event +* | | +* `--[n:1]-> pmu <-[1:n]--' * * * struct perf_event_pmu_context lifetime is refcount based and RCU freed
@@ -865,6 +865,9 @@ struct perf_event {* ctx->mutex pinning the configuration. Since we hold a reference on * group_leader (through the filedesc) it can't go away, therefore it's * associated pmu_ctx must exist and cannot change due to ctx->mutex. +* +* perf_event holds a refcount on perf_event_context +* perf_event holds a refcount on perf_event_pmu_context */ struct perf_event_pmu_context {struct pmu *pmu;
kernel/events/core.c +17 -0
@@ -4828,6 +4828,11 @@ find_get_pmu_context(struct pmu *pmu, struct perf_event_context *ctx,void *task_ctx_data =NULL ;if (!ctx->task) { +/* +* perf_pmu_migrate_context() / __perf_pmu_install_event() +* relies on the fact that find_get_pmu_context() cannot fail +* for CPU contexts. +*/ struct perf_cpu_pmu_context *cpc; cpc =per_cpu_ptr (pmu->cpu_pmu_context, event->cpu);
@@ -12897,6 +12902,9 @@ static void __perf_pmu_install_event(struct pmu *pmu, int cpu, struct perf_event *event) { struct perf_event_pmu_context *epc; +struct perf_event_context *old_ctx = event->ctx; + +get_ctx (ctx);/* normally find_get_context() */ event->cpu = cpu; epc =find_get_pmu_context (pmu, ctx, event);
@@ -12905,6 +12913,11 @@ static void __perf_pmu_install_event(struct pmu *pmu,if (event->state >=PERF_EVENT_STATE_OFF ) event->state =PERF_EVENT_STATE_INACTIVE ;perf_install_in_context (ctx, event, cpu); + +/* +* Now that event->ctx is updated and visible, put the old ctx. +*/ +put_ctx (old_ctx); } static void __perf_pmu_install(struct perf_event_context *ctx,
@@ -12943,6 +12956,10 @@ void perf_pmu_migrate_context(struct pmu *pmu, int src_cpu, int dst_cpu)struct perf_event_context *src_ctx, *dst_ctx;LIST_HEAD (events); +/* +* Since per-cpu context is persistent, no need to grab an extra +* reference. +*/ src_ctx = &per_cpu_ptr (&perf_cpu_context, src_cpu)->ctx; dst_ctx = &per_cpu_ptr (&perf_cpu_context, dst_cpu)->ctx;
[tip: perf/urgent] perf/core: Fix cpuctx refcounting
In reply to Peter ZijlstraThe following commit has been merged into the perf/urgent branch of tip:
The following commit has been merged into the perf/urgent branch of tip:
Commit-ID: 889c58b3155ff4c8e8671c95daef63d6fabbb6b1 Gitweb: https://git.kernel.org/tip/889c58b3155ff4c8e8671c95daef63d6fabbb6b1 Author: Peter Zijlstra <[email protected]> AuthorDate: Fri, 09 Jun 2023 12:34:46 +02:00 Committer: Ingo Molnar <[email protected]> CommitterDate: Wed, 15 Nov 2023 04:18:31 +01:00
perf/core: Fix cpuctx refcounting
Audit of the refcounting turned up that perf_pmu_migrate_context() fails to migrate the ctx refcount.
--- include/linux/perf_event.h | 13 ++++++++----- kernel/events/core.c | 17 +++++++++++++++++ 2 files changed, 25 insertions(+), 5 deletions(-)
include/linux/perf_event.h +8 -5
@@ -843,11 +843,11 @@ struct perf_event { };/* -* ,-----------------------[1:n]----------------------. -* V V -* perf_event_context <-[1:n]-> perf_event_pmu_context <--- perf_event -* ^ ^ | | -* `--------[1:n]---------' `-[n:1]-> pmu <-[1:n]-' +* ,-----------------------[1:n]------------------------. +* V V +* perf_event_context <-[1:n]-> perf_event_pmu_context <-[1:n]- perf_event +* | | +* `--[n:1]-> pmu <-[1:n]--' * * * struct perf_event_pmu_context lifetime is refcount based and RCU freed
@@ -865,6 +865,9 @@ struct perf_event {* ctx->mutex pinning the configuration. Since we hold a reference on * group_leader (through the filedesc) it can't go away, therefore it's * associated pmu_ctx must exist and cannot change due to ctx->mutex. +* +* perf_event holds a refcount on perf_event_context +* perf_event holds a refcount on perf_event_pmu_context */ struct perf_event_pmu_context {struct pmu *pmu;
kernel/events/core.c +17 -0
@@ -4828,6 +4828,11 @@ find_get_pmu_context(struct pmu *pmu, struct perf_event_context *ctx,void *task_ctx_data =NULL ;if (!ctx->task) { +/* +* perf_pmu_migrate_context() / __perf_pmu_install_event() +* relies on the fact that find_get_pmu_context() cannot fail +* for CPU contexts. +*/ struct perf_cpu_pmu_context *cpc; cpc =per_cpu_ptr (pmu->cpu_pmu_context, event->cpu);
@@ -12889,6 +12894,9 @@ static void __perf_pmu_install_event(struct pmu *pmu, int cpu, struct perf_event *event) { struct perf_event_pmu_context *epc; +struct perf_event_context *old_ctx = event->ctx; + +get_ctx (ctx);/* normally find_get_context() */ event->cpu = cpu; epc =find_get_pmu_context (pmu, ctx, event);
@@ -12897,6 +12905,11 @@ static void __perf_pmu_install_event(struct pmu *pmu,if (event->state >=PERF_EVENT_STATE_OFF ) event->state =PERF_EVENT_STATE_INACTIVE ;perf_install_in_context (ctx, event, cpu); + +/* +* Now that event->ctx is updated and visible, put the old ctx. +*/ +put_ctx (old_ctx); } static void __perf_pmu_install(struct perf_event_context *ctx,
@@ -12935,6 +12948,10 @@ void perf_pmu_migrate_context(struct pmu *pmu, int src_cpu, int dst_cpu)struct perf_event_context *src_ctx, *dst_ctx;LIST_HEAD (events); +/* +* Since per-cpu context is persistent, no need to grab an extra +* reference. +*/ src_ctx = &per_cpu_ptr (&perf_cpu_context, src_cpu)->ctx; dst_ctx = &per_cpu_ptr (&perf_cpu_context, dst_cpu)->ctx;
Peter Zijlstra
maintainerPATCH v3 23/57 perf: Simplify perf_event_alloc() error path
In reply to Peter ZijlstraThe error cleanup sequence in perf_event_alloc() is a subset of the existing _free_event() function (it must of course be).
Split this out into __free_event() and simplify the error path.
--- include/linux/perf_event.h | 1 kernel/events/core.c | 129 ++++++++++++++++++++++----------------------- 2 files changed, 66 insertions(+), 64 deletions(-)
include/linux/perf_event.h +1 -0
@@ -634,6 +634,7 @@ struct swevent_hlist {#define PERF_ATTACH_ITRACE 0x10#define PERF_ATTACH_SCHED_CB 0x20#define PERF_ATTACH_CHILD 0x40 +#define PERF_ATTACH_EXCLUSIVE 0x80struct bpf_prog ;struct perf_cgroup ;
kernel/events/core.c +65 -64
239 lines not on this page · raw message
Peter Zijlstra
maintainerPATCH v3 24/57 perf: Simplify perf_pmu_register() error path
In reply to Peter ZijlstraThe error path of perf_pmu_register() is of course very similar to a subset of perf_pmu_unregister(). Extract this common part in __perf_pmu_unregister() and simplify things.
--- kernel/events/core.c | 51 ++++++++++++++++++++++++--------------------------- 1 file changed, 24 insertions(+), 27 deletions(-)
kernel/events/core.c +24 -27
@@ -11426,20 +11426,35 @@ static int pmu_dev_alloc(struct pmu *pmustatic struct lock_class_key cpuctx_mutex;static struct lock_class_key cpuctx_lock; +static void __perf_pmu_unregister (struct pmu *pmu) +{ +free_percpu (pmu->pmu_disable_count); +if (pmu->type >=0 ) +idr_remove (&pmu_idr, pmu->type); +if (pmu_bus_running && pmu->dev && pmu->dev !=PMU_NULL_DEV ) { +if (pmu->nr_addr_filters) +device_remove_file (pmu->dev, &dev_attr_nr_addr_filters); +device_del (pmu->dev); +put_device (pmu->dev); + } +free_pmu_context (pmu); +} +int perf_pmu_register (struct pmu *pmu,const char *name,int type) {int cpu, ret, max =PERF_TYPE_MAX ; + pmu->type =-1 ; +mutex_lock (&pmus_lock); ret = -ENOMEM ; pmu->pmu_disable_count =alloc_percpu (int);if (!pmu->pmu_disable_count) goto unlock; - pmu->type =-1 ;if (WARN_ONCE (!name,"Can not register anonymous pmu.\n" )) { ret = -EINVAL ; - goto free_pdc; + goto free; } pmu->name = name;
@@ -11449,23 +11464,22 @@ int perf_pmu_register(struct pmu *pmu, c ret =idr_alloc (&pmu_idr, pmu, max,0 ,GFP_KERNEL );if (ret <0 ) - goto free_pdc; + goto free;WARN_ON (type >=0 && ret != type); - type = ret; - pmu->type = type; + pmu->type = ret;if (pmu_bus_running && !pmu->dev) { ret =pmu_dev_alloc (pmu);if (ret) - goto free_idr; + goto free; } ret = -ENOMEM ; pmu->cpu_pmu_context = alloc_percpu(struct perf_cpu_pmu_context);if (!pmu->cpu_pmu_context) - goto free_dev; + goto free;for_each_possible_cpu (cpu) {struct perf_cpu_pmu_context *cpc;
@@ -11511,17 +11525,8 @@ int perf_pmu_register(struct pmu *pmu, creturn ret; -free_dev: -if (pmu->dev && pmu->dev !=PMU_NULL_DEV ) { -device_del (pmu->dev); -put_device (pmu->dev); - } - -free_idr: -idr_remove (&pmu_idr, pmu->type); - -free_pdc: -free_percpu (pmu->pmu_disable_count); +free: +__perf_pmu_unregister (pmu); goto unlock; }EXPORT_SYMBOL_GPL (perf_pmu_register);
@@ -11538,15 +11543,7 @@ void perf_pmu_unregister(struct pmu *pmusynchronize_srcu (&pmus_srcu);synchronize_rcu (); -free_percpu (pmu->pmu_disable_count); -idr_remove (&pmu_idr, pmu->type); -if (pmu_bus_running && pmu->dev && pmu->dev !=PMU_NULL_DEV ) { -if (pmu->nr_addr_filters) -device_remove_file (pmu->dev, &dev_attr_nr_addr_filters); -device_del (pmu->dev); -put_device (pmu->dev); - } -free_pmu_context (pmu); +__perf_pmu_unregister (pmu);mutex_unlock (&pmus_lock); }EXPORT_SYMBOL_GPL (perf_pmu_unregister);
Peter Zijlstra
maintainerPATCH v3 25/57 perf: Simplify perf_fget_light()
In reply to Peter ZijlstraIntroduce fdnull and use it to simplify perf_fget_light() to either return a valid struct fd or not -- much like fdget() itself.
--- include/linux/file.h | 7 ++++++- kernel/events/core.c | 22 +++++++++++----------- 2 files changed, 17 insertions(+), 12 deletions(-)
include/linux/file.h +2 -0
@@ -59,6 +59,8 @@ static inline struct fd __to_fd(unsignedreturn (struct fd ){(struct file *)(v & ~3 ),v &3 }; } +#define fdnull __to_fd(0) +static inline struct fd fdget (unsigned int fd) {return __to_fd (__fdget (fd));
kernel/events/core.c +11 -11
@@ -5802,18 +5802,17 @@ EXPORT_SYMBOL_GPL(perf_event_period);static const struct file_operations perf_fops; -static inline int perf_fget_light (int fd,struct fd *p) +static inline struct fd perf_fdget (int fd) {struct fd f =fdget (fd);if (!f.file) -return -EBADF ; +return fdnull;if (f.file->f_op != &perf_fops) {fdput (f); -return -EBADF ; +return fdnull; } - *p = f; -return 0 ; +return f; } static int perf_event_set_output(struct perf_event *event,
@@ -5864,10 +5863,9 @@ static long _perf_ioctl(struct perf_evenint ret;if (arg !=-1 ) {struct perf_event *output_event; -struct fd output; - ret =perf_fget_light (arg, &output); -if (ret) -return ret; +struct fd output =perf_fdget (arg); +if (!output.file) +return -EBADF ; output_event = output.file->private_data; ret =perf_event_set_output (event, output_event);fdput (output);
@@ -12401,9 +12399,11 @@ SYSCALL_DEFINE5(perf_event_open,return event_fd;if (group_fd !=-1 ) { - err =perf_fget_light (group_fd, &group); -if (err) + group =perf_fdget (group_fd); +if (!group.file) { + err = -EBADF ; goto err_fd; + } group_leader = group.file->private_data;if (flags &PERF_FLAG_FD_OUTPUT ) output_event = group_leader;
Peter Zijlstra
maintainerPATCH v3 26/57 perf: Simplify event_function*()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/events/core.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-)
kernel/events/core.c +26 -13
@@ -214,6 +214,25 @@ struct event_function_struct {void *data; }; +typedef struct { +struct perf_cpu_context *cpuctx; +struct perf_event_context *ctx; +}class_perf_ctx_lock_t ; + +static inline void class_perf_ctx_lock_destructor (class_perf_ctx_lock_t *_T) +{ +if (_T->cpuctx) +perf_ctx_unlock (_T->cpuctx, _T->ctx); +} + +static inline class_perf_ctx_lock_t +class_perf_ctx_lock_constructor (struct perf_cpu_context *cpuctx, +struct perf_event_context *ctx) +{ +perf_ctx_lock (cpuctx, ctx); +return (class_perf_ctx_lock_t ){ cpuctx, ctx }; +} +static int event_function (void *info) {struct event_function_struct *efs = info;
@@ -224,17 +243,15 @@ static int event_function(void *info)int ret =0 ;lockdep_assert_irqs_disabled (); +guard (perf_ctx_lock)(cpuctx, task_ctx); -perf_ctx_lock (cpuctx, task_ctx);/* * Since we do the IPI call without holding ctx->lock things can have * changed, double check we hit the task we set out to hit. */ if (ctx->task) { -if (ctx->task != current) { - ret = -ESRCH ; - goto unlock; - } +if (ctx->task != current) +return -ESRCH ;/* * We only use event_function_call() on established contexts,
@@ -254,8 +271,6 @@ static int event_function(void *info) } efs->func (event, cpuctx, ctx, efs->data); -unlock: -perf_ctx_unlock (cpuctx, task_ctx);return ret; }
@@ -329,11 +344,11 @@ static void event_function_local(struct task_ctx = ctx; } -perf_ctx_lock (cpuctx, task_ctx); +guard (perf_ctx_lock)(cpuctx, task_ctx); task = ctx->task;if (task ==TASK_TOMBSTONE ) - goto unlock; +return ;if (task) {/*
@@ -343,18 +358,16 @@ static void event_function_local(struct*/ if (ctx->is_active) {if (WARN_ON_ONCE (task != current)) - goto unlock; +return ;if (WARN_ON_ONCE (cpuctx->task_ctx != ctx)) - goto unlock; +return ; } }else {WARN_ON_ONCE (&cpuctx->ctx != ctx); }func (event, cpuctx, ctx, data); -unlock: -perf_ctx_unlock (cpuctx, task_ctx); }#define PERF_FLAG_ALL (PERF_FLAG_FD_NO_GROUP |\
Re: [PATCH v3 26/57] perf: Simplify event_function*()
Quoted 22 lines · Peter Zijlstra
kernel/events/core.c +3
-5@@ -224,17 +243,15 @@ static int event_function(void *info)int ret =0 ;lockdep_assert_irqs_disabled (); +guard (perf_ctx_lock)(cpuctx, task_ctx); -perf_ctx_lock (cpuctx, task_ctx);/* * Since we do the IPI call without holding ctx->lock things can have * changed, double check we hit the task we set out to hit. */ if (ctx->task) { -if (ctx->task != current) { - ret = -ESRCH ; - goto unlock; - } +if (ctx->task != current) +return -ESRCH ;/* * We only use event_function_call() on established contexts,
kernel/events/core.c static int event_function(void *info) · line 275
Quoted 7 lines
kernel/events/core.c +0
-2@@ -254,8 +271,6 @@ static int event_function(void *info) } efs->func (event, cpuctx, ctx, efs->data); -unlock: -perf_ctx_unlock (cpuctx, task_ctx);return ret;
We can change this to a return 0; and get rid of the "ret" variable.
regards,
dan carpenter
Peter Zijlstra
maintainerRe: [PATCH v3 26/57] perf: Simplify event_function*()
Quoted 25 lines · Dan Carpenter
Quoted 24 lines · Peter Zijlstra
kernel/events/core.c +3
-5@@ -224,17 +243,15 @@ static int event_function(void *info)int ret =0 ;lockdep_assert_irqs_disabled (); +guard (perf_ctx_lock)(cpuctx, task_ctx); -perf_ctx_lock (cpuctx, task_ctx);/* * Since we do the IPI call without holding ctx->lock things can have * changed, double check we hit the task we set out to hit. */ if (ctx->task) { -if (ctx->task != current) { - ret = -ESRCH ; - goto unlock; - } +if (ctx->task != current) +return -ESRCH ;/* * We only use event_function_call() on established contexts, @@ -254,8 +271,6 @@ static int event_function(void *info) }
Quoted 8 lines
kernel/events/core.c static int event_function(void *info) · line 275
Quoted 5 lines
kernel/events/core.c +0
-2efs->func (event, cpuctx, ctx, efs->data); -unlock: -perf_ctx_unlock (cpuctx, task_ctx);return ret;We can change this to a return 0; and get rid of the "ret" variable.
This and the previous one, done!
Namhyung Kim
maintainerRe: [PATCH v3 26/57] perf: Simplify event_function*()
In reply to Peter ZijlstraHi Peter,
Quoted 13 lines · Peter Zijlstra
Use guards to reduce gotos and simplify control flow.
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/events/core.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-)kernel/events/core.c +0
-0@@ -214,6 +214,25 @@ struct event_function_struct {void *data; };
kernel/events/core.c struct event_function_struct { · line 225
Quoted 10 lines
kernel/events/core.c +9
-0+typedef struct { +struct perf_cpu_context *cpuctx; +struct perf_event_context *ctx; +}class_perf_ctx_lock_t ; + +static inline void class_perf_ctx_lock_destructor (class_perf_ctx_lock_t *_T) +{ +if (_T->cpuctx) +perf_ctx_unlock (_T->cpuctx, _T->ctx);
Shouldn't it be called unconditionally?
Thanks,
Namhyung
Quoted 80 lines
kernel/events/core.c +17
-13+} + +static inline class_perf_ctx_lock_t +class_perf_ctx_lock_constructor (struct perf_cpu_context *cpuctx, +struct perf_event_context *ctx) +{ +perf_ctx_lock (cpuctx, ctx); +return (class_perf_ctx_lock_t ){ cpuctx, ctx }; +} +static int event_function (void *info) {struct event_function_struct *efs = info;@@ -224,17 +243,15 @@ static int event_function(void *info)int ret =0 ;lockdep_assert_irqs_disabled (); +guard (perf_ctx_lock)(cpuctx, task_ctx); -perf_ctx_lock (cpuctx, task_ctx);/* * Since we do the IPI call without holding ctx->lock things can have * changed, double check we hit the task we set out to hit. */ if (ctx->task) { -if (ctx->task != current) { - ret = -ESRCH ; - goto unlock; - } +if (ctx->task != current) +return -ESRCH ;/* * We only use event_function_call() on established contexts, @@ -254,8 +271,6 @@ static int event_function(void *info) } efs->func (event, cpuctx, ctx, efs->data); -unlock: -perf_ctx_unlock (cpuctx, task_ctx);return ret; }@@ -329,11 +344,11 @@ static void event_function_local(struct task_ctx = ctx; } -perf_ctx_lock (cpuctx, task_ctx); +guard (perf_ctx_lock)(cpuctx, task_ctx); task = ctx->task;if (task ==TASK_TOMBSTONE ) - goto unlock; +return ;if (task) {/* @@ -343,18 +358,16 @@ static void event_function_local(struct*/ if (ctx->is_active) {if (WARN_ON_ONCE (task != current)) - goto unlock; +return ;if (WARN_ON_ONCE (cpuctx->task_ctx != ctx)) - goto unlock; +return ; } }else {WARN_ON_ONCE (&cpuctx->ctx != ctx); }func (event, cpuctx, ctx, data); -unlock: -perf_ctx_unlock (cpuctx, task_ctx); }#define PERF_FLAG_ALL (PERF_FLAG_FD_NO_GROUP |\
Peter Zijlstra
maintainerRe: [PATCH v3 26/57] perf: Simplify event_function*()
Quoted 17 lines · Namhyung Kim
Hi Peter,
Quoted 14 lines · Peter Zijlstra
Use guards to reduce gotos and simplify control flow.
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/events/core.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-)kernel/events/core.c +0
-0@@ -214,6 +214,25 @@ struct event_function_struct {void *data; };
Quoted 11 lines
kernel/events/core.c struct event_function_struct { · line 225
Quoted 9 lines
kernel/events/core.c +9
-0+typedef struct { +struct perf_cpu_context *cpuctx; +struct perf_event_context *ctx; +}class_perf_ctx_lock_t ; + +static inline void class_perf_ctx_lock_destructor (class_perf_ctx_lock_t *_T) +{ +if (_T->cpuctx) +perf_ctx_unlock (_T->cpuctx, _T->ctx);Shouldn't it be called unconditionally?
In all surviving cases it will be, so yeah, I can remove that condition.
Peter Zijlstra
maintainerPATCH v3 27/57 perf: Simplify perf_cgroup_connect()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- include/linux/file.h | 2 +- kernel/events/core.c | 19 ++++++++----------- 2 files changed, 9 insertions(+), 12 deletions(-)
kernel/events/core.c +8 -11
@@ -936,22 +936,20 @@ static inline int perf_cgroup_connect(in {struct perf_cgroup *cgrp;struct cgroup_subsys_state *css; -struct fd f =fdget (fd); -int ret =0 ; +int ret; +CLASS (fd, f)(fd);if (!f.file)return -EBADF ; css =css_tryget_online_from_dir (f.file->f_path.dentry, &perf_event_cgrp_subsys); -if (IS_ERR (css)) { - ret =PTR_ERR (css); - goto out; - } +if (IS_ERR (css)) +return PTR_ERR (css); ret =perf_cgroup_ensure_storage (event, css);if (ret) - goto out; +return ret; cgrp = container_of(css, struct perf_cgroup, css); event->cgrp = cgrp;
@@ -963,11 +961,10 @@ static inline int perf_cgroup_connect(in*/ if (group_leader && group_leader->cgrp != cgrp) {perf_detach_cgroup (event); - ret = -EINVAL ; +return -EINVAL ; } -out: -fdput (f); -return ret; + +return 0 ; } static inline void
Peter Zijlstra
maintainerPATCH v3 28/57 perf; Simplify event_sched_in()
In reply to Peter ZijlstraUse guards to reduce gotos and simplify control flow.
--- kernel/events/core.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
kernel/events/core.c +5 -8
@@ -1153,6 +1153,8 @@ void perf_pmu_enable(struct pmu *pmu) pmu->pmu_enable (pmu); } +DEFINE_GUARD(perf_pmu_disable, struct pmu *, perf_pmu_disable(_T), perf_pmu_enable(_T)) +static void perf_assert_pmu_disabled (struct pmu *pmu) {WARN_ON_ONCE (*this_cpu_ptr (pmu->pmu_disable_count) ==0 );
@@ -2489,7 +2491,6 @@ event_sched_in(struct perf_event *event, {struct perf_event_pmu_context *epc = event->pmu_ctx;struct perf_cpu_pmu_context *cpc =this_cpu_ptr (epc->pmu->cpu_pmu_context); -int ret =0 ;WARN_ON_ONCE (event->ctx != ctx);
@@ -2517,15 +2518,14 @@ event_sched_in(struct perf_event *event, event->hw.interrupts =0 ; } -perf_pmu_disable (event->pmu); +guard (perf_pmu_disable)(event->pmu);perf_log_itrace_start (event);if (event->pmu->add (event,PERF_EF_START )) {perf_event_set_state (event,PERF_EVENT_STATE_INACTIVE ); event->oncpu =-1 ; - ret = -EAGAIN ; - goto out; +return -EAGAIN ; }if (!is_software_event (event))
@@ -2536,10 +2536,7 @@ event_sched_in(struct perf_event *event,if (event->attr.exclusive) cpc->exclusive =1 ; -out: -perf_pmu_enable (event->pmu); - -return ret; +return 0 ; } static int
Re: [PATCH v3 28/57] perf; Simplify event_sched_in()
Quoted 1 line · Peter Zijlstra
Use guards to reduce gotos and simplify control flow.
Typoe in Subject: ";" should be ":"
Signature · 1 line
Kees Cook
Peter Zijlstra
maintainerPATCH v3 29/57 perf: Simplify: __perf_install_in_context()
In reply to Peter Zijlstra--- kernel/events/core.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
kernel/events/core.c +7 -14
@@ -2732,13 +2732,13 @@ static int __perf_install_in_context(vostruct perf_cpu_context *cpuctx =this_cpu_ptr (&perf_cpu_context);struct perf_event_context *task_ctx = cpuctx->task_ctx;bool reprogram = true; -int ret =0 ; -raw_spin_lock (&cpuctx->ctx.lock); -if (ctx->task) { -raw_spin_lock (&ctx->lock); +if (ctx->task) task_ctx = ctx; +guard (perf_ctx_lock)(cpuctx, task_ctx); + +if (ctx->task) { reprogram = (ctx->task == current);/*
@@ -2748,14 +2748,10 @@ static int __perf_install_in_context(vo* If its not running, we don't care, ctx->lock will * serialize against it becoming runnable. */ -if (task_curr (ctx->task) && !reprogram) { - ret = -ESRCH ; - goto unlock; - } +if (task_curr (ctx->task) && !reprogram) +return -ESRCH ;WARN_ON_ONCE (reprogram && cpuctx->task_ctx && cpuctx->task_ctx != ctx); - }else if (task_ctx) { -raw_spin_lock (&task_ctx->lock); }#ifdef CONFIG_CGROUP_PERF
@@ -2778,10 +2774,7 @@ static int __perf_install_in_context(voadd_event_to_ctx (event, ctx); } -unlock: -perf_ctx_unlock (cpuctx, task_ctx); - -return ret; +return 0 ; } static bool exclusive_event_installable(struct perf_event *event,
Peter Zijlstra
maintainerPATCH v3 30/57 perf: Simplify: *perf_event_{dis,en}able*()
In reply to Peter Zijlstra--- kernel/events/core.c | 51 ++++++++++++++++++++++----------------------------- 1 file changed, 22 insertions(+), 29 deletions(-)
kernel/events/core.c +22 -29
@@ -2415,7 +2415,7 @@ static void __perf_event_disable(structupdate_cgrp_time_from_event (event); } -perf_pmu_disable (event->pmu_ctx->pmu); +guard (perf_pmu_disable)(event->pmu_ctx->pmu);if (event == event->group_leader)group_sched_out (event, ctx);
@@ -2424,8 +2424,6 @@ static void __perf_event_disable(structperf_event_set_state (event,PERF_EVENT_STATE_OFF );perf_cgroup_event_disable (event, ctx); - -perf_pmu_enable (event->pmu_ctx->pmu); }/*
@@ -2446,12 +2444,10 @@ static void _perf_event_disable(struct p {struct perf_event_context *ctx = event->ctx; -raw_spin_lock_irq (&ctx->lock); -if (event->state <=PERF_EVENT_STATE_OFF ) { -raw_spin_unlock_irq (&ctx->lock); -return ; + scoped_guard (raw_spinlock_irq, &ctx->lock) { +if (event->state <=PERF_EVENT_STATE_OFF ) +return ; } -raw_spin_unlock_irq (&ctx->lock);event_function_call (event, __perf_event_disable,NULL ); }
@@ -2955,32 +2951,29 @@ static void _perf_event_enable(struct pe {struct perf_event_context *ctx = event->ctx; -raw_spin_lock_irq (&ctx->lock); -if (event->state >=PERF_EVENT_STATE_INACTIVE || - event->state <PERF_EVENT_STATE_ERROR ) { -out: -raw_spin_unlock_irq (&ctx->lock); -return ; - } + scoped_guard (raw_spinlock_irq, &ctx->lock) { +if (event->state >=PERF_EVENT_STATE_INACTIVE || + event->state <PERF_EVENT_STATE_ERROR ) +return ; -/* -* If the event is in error state, clear that first. -* -* That way, if we see the event in error state below, we know that it -* has gone back into error state, as distinct from the task having -* been scheduled away before the cross-call arrived. -*/ -if (event->state ==PERF_EVENT_STATE_ERROR ) {/* -* Detached SIBLING events cannot leave ERROR state. +* If the event is in error state, clear that first. +* +* That way, if we see the event in error state below, we know that it +* has gone back into error state, as distinct from the task having +* been scheduled away before the cross-call arrived. */ -if (event->event_caps &PERF_EV_CAP_SIBLING && - event->group_leader == event) - goto out; +if (event->state ==PERF_EVENT_STATE_ERROR ) { +/* +* Detached SIBLING events cannot leave ERROR state. +*/ +if (event->event_caps &PERF_EV_CAP_SIBLING && + event->group_leader == event) +return ; - event->state =PERF_EVENT_STATE_OFF ; + event->state =PERF_EVENT_STATE_OFF ; + } } -raw_spin_unlock_irq (&ctx->lock);event_function_call (event, __perf_event_enable,NULL ); }
Peter Zijlstra
maintainerPATCH v3 31/57 perf: Simplify perf_event_modify_attr()
In reply to Peter Zijlstra--- kernel/events/core.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
kernel/events/core.c +6 -6
@@ -3172,7 +3172,7 @@ static int perf_event_modify_attr(structWARN_ON_ONCE (event->ctx->parent_ctx); -mutex_lock (&event->child_mutex); +guard (mutex)(&event->child_mutex);/* * Event-type-independent attributes must be copied before event-type * modification, which will validate that final attributes match the
@@ -3181,16 +3181,16 @@ static int perf_event_modify_attr(structperf_event_modify_copy_attr (&event->attr, attr); err =func (event, attr);if (err) - goto out; +return err; +list_for_each_entry (child, &event->child_list, child_list) {perf_event_modify_copy_attr (&child->attr, attr); err =func (child, attr);if (err) - goto out; +return err; } -out: -mutex_unlock (&event->child_mutex); -return err; + +return 0 ; } static void __pmu_ctx_sched_out(struct perf_event_pmu_context *pmu_ctx,
Peter Zijlstra
maintainerPATCH v3 32/57 perf: Simplify perf_event_context_sched_in()
In reply to Peter Zijlstra--- kernel/events/core.c | 38 +++++++++++++++----------------------- 1 file changed, 15 insertions(+), 23 deletions(-)
kernel/events/core.c +15 -23
@@ -713,6 +713,9 @@ static void perf_ctx_enable(struct perf_perf_pmu_enable (pmu_ctx->pmu); } +DEFINE_GUARD(perf_ctx_disable, struct perf_event_context *, + perf_ctx_disable(_T), perf_ctx_enable(_T)) +static void ctx_sched_out (struct perf_event_context *ctx,enum event_type_t event_type);static void ctx_sched_in (struct perf_event_context *ctx,enum event_type_t event_type);
@@ -3906,31 +3909,27 @@ static void perf_event_context_sched_in(struct perf_cpu_context *cpuctx =this_cpu_ptr (&perf_cpu_context);struct perf_event_context *ctx; -rcu_read_lock (); +guard (rcu)(); + ctx =rcu_dereference (task->perf_event_ctxp);if (!ctx) - goto rcu_unlock; - -if (cpuctx->task_ctx == ctx) { -perf_ctx_lock (cpuctx, ctx); -perf_ctx_disable (ctx); - -perf_ctx_sched_task_cb (ctx, true); - -perf_ctx_enable (ctx); -perf_ctx_unlock (cpuctx, ctx); - goto rcu_unlock; - } +return ; -perf_ctx_lock (cpuctx, ctx); +guard (perf_ctx_lock)(cpuctx, ctx);/* * We must check ctx->nr_events while holding ctx->lock, such * that we serialize against perf_install_in_context(). */ if (!ctx->nr_events) - goto unlock; +return ; + +guard (perf_ctx_disable)(ctx); + +if (cpuctx->task_ctx == ctx) { +perf_ctx_sched_task_cb (ctx, true); +return ; + } -perf_ctx_disable (ctx);/* * We want to keep the following priority order: * cpu pinned (that don't need to move), task pinned,
@@ -3950,13 +3949,6 @@ static void perf_event_context_sched_in(if (!RB_EMPTY_ROOT (&ctx->pinned_groups.tree))perf_ctx_enable (&cpuctx->ctx); - -perf_ctx_enable (ctx); - -unlock: -perf_ctx_unlock (cpuctx, ctx); -rcu_unlock: -rcu_read_unlock (); }/*
Peter Zijlstra
maintainerPATCH v3 33/57 perf: Simplify perf_adjust_freq_unthr_context()
In reply to Peter Zijlstra--- kernel/events/core.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
kernel/events/core.c +3 -7
@@ -4100,7 +4100,7 @@ perf_adjust_freq_unthr_context(struct peif (!(ctx->nr_freq || unthrottle))return ; -raw_spin_lock (&ctx->lock); +guard (raw_spinlock)(&ctx->lock);list_for_each_entry_rcu (event, &ctx->event_list, event_entry) {if (event->state !=PERF_EVENT_STATE_ACTIVE )
@@ -4110,7 +4110,7 @@ perf_adjust_freq_unthr_context(struct peif (!event_filter_match (event))continue ; -perf_pmu_disable (event->pmu); +guard (perf_pmu_disable)(event->pmu); hwc = &event->hw;
@@ -4121,7 +4121,7 @@ perf_adjust_freq_unthr_context(struct pe }if (!event->attr.freq || !event->attr.sample_freq) - goto next; +continue ;/* * stop the event and update event->count
@@ -4143,11 +4143,7 @@ perf_adjust_freq_unthr_context(struct peperf_adjust_period (event, period, delta, false); event->pmu->start (event, delta >0 ?PERF_EF_RELOAD :0 ); - next: -perf_pmu_enable (event->pmu); } - -raw_spin_unlock (&ctx->lock); }/*
Re: [PATCH v3 33/57] perf: Simplify perf_adjust_freq_unthr_context()
This seems to have the same pattern that I *think* it's entirely broken, ie you are doing that
guard(perf_pmu_disable)(event->pmu);
inside a loop, and then you are randomly just removing the
perf_pmu_enable(event->pmu);
at the end of the loop, or when you do a "continue"./
That's not right.
The thing does not not go out of scope when the loop *iterates*.
It only goes out of scope when the loop *ends*.
Big difference as far as cleanup goes.
So you have not "simplified" the unlocking code, you've broken it. Now it no longer locks and unlocks for each iteration, it tries to re-lock every time.
Or have I mis-understood something completely?
Linus
Peter Zijlstra
maintainerRe: [PATCH v3 33/57] perf: Simplify perf_adjust_freq_unthr_context()
Quoted 5 lines · Linus Torvalds
The thing does not not go out of scope when the loop *iterates*.
It only goes out of scope when the loop *ends*.
Or have I mis-understood something completely?
I tried this before I used it and variables inside a for() loop have a scope of a single iteration.
$ gcc -O2 -o guard guard.c && ./guard spin_lock ponies __raw_spin_lock_irqsave can haz raw_spin_unlock_irqrestore mutex_lock mutex_unlock mutex_lock mutex_unlock mutex_lock mutex_unlock mutex_lock mutex_unlock mutex_lock mutex_unlock spin_unlock
---
#include <stdio.h>
#include <stdbool.h>
typedef struct {
} raw_spinlock_t;
typedef struct {
} spinlock_t;
typedef struct {
} mutex_t;
void raw_spin_lock(raw_spinlock_t *)
{
printf("%s\n", __FUNCTION__);
}void raw_spin_unlock(raw_spinlock_t *)
{
printf("%s\n", __FUNCTION__);
}unsigned long __raw_spin_lock_irqsave(raw_spinlock_t *lock)
{
printf("%s\n", __FUNCTION__);
return 0;
}#define raw_spin_lock_irqsave(lock, flags) \
flags = __raw_spin_lock_irqsave(lock)
void raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags)
{
printf("%s\n", __FUNCTION__);
}void spin_lock(spinlock_t *)
{
printf("%s\n", __FUNCTION__);
}void spin_unlock(spinlock_t *)
{
printf("%s\n", __FUNCTION__);
}void mutex_lock(mutex_t *)
{
printf("%s\n", __FUNCTION__);
}void mutex_unlock(mutex_t *)
{
printf("%s\n", __FUNCTION__);
}#define DEFINE_LOCK_GUARD(_type, _Type, _Lock, _Unlock, ...) \
typedef struct { \
_Type *lock; \
__VA_ARGS__ \
} lock_guard_##_type##_t; \
\
static inline void lock_guard_##_type##_cleanup(void *_g) \
{ \
lock_guard_##_type##_t *_G = _g; \
if (_G->lock) \
_Unlock; \
} \
\
static inline lock_guard_##_type##_t lock_guard_##_type##_init(_Type *lock) \
{ \
lock_guard_##_type##_t _g = { .lock = lock }, *_G = &_g; \
_Lock; \
return _g; \
}DEFINE_LOCK_GUARD(raw, raw_spinlock_t,
raw_spin_lock(_G->lock), raw_spin_unlock(_G->lock) )
DEFINE_LOCK_GUARD(spin, spinlock_t,
spin_lock(_G->lock), spin_unlock(_G->lock) )
DEFINE_LOCK_GUARD(mutex, mutex_t,
mutex_lock(_G->lock), mutex_unlock(_G->lock) )
DEFINE_LOCK_GUARD(raw_irqsave, raw_spinlock_t,
raw_spin_lock_irqsave(_G->lock, _G->flags), raw_spin_unlock_irqrestore(_G->lock, _G->flags), unsigned long flags; )
#define __cleanup(func) __attribute__((__cleanup__(func)))
#define lock_guard(_type, _name, _ptr) \ lock_guard_##_type##_t _name __cleanup(lock_guard_##_type##_cleanup) = \ lock_guard_##_type##_init(_ptr)
#define lock_scope(_type, _ptr) \
for (struct { lock_guard_##_type##_t guard ; bool done; } _scope __cleanup(lock_guard_##_type##_cleanup) = \
{ .guard = lock_guard_##_type##_init(_ptr), .done = false }; \
!_scope.done; _scope.done = true)raw_spinlock_t raw_lock;spinlock_t lock;mutex_t mutex;
void main(void)
{
lock_guard(spin, guard, &lock);
printf("ponies\n");
lock_scope(raw_irqsave, &raw_lock) {
printf("can haz\n");
}
for (int i=0; i<5; i++) {
lock_guard(mutex, foo, &mutex);
continue;
}
}Re: [PATCH v3 33/57] perf: Simplify perf_adjust_freq_unthr_context()
Quoted 3 lines · Peter Zijlstra
I tried this before I used it and variables inside a
for()loop have a scope of a single iteration.
Whee. Ok, that surprised me. But I guess that I shouldn't have found it surprising, since the value doesn't survive from one iteration to the next.
My mental picture of the scope was just different - and apparently wrong.
But thinking about it, it's not just that the value doesn't survive, it's also that the "continue" will exit the scope in order to go back to the "for()" loop.
I guess the "goto repeat" ends up being similar, since - as Ian Lance Taylor said in one of the earlier discussions - that "__cleanup__" ends up creating an implicit hidden scope for the variable. So a 'goto' to before the variable was declared implicitly exits the scope.
Ugh. I do really hate how subtle that is, though.
So while it might not be the horrible bug I thought it was, I'd _really_ like us to not do those things just from a sanity angle.
Linus
Peter Zijlstra
maintainerRe: [PATCH v3 33/57] perf: Simplify perf_adjust_freq_unthr_context()
Quoted 2 lines · Linus Torvalds
So while it might not be the horrible bug I thought it was, I'd _really_ like us to not do those things just from a sanity angle.
OK, let me go see if there's another way to sanely write those.
Is this less offensive?
restart:
scoped_guard (rcu) {
list_for_each_entry_rcu(..) {
...
if (err == -EAGAIN)
goto restart;
}
}That has the explicit scope on the rcu and now the goto explicitly exist it.
Gonna have to think what to do about the continue ...
Peter Zijlstra
maintainerRe: [PATCH v3 33/57] perf: Simplify perf_adjust_freq_unthr_context()
In reply to Linus TorvaldsQuoted 3 lines · Linus Torvalds
But thinking about it, it's not just that the value doesn't survive, it's also that the "continue" will exit the scope in order to go back to the "
for()" loop.
So if you still feel the continue is a step too far; the alternative isn't horrible either..
---
kernel/events/core.c +23 -28
@@ -4090,7 +4090,7 @@ perf_adjust_freq_unthr_context(struct peif (!(ctx->nr_freq || unthrottle))return ; -raw_spin_lock (&ctx->lock); +guard (raw_spinlock)(&ctx->lock);list_for_each_entry_rcu (event, &ctx->event_list, event_entry) {if (event->state !=PERF_EVENT_STATE_ACTIVE )
@@ -4100,7 +4100,7 @@ perf_adjust_freq_unthr_context(struct peif (!event_filter_match (event))continue ; -perf_pmu_disable (event->pmu); +guard (perf_pmu_disable)(event->pmu); hwc = &event->hw;
@@ -4110,34 +4110,29 @@ perf_adjust_freq_unthr_context(struct pe event->pmu->start (event,0 ); } -if (!event->attr.freq || !event->attr.sample_freq) - goto next; +if (event->attr.freq && event->attr.sample_freq) { +/* +* stop the event and update event->count +*/ + event->pmu->stop (event,PERF_EF_UPDATE ); + + now =local64_read (&event->count); + delta = now - hwc->freq_count_stamp; + hwc->freq_count_stamp = now; + +/* +* restart the event +* reload only if value has changed +* we have stopped the event so tell that +* to perf_adjust_period() to avoid stopping it +* twice. +*/ +if (delta >0 ) +perf_adjust_period (event, period, delta, false); -/* -* stop the event and update event->count -*/ - event->pmu->stop (event,PERF_EF_UPDATE ); - - now =local64_read (&event->count); - delta = now - hwc->freq_count_stamp; - hwc->freq_count_stamp = now; - -/* -* restart the event -* reload only if value has changed -* we have stopped the event so tell that -* to perf_adjust_period() to avoid stopping it -* twice. -*/ -if (delta >0 ) -perf_adjust_period (event, period, delta, false); - - event->pmu->start (event, delta >0 ?PERF_EF_RELOAD :0 ); - next: -perf_pmu_enable (event->pmu); + event->pmu->start (event, delta >0 ?PERF_EF_RELOAD :0 ); + } } - -raw_spin_unlock (&ctx->lock); }/*
Peter Zijlstra
maintainerPATCH v3 34/57 perf: Simplify perf_event_*_on_exec()
In reply to Peter Zijlstra--- kernel/events/core.c | 88 +++++++++++++++++++++++---------------------------- 1 file changed, 40 insertions(+), 48 deletions(-)
kernel/events/core.c +40 -48
@@ -4318,39 +4318,36 @@ static void perf_event_enable_on_exec(stenum event_type_t event_type =0 ;struct perf_cpu_context *cpuctx;struct perf_event *event; -unsigned long flags;int enabled =0 ; -local_irq_save (flags); -if (WARN_ON_ONCE (current->perf_event_ctxp != ctx)) - goto out; - -if (!ctx->nr_events) - goto out; - - cpuctx =this_cpu_ptr (&perf_cpu_context); -perf_ctx_lock (cpuctx, ctx); -ctx_sched_out (ctx,EVENT_TIME ); - -list_for_each_entry (event, &ctx->event_list, event_entry) { - enabled |=event_enable_on_exec (event, ctx); - event_type |=get_event_type (event); +scoped_guard (irqsave) { +if (WARN_ON_ONCE (current->perf_event_ctxp != ctx)) +return ; + +if (!ctx->nr_events) +return ; + + cpuctx =this_cpu_ptr (&perf_cpu_context); +guard (perf_ctx_lock)(cpuctx, ctx); + +ctx_sched_out (ctx,EVENT_TIME ); + +list_for_each_entry (event, &ctx->event_list, event_entry) { + enabled |=event_enable_on_exec (event, ctx); + event_type |=get_event_type (event); + } + +/* +* Unclone and reschedule this context if we enabled any event. +*/ +if (enabled) { + clone_ctx =unclone_ctx (ctx); +ctx_resched (cpuctx, ctx, event_type); + }else { +ctx_sched_in (ctx,EVENT_TIME ); + } } -/* -* Unclone and reschedule this context if we enabled any event. -*/ -if (enabled) { - clone_ctx =unclone_ctx (ctx); -ctx_resched (cpuctx, ctx, event_type); - }else { -ctx_sched_in (ctx,EVENT_TIME ); - } -perf_ctx_unlock (cpuctx, ctx); - -out: -local_irq_restore (flags); -if (clone_ctx)put_ctx (clone_ctx); }
@@ -4367,34 +4364,29 @@ static void perf_event_remove_on_exec(st {struct perf_event_context *clone_ctx =NULL ;struct perf_event *event, *next; -unsigned long flags;bool modified = false; -mutex_lock (&ctx->mutex); + scoped_guard (mutex, &ctx->mutex) { +if (WARN_ON_ONCE (ctx->task != current)) +return ; -if (WARN_ON_ONCE (ctx->task != current)) - goto unlock; +list_for_each_entry_safe (event, next, &ctx->event_list, event_entry) { +if (!event->attr.remove_on_exec) +continue ; -list_for_each_entry_safe (event, next, &ctx->event_list, event_entry) { -if (!event->attr.remove_on_exec) -continue ; +if (!is_kernel_event (event)) +perf_remove_from_owner (event); -if (!is_kernel_event (event)) -perf_remove_from_owner (event); + modified = true; - modified = true; +perf_event_exit_event (event, ctx); + } -perf_event_exit_event (event, ctx); +guard (raw_spinlock_irqsave)(&ctx->lock); +if (modified) + clone_ctx =unclone_ctx (ctx); } -raw_spin_lock_irqsave (&ctx->lock, flags); -if (modified) - clone_ctx =unclone_ctx (ctx); -raw_spin_unlock_irqrestore (&ctx->lock, flags); - -unlock: -mutex_unlock (&ctx->mutex); -if (clone_ctx)put_ctx (clone_ctx); }
Peter Zijlstra
maintainerPATCH v3 35/57 perf: Simplify *perf_event_read*()
In reply to Peter Zijlstra--- kernel/events/core.c | 54 ++++++++++++++++----------------------------------- 1 file changed, 17 insertions(+), 37 deletions(-)
kernel/events/core.c +17 -37
@@ -4435,7 +4435,8 @@ static void __perf_event_read(void *infoif (ctx->task && cpuctx->task_ctx != ctx)return ; -raw_spin_lock (&ctx->lock); +guard (raw_spinlock)(&ctx->lock); +if (ctx->is_active &EVENT_TIME ) {update_context_time (ctx);update_cgrp_time_from_event (event);
@@ -4446,12 +4447,12 @@ static void __perf_event_read(void *infoperf_event_update_sibling_time (event);if (event->state !=PERF_EVENT_STATE_ACTIVE ) - goto unlock; +return ;if (!data->group) { pmu->read (event); data->ret =0 ; - goto unlock; +return ; } pmu->start_txn (pmu,PERF_PMU_TXN_READ );
@@ -4469,9 +4470,6 @@ static void __perf_event_read(void *info } data->ret = pmu->commit_txn (pmu); - -unlock: -raw_spin_unlock (&ctx->lock); }static inline u64 perf_event_count (struct perf_event *event)
@@ -4502,43 +4500,32 @@ static void calc_timer_values(struct perint perf_event_read_local (struct perf_event *event,u64 *value,u64 *enabled,u64 *running) { -unsigned long flags; -int ret =0 ; -/* * Disabling interrupts avoids all counter scheduling (context * switches, timer based rotation and IPIs). */ -local_irq_save (flags); +guard (irqsave)();/* * It must not be an event with inherit set, we cannot read * all child counters from atomic context. */ -if (event->attr.inherit) { - ret = -EOPNOTSUPP ; - goto out; - } +if (event->attr.inherit) +return -EOPNOTSUPP ;/* If this is a per-task event, it must be for current */ if ((event->attach_state &PERF_ATTACH_TASK ) && - event->hw.target != current) { - ret = -EINVAL ; - goto out; - } + event->hw.target != current) +return -EINVAL ;/* If this is a per-CPU event, it must be for this CPU */ if (!(event->attach_state &PERF_ATTACH_TASK ) && - event->cpu !=smp_processor_id ()) { - ret = -EINVAL ; - goto out; - } + event->cpu !=smp_processor_id ()) +return -EINVAL ;/* If this is a pinned event it must be running on this CPU */ -if (event->attr.pinned && event->oncpu !=smp_processor_id ()) { - ret = -EBUSY ; - goto out; - } +if (event->attr.pinned && event->oncpu !=smp_processor_id ()) +return -EBUSY ;/* * If the event is currently on this CPU, its either a per-task event,
@@ -4558,10 +4545,8 @@ int perf_event_read_local(struct perf_evif (running) *running = __running; } -out: -local_irq_restore (flags); -return ret; +return 0 ; }static int perf_event_read (struct perf_event *event,bool group)
@@ -4595,7 +4580,7 @@ static int perf_event_read(struct perf_e .ret = 0, }; -preempt_disable (); +guard (preempt)(); event_cpu =__perf_event_read_cpu (event, event_cpu);/*
@@ -4609,19 +4594,15 @@ static int perf_event_read(struct perf_e* after this. */ (void )smp_call_function_single (event_cpu, __perf_event_read, &data,1 ); -preempt_enable (); ret = data.ret; }else if (state ==PERF_EVENT_STATE_INACTIVE ) {struct perf_event_context *ctx = event->ctx; -unsigned long flags; -raw_spin_lock_irqsave (&ctx->lock, flags); +guard (raw_spinlock_irqsave)(&ctx->lock); state = event->state; -if (state !=PERF_EVENT_STATE_INACTIVE ) { -raw_spin_unlock_irqrestore (&ctx->lock, flags); +if (state !=PERF_EVENT_STATE_INACTIVE ) goto again; - }/* * May read while context is not active (e.g., thread is
@@ -4635,7 +4616,6 @@ static int perf_event_read(struct perf_eperf_event_update_time (event);if (group)perf_event_update_sibling_time (event); -raw_spin_unlock_irqrestore (&ctx->lock, flags); }return ret;
Peter Zijlstra
maintainerPATCH v3 36/57 perf: Simplify find_get_pmu_context()
In reply to Peter Zijlstra--- kernel/events/core.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
kernel/events/core.c +9 -10
@@ -4757,11 +4757,14 @@ find_get_context(struct task_struct *tasreturn ERR_PTR (err); } +/* +* Returns a matching perf_event_pmu_context with elevated refcount or NULL. +*/ static struct perf_event_pmu_context *find_get_pmu_context (struct pmu *pmu,struct perf_event_context *ctx,struct perf_event *event) { -struct perf_event_pmu_context *new =NULL , *epc; +struct perf_event_pmu_context *epc;void *task_ctx_data =NULL ;if (!ctx->task) {
@@ -4788,16 +4791,14 @@ find_get_pmu_context(struct pmu *pmu, streturn epc; } - new =kzalloc (sizeof (*epc),GFP_KERNEL ); + void *new __free(kfree) = kzalloc(sizeof(*epc), GFP_KERNEL);if (!new) -return ERR_PTR (-ENOMEM ); +return NULL ;if (event->attach_state &PERF_ATTACH_TASK_DATA ) { task_ctx_data =alloc_task_ctx_data (pmu); -if (!task_ctx_data) { -kfree (new); -return ERR_PTR (-ENOMEM ); - } +if (!task_ctx_data) +return NULL ; }__perf_init_event_pmu_context (new, pmu);
@@ -4820,8 +4821,7 @@ find_get_pmu_context(struct pmu *pmu, st } } - epc = new; - new =NULL ; + epc =no_free_ptr (new);list_add (&epc->pmu_ctx_entry, &ctx->pmu_ctx_list); epc->ctx = ctx;
@@ -4835,7 +4835,6 @@ find_get_pmu_context(struct pmu *pmu, straw_spin_unlock_irq (&ctx->lock);free_task_ctx_data (pmu, task_ctx_data); -kfree (new);return epc; }
Peter Zijlstra
maintainerPATCH v3 37/57 perf: Simplify perf_read_group()
In reply to Peter Zijlstra--- kernel/events/core.c | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-)
kernel/events/core.c +11 -19
@@ -5472,11 +5472,10 @@ static int perf_read_group(struct perf_estruct perf_event *leader = event->group_leader, *child;struct perf_event_context *ctx = leader->ctx;int ret; -u64 *values;lockdep_assert_held (&ctx->mutex); - values =kzalloc (event->read_size,GFP_KERNEL ); + u64 *values __free(kfree) = kzalloc(event->read_size, GFP_KERNEL);if (!values)return -ENOMEM ;
@@ -5486,29 +5485,22 @@ static int perf_read_group(struct perf_e* By locking the child_mutex of the leader we effectively * lock the child list of all siblings.. XXX explain how. */ -mutex_lock (&leader->child_mutex); - - ret =__perf_read_group_add (leader, read_format, values); -if (ret) - goto unlock; - -list_for_each_entry (child, &leader->child_list, child_list) { - ret =__perf_read_group_add (child, read_format, values); + scoped_guard (mutex, &leader->child_mutex) { + ret =__perf_read_group_add (leader, read_format, values);if (ret) - goto unlock; - } +return ret; -mutex_unlock (&leader->child_mutex); +list_for_each_entry (child, &leader->child_list, child_list) { + ret =__perf_read_group_add (child, read_format, values); +if (ret) +return ret; + } + } ret = event->read_size;if (copy_to_user (buf, values, event->read_size)) - ret = -EFAULT ; - goto out; +return -EFAULT ; -unlock: -mutex_unlock (&leader->child_mutex); -out: -kfree (values);return ret; }
Peter Zijlstra
maintainerPATCH v3 38/57 perf: Simplify IOC_SET_OUTPUT
In reply to Peter Zijlstra--- kernel/events/core.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
kernel/events/core.c +9 -8
@@ -5762,6 +5762,11 @@ static inline struct fd perf_fdget(int freturn f; } +static inline bool is_perf_fd (struct fd fd) +{ +return fd.file && fd.file->f_op == &perf_fops; +} +static int perf_event_set_output (struct perf_event *event,struct perf_event *output_event);static int perf_event_set_filter (struct perf_event *event,void __user *arg);
@@ -5807,19 +5812,15 @@ static long _perf_ioctl(struct perf_evencase PERF_EVENT_IOC_SET_OUTPUT : { -int ret;if (arg !=-1 ) {struct perf_event *output_event; -struct fd output =perf_fdget (arg); -if (!output.file) +CLASS (fd, output)(arg); +if (!is_perf_fd (output))return -EBADF ; output_event = output.file->private_data; - ret =perf_event_set_output (event, output_event); -fdput (output); - }else { - ret =perf_event_set_output (event,NULL ); +return perf_event_set_output (event, output_event); } -return ret; +return perf_event_set_output (event,NULL ); }case PERF_EVENT_IOC_SET_FILTER :
Peter Zijlstra
maintainerPATCH v3 39/57 perf: Simplify perf_event_*_userpage()
In reply to Peter Zijlstra--- kernel/events/core.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-)
kernel/events/core.c +10 -20
@@ -5971,10 +5971,10 @@ static void perf_event_init_userpage(strstruct perf_event_mmap_page *userpg;struct perf_buffer *rb; -rcu_read_lock (); +guard (rcu)(); rb =rcu_dereference (event->rb);if (!rb) - goto unlock; +return ; userpg = rb->user_page;
@@ -5983,9 +5983,6 @@ static void perf_event_init_userpage(str userpg->size = offsetof(struct perf_event_mmap_page , __reserved); userpg->data_offset =PAGE_SIZE ; userpg->data_size =perf_data_size (rb); - -unlock: -rcu_read_unlock (); } void __weak arch_perf_update_userpage(
@@ -6004,10 +6001,10 @@ void perf_event_update_userpage(struct pstruct perf_buffer *rb;u64 enabled, running, now; -rcu_read_lock (); +guard (rcu)(); rb =rcu_dereference (event->rb);if (!rb) - goto unlock; +return ;/* * compute total_time_enabled, total_time_running
@@ -6025,7 +6022,7 @@ void perf_event_update_userpage(struct p* Disable preemption to guarantee consistent time stamps are stored to * the user page. */ -preempt_disable (); +guard (preempt)(); ++userpg->lock;barrier (); userpg->index =perf_event_index (event);
@@ -6043,9 +6040,6 @@ void perf_event_update_userpage(struct pbarrier (); ++userpg->lock; -preempt_enable (); -unlock: -rcu_read_unlock (); }EXPORT_SYMBOL_GPL (perf_event_update_userpage);
@@ -6061,27 +6055,23 @@ static vm_fault_t perf_mmap_fault(structreturn ret; } -rcu_read_lock (); +guard (rcu)(); rb =rcu_dereference (event->rb);if (!rb) - goto unlock; +return ret;if (vmf->pgoff && (vmf->flags &FAULT_FLAG_WRITE )) - goto unlock; +return ret; vmf->page =perf_mmap_to_page (rb, vmf->pgoff);if (!vmf->page) - goto unlock; +return ret;get_page (vmf->page); vmf->page->mapping = vmf->vma->vm_file->f_mapping; vmf->page->index = vmf->pgoff; - ret =0 ; -unlock: -rcu_read_unlock (); - -return ret; +return 0 ; } static void ring_buffer_attach(struct perf_event *event,
Peter Zijlstra
maintainerPATCH v3 40/57 perf: Simplify perf_mmap_close()/perf_aux_sample_output()
In reply to Peter Zijlstra--- kernel/events/core.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
kernel/events/core.c +8 -12
@@ -6179,6 +6179,9 @@ void ring_buffer_put(struct perf_buffercall_rcu (&rb->rcu_head, rb_free_rcu); } +DEFINE_CLASS(ring_buffer_get, struct perf_buffer *, ring_buffer_put(_T), + ring_buffer_get(event), struct perf_event *event) +static void perf_mmap_open (struct vm_area_struct *vma) {struct perf_event *event = vma->vm_file->private_data;
@@ -6206,7 +6209,7 @@ static void perf_pmu_output_stop(structstatic void perf_mmap_close (struct vm_area_struct *vma) {struct perf_event *event = vma->vm_file->private_data; -struct perf_buffer *rb =ring_buffer_get (event); +CLASS (ring_buffer_get, rb)(event);struct user_struct *mmap_user = rb->mmap_user;int mmap_locked = rb->mmap_locked;unsigned long size =perf_data_size (rb);
@@ -6245,14 +6248,14 @@ static void perf_mmap_close(struct vm_ar detach_rest = true;if (!atomic_dec_and_mutex_lock (&event->mmap_count, &event->mmap_mutex)) - goto out_put; +return ;ring_buffer_attach (event,NULL );mutex_unlock (&event->mmap_mutex);/* If there's still other mmap()s of this buffer, we're done. */ if (!detach_rest) - goto out_put; +return ;/* * No other mmap()s, detach from all other events that might redirect
@@ -6309,9 +6312,6 @@ static void perf_mmap_close(struct vm_ar &mmap_user->locked_vm);atomic64_sub (mmap_locked, &vma->vm_mm->pinned_vm);free_uid (mmap_user); - -out_put: -ring_buffer_put (rb);/* could be last */ }static const struct vm_operations_struct perf_mmap_vmops = {
@@ -6962,14 +6962,13 @@ static void perf_aux_sample_output(struc struct perf_sample_data *data) {struct perf_event *sampler = event->aux_event; -struct perf_buffer *rb;unsigned long pad;long size;if (WARN_ON_ONCE (!sampler || !data->aux_size))return ; - rb =ring_buffer_get (sampler); +CLASS (ring_buffer_get, rb)(sampler);if (!rb)return ;
@@ -6982,7 +6981,7 @@ static void perf_aux_sample_output(struc* like to know. */ if (WARN_ON_ONCE (size <0 )) - goto out_put; +return ;/* * The pad comes from ALIGN()ing data->aux_size up to u64 in
@@ -6996,9 +6995,6 @@ static void perf_aux_sample_output(strucu64 zero =0 ;perf_output_copy (handle, &zero, pad); } - -out_put: -ring_buffer_put (rb); }/*
Peter Zijlstra
maintainerPATCH v3 41/57 perf: Simplify __perf_event_output()
In reply to Peter Zijlstra--- kernel/events/core.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)
kernel/events/core.c +3 -8
@@ -7739,22 +7739,17 @@ __perf_event_output(struct perf_event *eint err;/* protect the callchain buffers */ -rcu_read_lock (); +guard (rcu)();perf_prepare_sample (data, event, regs);perf_prepare_header (&header, data, event, regs); - err =output_begin (&handle, data, event, header.size);if (err) - goto exit; - +return err;perf_output_sample (&handle, &header, data, event); -perf_output_end (&handle); -exit: -rcu_read_unlock (); -return err; +return 0 ; }void
Peter Zijlstra
maintainerPATCH v3 42/57 perf: Simplify perf_iterate_sb()
In reply to Peter Zijlstra--- kernel/events/core.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
kernel/events/core.c +3 -6
@@ -7871,8 +7871,8 @@ perf_iterate_sb(perf_iterate_f output, v {struct perf_event_context *ctx; -rcu_read_lock (); -preempt_disable (); +guard (rcu)(); +guard (preempt)();/* * If we have task_ctx != NULL we only notify the task context itself.
@@ -7881,7 +7881,7 @@ perf_iterate_sb(perf_iterate_f output, v*/ if (task_ctx) {perf_iterate_ctx (task_ctx, output, data, false); - goto done; +return ; }perf_iterate_sb_cpu (output, data);
@@ -7889,9 +7889,6 @@ perf_iterate_sb(perf_iterate_f output, v ctx =rcu_dereference (current->perf_event_ctxp);if (ctx)perf_iterate_ctx (ctx, output, data, false); -done: -preempt_enable (); -rcu_read_unlock (); }/*
Peter Zijlstra
maintainerPATCH v3 43/57 perf: Simplify perf_sw_event()
In reply to Peter Zijlstra--- kernel/events/core.c | 37 ++++++++++++------------------------- 1 file changed, 12 insertions(+), 25 deletions(-)
kernel/events/core.c +12 -25
@@ -9701,17 +9701,15 @@ static void do_perf_sw_event(enum perf_tstruct perf_event *event;struct hlist_head *head; -rcu_read_lock (); +guard (rcu)(); head =find_swevent_head_rcu (swhash, type, event_id);if (!head) - goto end; +return ;hlist_for_each_entry_rcu (event, head, hlist_entry) {if (perf_swevent_match (event, type, event_id, data, regs))perf_swevent_event (event, nr, data, regs); } -end: -rcu_read_unlock (); } DEFINE_PER_CPU(struct pt_regs, __perf_regs[4]);
@@ -9746,16 +9744,13 @@ void __perf_sw_event(u32 event_id, u64 n {int rctx; -preempt_disable_notrace (); +guard (preempt_notrace)(); rctx =perf_swevent_get_recursion_context ();if (unlikely (rctx <0 )) - goto fail; +return ;___perf_sw_event (event_id, nr, regs, addr); -perf_swevent_put_recursion_context (rctx); -fail: -preempt_enable_notrace (); }static void perf_swevent_read (struct perf_event *event)
@@ -9844,21 +9839,17 @@ static int swevent_hlist_get_cpu(int cpustruct swevent_htable *swhash = &per_cpu (swevent_htable, cpu);int err =0 ; -mutex_lock (&swhash->hlist_mutex); +guard (mutex)(&swhash->hlist_mutex);if (!swevent_hlist_deref (swhash) &&cpumask_test_cpu (cpu, perf_online_mask)) {struct swevent_hlist *hlist; hlist =kzalloc (sizeof (*hlist),GFP_KERNEL ); -if (!hlist) { - err = -ENOMEM ; - goto exit; - } +if (!hlist) +return -ENOMEM ;rcu_assign_pointer (swhash->swevent_hlist, hlist); } swhash->hlist_refcount++; -exit: -mutex_unlock (&swhash->hlist_mutex);return err; }
@@ -10115,16 +10106,12 @@ void perf_tp_event(u16 event_type, u64 cif (task && task != current) {struct perf_event_context *ctx; -rcu_read_lock (); +guard (rcu)(); ctx =rcu_dereference (task->perf_event_ctxp); -if (!ctx) - goto unlock; - -raw_spin_lock (&ctx->lock); -perf_tp_event_target_task (count, record, regs, &data, ctx); -raw_spin_unlock (&ctx->lock); -unlock: -rcu_read_unlock (); +if (ctx) { +guard (raw_spinlock)(&ctx->lock); +perf_tp_event_target_task (count, record, regs, &data, ctx); + } }perf_swevent_put_recursion_context (rctx);
Peter Zijlstra
maintainerPATCH v3 44/57 perf: Simplify bpf_overflow_handler()
In reply to Peter Zijlstra--- kernel/events/core.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
kernel/events/core.c +7 -9
@@ -10288,16 +10288,14 @@ static void bpf_overflow_handler(structint ret =0 ; ctx.regs =perf_arch_bpf_user_pt_regs (regs); -if (unlikely (__this_cpu_inc_return (bpf_prog_active) !=1 )) - goto out; -rcu_read_lock (); - prog =READ_ONCE (event->prog); -if (prog) { -perf_prepare_sample (data, event, regs); - ret =bpf_prog_run (prog, &ctx); +if (likely (__this_cpu_inc_return (bpf_prog_active) ==1 )) { +guard (rcu)(); + prog =READ_ONCE (event->prog); +if (prog) { +perf_prepare_sample (data, event, regs); + ret =bpf_prog_run (prog, &ctx); + } } -rcu_read_unlock (); -out:__this_cpu_dec (bpf_prog_active);if (!ret)return ;
Peter Zijlstra
maintainerPATCH v3 45/57 perf: Simplify perf_event_parse_addr_filter()
In reply to Peter ZijlstraXXX this code needs a cleanup
--- kernel/events/core.c | 56 ++++++++++++++++++++------------------------------- 1 file changed, 22 insertions(+), 34 deletions(-)
kernel/events/core.c +22 -34
@@ -10495,6 +10495,8 @@ static void free_filters_list(struct lis } } +DEFINE_FREE(filter_list, struct list_head *, if (_T) free_filters_list(_T)) +/* * Free existing address filters and optionally install new ones */
@@ -10658,13 +10660,15 @@ perf_event_parse_addr_filter(struct perf struct list_head *filters) {struct perf_addr_filter *filter =NULL ; -char *start, *orig, *filename =NULL ;substring_t args[MAX_OPT_ARGS ];int state =IF_STATE_ACTION , token;unsigned int kernel =0 ; -int ret = -EINVAL ; +char *start; +int ret; - orig = fstr =kstrdup (fstr,GFP_KERNEL ); + struct list_head *fguard __free(filter_list) = filters; + char *filename __free(kfree) = NULL; + char *orig __free(kfree) = fstr = kstrdup(fstr, GFP_KERNEL);if (!fstr)return -ENOMEM ;
@@ -10674,7 +10678,6 @@ perf_event_parse_addr_filter(struct perf [IF_ACT_START] = PERF_ADDR_FILTER_ACTION_START, [IF_ACT_STOP] = PERF_ADDR_FILTER_ACTION_STOP, }; - ret = -EINVAL ;if (!*start)continue ;
@@ -10683,7 +10686,7 @@ perf_event_parse_addr_filter(struct perfif (state ==IF_STATE_ACTION ) { filter =perf_addr_filter_new (event, filters);if (!filter) - goto fail; +return -EINVAL ; } token =match_token (start, if_tokens, args);
@@ -10692,7 +10695,7 @@ perf_event_parse_addr_filter(struct perfcase IF_ACT_START :case IF_ACT_STOP :if (state !=IF_STATE_ACTION ) - goto fail; +return -EINVAL ; filter->action = actions[token]; state =IF_STATE_SOURCE ;
@@ -10706,18 +10709,18 @@ perf_event_parse_addr_filter(struct perfcase IF_SRC_FILEADDR :case IF_SRC_FILE :if (state !=IF_STATE_SOURCE ) - goto fail; +return -EINVAL ; *args[0 ].to =0 ; ret =kstrtoul (args[0 ].from,0 , &filter->offset);if (ret) - goto fail; +return ret;if (token ==IF_SRC_KERNEL || token ==IF_SRC_FILE ) { *args[1 ].to =0 ; ret =kstrtoul (args[1 ].from,0 , &filter->size);if (ret) - goto fail; +return ret; }if (token ==IF_SRC_FILE || token ==IF_SRC_FILEADDR ) {
@@ -10725,17 +10728,15 @@ perf_event_parse_addr_filter(struct perfkfree (filename); filename =match_strdup (&args[fpos]); -if (!filename) { - ret = -ENOMEM ; - goto fail; - } +if (!filename) +return -ENOMEM ; } state =IF_STATE_END ;break ;default : - goto fail; +return -EINVAL ; }/*
@@ -10744,19 +10745,17 @@ perf_event_parse_addr_filter(struct perf* attribute. */ if (state ==IF_STATE_END ) { - ret = -EINVAL ; -/* * ACTION "filter" must have a non-zero length region * specified. */ if (filter->action ==PERF_ADDR_FILTER_ACTION_FILTER && !filter->size) - goto fail; +return -EINVAL ;if (!kernel) {if (!filename) - goto fail; +return -EINVAL ;/* * For now, we only support file-based filters
@@ -10766,21 +10765,19 @@ perf_event_parse_addr_filter(struct perf* mapped at different virtual addresses in * different processes. */ - ret = -EOPNOTSUPP ;if (!event->ctx->task) - goto fail; +return -EOPNOTSUPP ;/* look up the path and grab its inode */ ret =kern_path (filename,LOOKUP_FOLLOW , &filter->path);if (ret) - goto fail; +return ret; - ret = -EINVAL ;if (!filter->path.dentry || !S_ISREG (d_inode (filter->path.dentry) ->i_mode)) - goto fail; +return -EINVAL ; event->addr_filters.nr_file_filters++; }
@@ -10795,19 +10792,10 @@ perf_event_parse_addr_filter(struct perf }if (state !=IF_STATE_ACTION ) - goto fail; - -kfree (filename); -kfree (orig); +return -EINVAL ; +no_free_ptr (fguard);// allow filters to escape to the caller return 0 ; - -fail: -kfree (filename); -free_filters_list (filters); -kfree (orig); - -return ret; } static int
Re: [PATCH v3 45/57] perf: Simplify perf_event_parse_addr_filter()
Quoted 1 line · Peter Zijlstra
XXX this code needs a cleanup
:)
Quoted 23 lines
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/events/core.c | 56 ++++++++++++++++++++------------------------------- 1 file changed, 22 insertions(+), 34 deletions(-)kernel/events/core.c +2
-1@@ -10495,6 +10495,8 @@ static void free_filters_list(struct lis } } +DEFINE_FREE(filter_list, struct list_head *, if (_T) free_filters_list(_T)) +/* * Free existing address filters and optionally install new ones */ @@ -10658,13 +10660,15 @@ perf_event_parse_addr_filter(struct perf struct list_head *filters) {struct perf_addr_filter *filter =NULL ; -char *start, *orig, *filename =NULL ;substring_t args[MAX_OPT_ARGS ];
kernel/events/core.c perf_event_parse_addr_filter(struct perf · line 10671
Quoted 10 lines
kernel/events/core.c +5
-2int state =IF_STATE_ACTION , token;unsigned int kernel =0 ; -int ret = -EINVAL ; +char *start; +int ret; - orig = fstr =kstrdup (fstr,GFP_KERNEL ); + struct list_head *fguard __free(filter_list) = filters; + char *filename __free(kfree) = NULL; + char *orig __free(kfree) = fstr = kstrdup(fstr, GFP_KERNEL);
Ah! I see a kfree users. Sorry for the noise. :)
Signature · 1 line
Kees Cook
Peter Zijlstra
maintainerPATCH v3 46/57 perf: Simplify pmu_dev_alloc()
In reply to Peter Zijlstra--- kernel/events/core.c | 65 ++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 34 deletions(-)
kernel/events/core.c +31 -34
@@ -11285,49 +11285,46 @@ static void pmu_dev_release(struct devicstatic int pmu_dev_alloc (struct pmu *pmu) { -int ret = -ENOMEM ; +int ret; - pmu->dev =kzalloc (sizeof (struct device ),GFP_KERNEL ); -if (!pmu->dev) - goto out; + struct device *dev __free(put_device) = +kzalloc (sizeof (struct device ),GFP_KERNEL ); +if (!dev) +return -ENOMEM ; - pmu->dev->groups = pmu->attr_groups; -device_initialize (pmu->dev); + dev->groups = pmu->attr_groups; +device_initialize (dev); -dev_set_drvdata (pmu->dev, pmu); - pmu->dev->bus = &pmu_bus; - pmu->dev->release = pmu_dev_release; +dev_set_drvdata (dev, pmu); + dev->bus = &pmu_bus; + dev->release = pmu_dev_release; - ret =dev_set_name (pmu->dev,"%s" , pmu->name); + ret =dev_set_name (dev,"%s" , pmu->name);if (ret) - goto free_dev; +return ret; - ret =device_add (pmu->dev); + ret =device_add (dev);if (ret) - goto free_dev; +return ret; -/* For PMUs with address filters, throw in an extra attribute: */ -if (pmu->nr_addr_filters) - ret =device_create_file (pmu->dev, &dev_attr_nr_addr_filters); - -if (ret) - goto del_dev; + struct device *del __free(device_del) = dev; -if (pmu->attr_update) - ret =sysfs_update_groups (&pmu->dev->kobj, pmu->attr_update); - -if (ret) - goto del_dev; - -out: -return ret; - -del_dev: -device_del (pmu->dev); - -free_dev: -put_device (pmu->dev); - goto out; +/* For PMUs with address filters, throw in an extra attribute: */ +if (pmu->nr_addr_filters) { + ret =device_create_file (dev, &dev_attr_nr_addr_filters); +if (ret) +return ret; + } + +if (pmu->attr_update) { + ret =sysfs_update_groups (&dev->kobj, pmu->attr_update); +if (ret) +return ret; + } + +no_free_ptr (del); + pmu->dev =no_free_ptr (dev); +return 0 ; }static struct lock_class_key cpuctx_mutex;
Peter Zijlstra
maintainerRe: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
Quoted 25 lines · Peter Zijlstra
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/events/core.c | 65 ++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 34 deletions(-)kernel/events/core.c +10
-0@@ -11285,49 +11285,46 @@ static void pmu_dev_release(struct devicstatic int pmu_dev_alloc (struct pmu *pmu) { +int ret; + struct device *dev __free(put_device) = +kzalloc (sizeof (struct device ),GFP_KERNEL ); +if (!dev) +return -ENOMEM ; + dev->groups = pmu->attr_groups; +device_initialize (dev); +dev_set_drvdata (dev, pmu); + dev->bus = &pmu_bus; + dev->release = pmu_dev_release;
kernel/events/core.c static void pmu_dev_release(struct devic · line 11310
Quoted 10 lines
kernel/events/core.c +5
-0+ ret =dev_set_name (dev,"%s" , pmu->name);if (ret) +return ret; + ret =device_add (dev);if (ret) +return ret; + struct device *del __free(device_del) = dev;
Greg, I'm not much familiar with the whole device model, but it seems unfortunate to me that one has to call device_del() explicitly if we already have a put_device() queued.
Is there a saner way to write this?
Quoted 18 lines
kernel/events/core.c +16
-0+/* For PMUs with address filters, throw in an extra attribute: */ +if (pmu->nr_addr_filters) { + ret =device_create_file (dev, &dev_attr_nr_addr_filters); +if (ret) +return ret; + } + +if (pmu->attr_update) { + ret =sysfs_update_groups (&dev->kobj, pmu->attr_update); +if (ret) +return ret; + } + +no_free_ptr (del); + pmu->dev =no_free_ptr (dev); +return 0 ; }
Greg KH
maintainerRe: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
Quoted 35 lines · Peter Zijlstra
Quoted 34 lines · Peter Zijlstra
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/events/core.c | 65 ++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 34 deletions(-)kernel/events/core.c +14
-0@@ -11285,49 +11285,46 @@ static void pmu_dev_release(struct devicstatic int pmu_dev_alloc (struct pmu *pmu) { +int ret; + struct device *dev __free(put_device) = +kzalloc (sizeof (struct device ),GFP_KERNEL ); +if (!dev) +return -ENOMEM ; + dev->groups = pmu->attr_groups; +device_initialize (dev); +dev_set_drvdata (dev, pmu); + dev->bus = &pmu_bus; + dev->release = pmu_dev_release; + ret =dev_set_name (dev,"%s" , pmu->name);if (ret) +return ret; + ret =device_add (dev);if (ret) +return ret;
Quoted 7 lines
kernel/events/core.c static void pmu_dev_release(struct devic · line 11310
Quoted 1 line
kernel/events/core.c +1
-0+ struct device *del __free(device_del) = dev;Greg, I'm not much familiar with the whole device model, but it seems unfortunate to me that one has to call
device_del()explicitly if we already have aput_device()queued.Is there a saner way to write this?
Yes, there should be, let me look into it later tonight, need to get some stable kernels out for review first...
thanks,
greg k-h
Greg KH
maintainerRe: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
In reply to Peter ZijlstraQuoted 35 lines · Peter Zijlstra
Quoted 34 lines · Peter Zijlstra
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/events/core.c | 65 ++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 34 deletions(-)kernel/events/core.c +14
-0@@ -11285,49 +11285,46 @@ static void pmu_dev_release(struct devicstatic int pmu_dev_alloc (struct pmu *pmu) { +int ret; + struct device *dev __free(put_device) = +kzalloc (sizeof (struct device ),GFP_KERNEL ); +if (!dev) +return -ENOMEM ; + dev->groups = pmu->attr_groups; +device_initialize (dev); +dev_set_drvdata (dev, pmu); + dev->bus = &pmu_bus; + dev->release = pmu_dev_release; + ret =dev_set_name (dev,"%s" , pmu->name);if (ret) +return ret; + ret =device_add (dev);if (ret) +return ret;
Quoted 7 lines
kernel/events/core.c static void pmu_dev_release(struct devic · line 11310
Quoted 1 line
kernel/events/core.c +1
-0+ struct device *del __free(device_del) = dev;Greg, I'm not much familiar with the whole device model, but it seems unfortunate to me that one has to call
device_del()explicitly if we already have aput_device()queued.Is there a saner way to write this?
Ok, the "problem" here is that you have decided to do the "complex" way to initialize a struct device. And as such, you have to do more housekeeping than if you were to just use the simple interface.
The rule is, after you call device_initialize() you HAVE to call put_device() on the pointer if something goes wrong and you want to clean up properly. Unless you have called device_add(), and at that point in time, then you HAVE to call device_del() if the device_add() call succeeded. If the device_add() call failed, then you HAVE to call put_device().
Yeah, it's a pain, but you are trying to hand-roll code that is not a "normal" path for a struct device, sorry.
I don't know if you really can encode all of that crazy logic in the cleanup api, UNLESS you can "switch" the cleanup function at a point in time (i.e. after device_add() is successful). Is that possible?
Anyway, let me see about just cleaning up this code in general, I don't think you need the complex interface here for a tiny struct device at all, which would make this specific instance moot :)
Also, nit, you are racing with userspace by attempting to add new device files _AFTER_ the device is registered with the driver core, this whole thing can be made more simpler I hope, give me a bit...
thanks
greg k-h
Re: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
Quoted 6 lines · Greg KH
Yeah, it's a pain, but you are trying to hand-roll code that is not a "normal" path for a struct device, sorry.
I don't know if you really can encode all of that crazy logic in the cleanup api, UNLESS you can "switch" the cleanup function at a point in time (i.e. after
device_add()is successful). Is that possible?
What _could_ make sense is that device_add() completely takes ownership of the given pointer, and takes care of calling put_device() on failure.
Then you can have
struct device *dev_struct __free(put_device) = kzalloc(sizeof(struct device), GFP_KERNEL); struct device *dev __free(device_del) = device_add(no_free_ptr(dev_struct)); /* dev_struct is NULL now */ pmu->dev = no_free_ptr(dev);
Paolo
Greg KH
maintainerRe: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
Quoted 10 lines · Paolo Bonzini
Quoted 6 lines · Greg KH
Yeah, it's a pain, but you are trying to hand-roll code that is not a "normal" path for a struct device, sorry.
I don't know if you really can encode all of that crazy logic in the cleanup api, UNLESS you can "switch" the cleanup function at a point in time (i.e. after
device_add()is successful). Is that possible?What _could_ make sense is that
device_add()completely takes ownership of the given pointer, and takes care of callingput_device()on failure.
I think we tried that decades ago :)
Problem is that the caller wants to clean stuff up associted with that struct device-embedded structure _before_ the final put_device() is called which would usually free the memory of the overall structure.
So device_add() can't call put_device() on the error path within it, you need the caller to unwind it's local stuff before that happens.
Quoted 11 lines
Then you can have
struct device *dev_struct __free(put_device) = kzalloc(sizeof(struct device), GFP_KERNEL); struct device *dev __free(device_del) = device_add(no_free_ptr(dev_struct)); /* dev_struct is NULL now */ pmu->dev = no_free_ptr(dev);
I don't see how that works properly, what am I missing?
thanks,
greg k-h
Greg KH
maintainerRe: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
In reply to Greg KHQuoted 62 lines · Greg KH
Quoted 35 lines · Peter Zijlstra
Quoted 34 lines · Peter Zijlstra
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/events/core.c | 65 ++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 34 deletions(-)kernel/events/core.c +14
-0@@ -11285,49 +11285,46 @@ static void pmu_dev_release(struct devicstatic int pmu_dev_alloc (struct pmu *pmu) { +int ret; + struct device *dev __free(put_device) = +kzalloc (sizeof (struct device ),GFP_KERNEL ); +if (!dev) +return -ENOMEM ; + dev->groups = pmu->attr_groups; +device_initialize (dev); +dev_set_drvdata (dev, pmu); + dev->bus = &pmu_bus; + dev->release = pmu_dev_release; + ret =dev_set_name (dev,"%s" , pmu->name);if (ret) +return ret; + ret =device_add (dev);if (ret) +return ret;Quoted 7 lines
kernel/events/core.c static void pmu_dev_release(struct devic · line 11310
Quoted 1 line
kernel/events/core.c +1
-0+ struct device *del __free(device_del) = dev;Greg, I'm not much familiar with the whole device model, but it seems unfortunate to me that one has to call
device_del()explicitly if we already have aput_device()queued.Is there a saner way to write this?
Ok, the "problem" here is that you have decided to do the "complex" way to initialize a struct device. And as such, you have to do more housekeeping than if you were to just use the simple interface.
The rule is, after you call
device_initialize()you HAVE to callput_device()on the pointer if something goes wrong and you want to clean up properly. Unless you have calleddevice_add(), and at that point in time, then you HAVE to calldevice_del()if thedevice_add()call succeeded. If thedevice_add()call failed, then you HAVE to callput_device().Yeah, it's a pain, but you are trying to hand-roll code that is not a "normal" path for a struct device, sorry.
I don't know if you really can encode all of that crazy logic in the cleanup api, UNLESS you can "switch" the cleanup function at a point in time (i.e. after
device_add()is successful). Is that possible?
Quoted 7 lines
Anyway, let me see about just cleaning up this code in general, I don't think you need the complex interface here for a tiny struct device at all, which would make this specific instance moot :)
Also, nit, you are racing with userspace by attempting to add new device files _AFTER_ the device is registered with the driver core, this whole thing can be made more simpler I hope, give me a bit...
Nope, I was wrong, I can fix the race condition, but the logic here for how to init and clean up on errors is right, and you want this because you are a bus and so, you need the two-step init/teardown process, sorry.
Here's the patch I came up with to get rid of the race, but doesn't really help you out here at all :(
------------------------
From foo@baz Mon Jun 12 03:07:54 PM CEST 2023
Date: Mon, 12 Jun 2023 15:07:54 +0200
To: Greg KH <[email protected]>
From: Greg Kroah-Hartman <[email protected]>
Subject: [PATCH] perf/core: fix narrow startup race when creating the perf nr_addr_filters sysfs file
kernel/events/core.c +28 -12
@@ -11351,9 +11351,32 @@ static DEVICE_ATTR_RW(perf_event_mux_interval_ms);static struct attribute *pmu_dev_attrs[] = { &dev_attr_type.attr, &dev_attr_perf_event_mux_interval_ms.attr, + &dev_attr_nr_addr_filters.attr, +NULL , +}; + +static umode_t pmu_dev_is_visible (struct kobject *kobj,struct attribute *a,int n) +{ +struct device *dev =kobj_to_dev (kobj); +struct pmu *pmu =dev_get_drvdata (dev); + +if (!pmu->nr_addr_filters) +return 0 ; + +return a->mode; + +return 0 ; +} + +static struct attribute_group pmu_dev_attr_group = { + .is_visible = pmu_dev_is_visible, + .attrs = pmu_dev_attrs, +}; + +const static struct attribute_group *pmu_dev_groups[] = { + &pmu_dev_attr_group,NULL , }; -ATTRIBUTE_GROUPS (pmu_dev);static int pmu_bus_running;static struct bus_type pmu_bus = {
@@ -11389,18 +11412,11 @@ static int pmu_dev_alloc(struct pmu *pmu)if (ret) goto free_dev; -/* For PMUs with address filters, throw in an extra attribute: */ -if (pmu->nr_addr_filters) - ret =device_create_file (pmu->dev, &dev_attr_nr_addr_filters); - -if (ret) - goto del_dev; - -if (pmu->attr_update) +if (pmu->attr_update) { ret =sysfs_update_groups (&pmu->dev->kobj, pmu->attr_update); - -if (ret) - goto del_dev; +if (ret) + goto del_dev; + } out:return ret;
[tip: perf/core] perf/core: Fix narrow startup race when creating the perf nr_addr_filters sysfs file
The following commit has been merged into the perf/core branch of tip:
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 652ffc2104ec1f69dd4a46313888c33527145ccf Gitweb: https://git.kernel.org/tip/652ffc2104ec1f69dd4a46313888c33527145ccf Author: Greg KH <[email protected]> AuthorDate: Mon, 12 Jun 2023 15:09:09 +02:00 Committer: Peter Zijlstra <[email protected]> CommitterDate: Wed, 15 Nov 2023 10:15:50 +01:00
perf/core: Fix narrow startup race when creating the perf nr_addr_filters sysfs file
--- kernel/events/core.c | 40 ++++++++++++++++++++++++++++------------ 1 file changed, 28 insertions(+), 12 deletions(-)
kernel/events/core.c +28 -12
@@ -11408,9 +11408,32 @@ static DEVICE_ATTR_RW(perf_event_mux_interval_ms);static struct attribute *pmu_dev_attrs[] = { &dev_attr_type.attr, &dev_attr_perf_event_mux_interval_ms.attr, + &dev_attr_nr_addr_filters.attr, +NULL , +}; + +static umode_t pmu_dev_is_visible (struct kobject *kobj,struct attribute *a,int n) +{ +struct device *dev =kobj_to_dev (kobj); +struct pmu *pmu =dev_get_drvdata (dev); + +if (!pmu->nr_addr_filters) +return 0 ; + +return a->mode; + +return 0 ; +} + +static struct attribute_group pmu_dev_attr_group = { + .is_visible = pmu_dev_is_visible, + .attrs = pmu_dev_attrs, +}; + +static const struct attribute_group *pmu_dev_groups[] = { + &pmu_dev_attr_group,NULL , }; -ATTRIBUTE_GROUPS (pmu_dev);static int pmu_bus_running;static struct bus_type pmu_bus = {
@@ -11447,18 +11470,11 @@ static int pmu_dev_alloc(struct pmu *pmu)if (ret) goto free_dev; -/* For PMUs with address filters, throw in an extra attribute: */ -if (pmu->nr_addr_filters) - ret =device_create_file (pmu->dev, &dev_attr_nr_addr_filters); - -if (ret) - goto del_dev; - -if (pmu->attr_update) +if (pmu->attr_update) { ret =sysfs_update_groups (&pmu->dev->kobj, pmu->attr_update); - -if (ret) - goto del_dev; +if (ret) + goto del_dev; + } out:return ret;
Re: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
In reply to Peter ZijlstraQuoted 35 lines · Peter Zijlstra
Quoted 34 lines · Peter Zijlstra
Signed-off-by Peter Zijlstra (Intel) <[email protected]>--- kernel/events/core.c | 65 ++++++++++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 34 deletions(-)kernel/events/core.c +14
-0@@ -11285,49 +11285,46 @@ static void pmu_dev_release(struct devicstatic int pmu_dev_alloc (struct pmu *pmu) { +int ret; + struct device *dev __free(put_device) = +kzalloc (sizeof (struct device ),GFP_KERNEL ); +if (!dev) +return -ENOMEM ; + dev->groups = pmu->attr_groups; +device_initialize (dev); +dev_set_drvdata (dev, pmu); + dev->bus = &pmu_bus; + dev->release = pmu_dev_release; + ret =dev_set_name (dev,"%s" , pmu->name);if (ret) +return ret; + ret =device_add (dev);if (ret) +return ret;
Quoted 7 lines
kernel/events/core.c static void pmu_dev_release(struct devic · line 11310
Quoted 1 line
kernel/events/core.c +1
-0+ struct device *del __free(device_del) = dev;Greg, I'm not much familiar with the whole device model, but it seems unfortunate to me that one has to call
device_del()explicitly if we already have aput_device()queued.Is there a saner way to write this?
Ok, to answer my other question, yes, you are changing the free call here in the "middle" of the function, sorry, I missed "del" vs. "dev" and was wondering how this would work...
This should work, it's tricky, especially:
Quoted 2 lines
kernel/events/core.c static void pmu_dev_release(struct devic · line 11326
Quoted 2 lines
kernel/events/core.c +2
-0+no_free_ptr (del); + pmu->dev =no_free_ptr (dev);
this.
I had to stare at it for a while to realize that yes, you are calling the two different "cleanup" functions prior to thes lines, on the same pointer, and that the order is correct.
Ick, this is going to be a rough audit for bus code that gets converted to this, BUT bonus is that once it's done, any changes to the middle of the function should "just work", and it's a good task for an intern to do :)
Mind if I try this series to convert a more "normal" driver to see how it works with that? That's going to be the true test, see if the changes make sense to someone who doesn't really know the internals of the driver core like this...
thanks,
greg k-h
Peter Zijlstra
maintainerRe: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
Quoted 4 lines · Greg KH
Ick, this is going to be a rough audit for bus code that gets converted to this, BUT bonus is that once it's done, any changes to the middle of the function should "just work", and it's a good task for an intern to do :)
Hehe. The 'bonus' in all this is that if you get it wrong, it's the very likely success case that goes sideways and it should instantly explode.
While all the unlikely error cases should all just work -- famous last words etc..
Quoted 1 line
Reviewed-by Greg Kroah-Hartman <[email protected]>
Thanks!
Quoted 4 lines
Mind if I try this series to convert a more "normal" driver to see how it works with that? That's going to be the true test, see if the changes make sense to someone who doesn't really know the internals of the driver core like this...
Not at all, feel tree to have a go at that. I picked code I was familiar with, but it would ofc. be good to have others give it a spin too.
Greg KH
maintainerRe: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
Quoted 7 lines · Peter Zijlstra
Quoted 4 lines
Mind if I try this series to convert a more "normal" driver to see how it works with that? That's going to be the true test, see if the changes make sense to someone who doesn't really know the internals of the driver core like this...
Not at all, feel tree to have a go at that. I picked code I was familiar with, but it would ofc. be good to have others give it a spin too.
Ok, here's an attempt to clean up misc.c a bit. It does two things, adds a guard for the global misc_mtx mutex when used (sometimes, it's not always paired with a release.)
Then in the last part of the file, I abuse the DEFINE_FREE() to handle a special case of removing a proc file if things go bad (and add a DEFINE_FREE() for class_destroy(), which should go into include/device/class.h.
I've only test-built it, but is this the proper use of DEFINE_FREE()? There wasn't much documentation :)
To be fair the end-result of misc_init() is much nicer and cleaner and "obviously correct", which is good, even with the crazy proc file mess in it. So I like the idea overall, need to figure out when to use DEFINE_CLASS() vs. DEFINE_FREE(), that isn't obvious to me.
Also, you can't put a DEFINE_FREE() within a function declaration, which I guess makes sense, but the build warning is very odd when you attempt it, mentioning an "invalid storage class". Is that supposed to be able to work?
thanks,
greg k-h
--- drivers/char/misc.c | 69 +++++++++++++++++---------------------------- 1 file changed, 26 insertions(+), 43 deletions(-)
drivers/char/misc.c +26 -43
@@ -123,10 +123,9 @@ static int misc_open(struct inode *inode, struct file *file) {int minor =iminor (inode);struct miscdevice *c =NULL , *iter; -int err = -ENODEV ;const struct file_operations *new_fops =NULL ; -mutex_lock (&misc_mtx); +guard (mutex)(&misc_mtx);list_for_each_entry (iter, &misc_list, list) {if (iter->minor != minor)
@@ -149,7 +148,7 @@ static int misc_open(struct inode *inode, struct file *file)break ; }if (!new_fops) - goto fail; +return -ENODEV ; }/*
@@ -159,13 +158,11 @@ static int misc_open(struct inode *inode, struct file *file)*/ file->private_data = c; - err =0 ;replace_fops (file, new_fops);if (file->f_op->open) - err = file->f_op->open (inode, file); -fail: -mutex_unlock (&misc_mtx); -return err; +return file->f_op->open (inode, file); + +return 0 ; }static struct class *misc_class;
@@ -197,29 +194,24 @@ static const struct file_operations misc_fops = {int misc_register (struct miscdevice *misc) {dev_t dev; -int err =0 ;bool is_dynamic = (misc->minor ==MISC_DYNAMIC_MINOR );INIT_LIST_HEAD (&misc->list); -mutex_lock (&misc_mtx); +guard (mutex)(&misc_mtx);if (is_dynamic) {int i =misc_minor_alloc (); -if (i <0 ) { - err = -EBUSY ; - goto out; - } +if (i <0 ) +return -EBUSY ; misc->minor = i; }else {struct miscdevice *c;list_for_each_entry (c, &misc_list, list) { -if (c->minor == misc->minor) { - err = -EBUSY ; - goto out; - } +if (c->minor == misc->minor) +return -EBUSY ; } }
@@ -233,8 +225,7 @@ int misc_register(struct miscdevice *misc)misc_minor_free (misc->minor); misc->minor =MISC_DYNAMIC_MINOR ; } - err =PTR_ERR (misc->this_device); - goto out; +return PTR_ERR (misc->this_device); }/*
@@ -242,9 +233,7 @@ int misc_register(struct miscdevice *misc)* earlier defaults */ list_add (&misc->list, &misc_list); - out: -mutex_unlock (&misc_mtx); -return err; +return 0 ; }EXPORT_SYMBOL (misc_register);
@@ -261,11 +250,10 @@ void misc_deregister(struct miscdevice *misc)if (WARN_ON (list_empty (&misc->list)))return ; -mutex_lock (&misc_mtx); +guard (mutex)(&misc_mtx);list_del (&misc->list);device_destroy (misc_class,MKDEV (MISC_MAJOR , misc->minor));misc_minor_free (misc->minor); -mutex_unlock (&misc_mtx); }EXPORT_SYMBOL (misc_deregister);
@@ -280,29 +268,24 @@ static char *misc_devnode(const struct device *dev, umode_t *mode)return NULL ; } +DEFINE_FREE(class_destroy, struct class *, if (_T) class_destroy(_T)); +DEFINE_FREE(remove_proc, struct proc_dir_entry *, if (_T) remove_proc_entry("misc", NULL));static int __init misc_init (void ) { -int err; -struct proc_dir_entry *ret; + struct proc_dir_entry *ret __free(remove_proc) = proc_create_seq("misc", 0, NULL, &misc_seq_ops); + struct class *c __free(class_destroy) = class_create("misc"); - ret =proc_create_seq ("misc" ,0 ,NULL , &misc_seq_ops); - misc_class =class_create ("misc" ); - err =PTR_ERR (misc_class); -if (IS_ERR (misc_class)) - goto fail_remove; +if (IS_ERR (c)) +return PTR_ERR (c); - err = -EIO ;if (register_chrdev (MISC_MAJOR ,"misc" , &misc_fops)) - goto fail_printk; - misc_class->devnode = misc_devnode; -return 0 ; +return -EIO ; -fail_printk: -pr_err ("unable to get major %d for misc devices\n" ,MISC_MAJOR ); -class_destroy (misc_class); -fail_remove: -if (ret) -remove_proc_entry ("misc" ,NULL ); -return err; + c->devnode = misc_devnode; + + misc_class =no_free_ptr (c); +no_free_ptr (ret); + +return 0 ; }subsys_initcall (misc_init);
Signature · 1 line
2.41.0
Peter Zijlstra
maintainerRe: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
Quoted 7 lines · Greg KH
Then in the last part of the file, I abuse the
DEFINE_FREE()to handle a special case of removing a proc file if things go bad (and add aDEFINE_FREE()forclass_destroy(), which should go intoinclude/device/class.h.I've only test-built it, but is this the proper use of
DEFINE_FREE()? There wasn't much documentation :)
Yes, this looks right.
Quoted 4 lines
To be fair the end-result of
misc_init()is much nicer and cleaner and "obviously correct", which is good, even with the crazy proc file mess in it. So I like the idea overall, need to figure out when to useDEFINE_CLASS()vs.DEFINE_FREE(), that isn't obvious to me.
CLASS is meant for things that have an obvious contructor as well as a destructor, that always go together. Like for example the lock things, they always pair a lock and unlock. But also things like:
fdget()+fdput(), these can also always be paired, and if you want the file to escape you simply take yet another reference to prevent the fdput() from being the final.
Quoted 4 lines
Also, you can't put a
DEFINE_FREE()within a function declaration, which I guess makes sense, but the build warning is very odd when you attempt it, mentioning an "invalid storage class". Is that supposed to be able to work?
No, DEFINE_FREE() and DEFINE_CLASS() end up defining a bunch of inline functions, which can't be done inside another function.
If only C would have lambda functions ... alas.
drivers/char/misc.c static char *misc_devnode(const struct device *dev, umode_t *mode) · line 271
Quoted 5 lines
drivers/char/misc.c +1
-0@@ -280,29 +268,24 @@ static char *misc_devnode(const struct device *dev, umode_t *mode)return NULL ; } +DEFINE_FREE(class_destroy, struct class *, if (_T) class_destroy(_T));
Documentation for class_create() says it will return ERR_PTR(), so then this should be something like:
DEFINE_FRERE(class_destroy, struct class *, if (!IS_ERR_OR_NULL(_T)) class_destroy(_T))
Quoted 9 lines
drivers/char/misc.c +5
-0+DEFINE_FREE(remove_proc, struct proc_dir_entry *, if (_T) remove_proc_entry("misc", NULL));static int __init misc_init (void ) { + struct proc_dir_entry *ret __free(remove_proc) = proc_create_seq("misc", 0, NULL, &misc_seq_ops); + struct class *c __free(class_destroy) = class_create("misc"); +if (IS_ERR (c)) +return PTR_ERR (c);
drivers/char/misc.c static char *misc_devnode(const struct device *dev, umode_t *mode) · line 290
Quoted 10 lines
drivers/char/misc.c +7
-0if (register_chrdev (MISC_MAJOR ,"misc" , &misc_fops)) +return -EIO ; + c->devnode = misc_devnode; + + misc_class =no_free_ptr (c); +no_free_ptr (ret); + +return 0 ; }
And yes, this does look nicer.
Greg KH
maintainerRe: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
Quoted 6 lines · Peter Zijlstra
Quoted 4 lines · Greg KH
To be fair the end-result of
misc_init()is much nicer and cleaner and "obviously correct", which is good, even with the crazy proc file mess in it. So I like the idea overall, need to figure out when to useDEFINE_CLASS()vs.DEFINE_FREE(), that isn't obvious to me.
Quoted 6 lines
CLASS is meant for things that have an obvious contructor as well as a destructor, that always go together. Like for example the lock things, they always pair a lock and unlock. But also things like:
fdget()+fdput(), these can also always be paired, and if you want the file to escape you simply take yet another reference to prevent thefdput()from being the final.
Ok, so then the class_destroy stuff down below here should be DEFINE_CLASS()?
Quoted 10 lines
drivers/char/misc.c static char *misc_devnode(const struct device *dev, umode_t *mode) · line 271
Quoted 5 lines
drivers/char/misc.c +1
-0@@ -280,29 +268,24 @@ static char *misc_devnode(const struct device *dev, umode_t *mode)return NULL ; } +DEFINE_FREE(class_destroy, struct class *, if (_T) class_destroy(_T));Documentation for
class_create()says it will returnERR_PTR(), so then this should be something like:
DEFINE_FRERE(class_destroy, struct class *, if (!IS_ERR_OR_NULL(_T)) class_destroy(_T))
Nit, as class_destroy() handles this type of check within it, it can be even simpler:
DEFINE_FREE(class_destroy, struct class *, class_destroy(_T)); or would that be: DEFINE_CLASS(class_destroy, struct class *, class_destroy(_T));
?
Quoted 12 lines
Quoted 12 lines
drivers/char/misc.c +6
-0+DEFINE_FREE(remove_proc, struct proc_dir_entry *, if (_T) remove_proc_entry("misc", NULL));static int __init misc_init (void ) { + struct proc_dir_entry *ret __free(remove_proc) = proc_create_seq("misc", 0, NULL, &misc_seq_ops); + struct class *c __free(class_destroy) = class_create("misc"); +if (IS_ERR (c)) +return PTR_ERR (c);if (register_chrdev (MISC_MAJOR ,"misc" , &misc_fops)) +return -EIO ;
Quoted 9 lines
drivers/char/misc.c static char *misc_devnode(const struct device *dev, umode_t *mode) · line 290
Quoted 7 lines
drivers/char/misc.c +6
-0+ c->devnode = misc_devnode; + + misc_class =no_free_ptr (c); +no_free_ptr (ret); + +return 0 ; }And yes, this does look nicer.
I have a ton of future patches coming that does a bunch of class_create/destroy changes that would be made a LOT simpler with this patchset, and I really don't want to have to hit the same codepaths twice if at all possible.
So what's the odds this can be reasonable enough to get into 6.5-rc1 so we can rely on it there?
thanks,
greg k-h
Peter Zijlstra
maintainerRe: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()
Quoted 5 lines · Greg KH
Quoted 1 line
DEFINE_FRERE(class_destroy, struct class *, if (!IS_ERR_OR_NULL(_T)) class_destroy(_T))Nit, as
class_destroy()handles this type of check within it, it can be even simpler:DEFINE_FREE(class_destroy, struct class *, class_destroy(_T));
Note that that means there will be an unconditional call to class_destroy() in the success path. As long as that is never a hot-path this should be fine I suppose, but it is something Linus pointed out earlier.
Quoted 2 lines
or would that be:
DEFINE_CLASS(class_destroy, struct class *, class_destroy(_T));
Has a slightly different syntax per the comment I did do write :-)
DEFINE_CLASS(class, struct class *,
if (!IS_ERR_OR_NULL(_T)) class_destroy(_T), class_create(cname), const char *name)
static int __init misc_init(void)
{
struct proc_dir_entry *ret __free(remove_proc) =
proc_create_seq("misc", 0, NULL, &misc_seq_ops);
CLASS(class, c)("misc");
if (IS_ERR(c))
return PTR_ERR(c);
if (register_chrdev(MISC_MAJOR, "misc", &misc_fops))
return -EIO;
c->devnode = misc_devnode;
misc_class = no_free_ptr(c);
no_free_ptr(ret);
return 0;
}The no_free_ptr() should work with CLASS(), but I'm not sure that's recommended, lots of un-explored terretory here :-)
Similarly I suppose you could do something like:
DEFINE_CLASS(proc_dir, struct proc_dir_entry *,
proc_remove(_T), proc_create(pname, mode, parent, proc_ops), const char *pname, umode_t mode, struct proc_dir_entry *parent, const struct proc_ops *proc_ops)
EXTEND_CLASS(proc_dir, _seq, proc_create_seq(pname, mode, parent, ops, state_size, data),
const char *pname, umode_t mode, struct proc_dir_entry *parent, const struct seq_operations *ops, unsigned int state_size, void *data)
EXTEND_CLASS(proc_dir, _seq_private, .....)
(urgh, C really needs better forwarding support)
Then you could write it something like:
static int __init misc_init(void)
{
CLASS(proc_dir_seq, ret)("misc", 0, NULL, &misc_seq_ops);
CLASS(class, c)("misc");
if (IS_ERR(c))
return PTR_ERR(c);
if (register_chrdev(MISC_MAJOR, "misc", &misc_fops))
return -EIO;
c->devnode = misc_devnode;
misc_class = no_free_ptr(c);
no_free_ptr(ret);
return 0;
}Is what what we want?
(also, perhaps I should prefix the macro arguments with an '_', as is you can't use 'name' as a constructor argument because the thing would expand weird)
Quoted 7 lines
I have a ton of future patches coming that does a bunch of class_create/destroy changes that would be made a LOT simpler with this patchset, and I really don't want to have to hit the same codepaths twice if at all possible.
So what's the odds this can be reasonable enough to get into 6.5-rc1 so we can rely on it there?
That's one for Linus I suppose.. the only remaining issue I still have is the no_free_*() naming. One suggestion that made sense had it called take_*().
All we really need are the first 4 patches to land; thereafter we can gradually start converting things.
Peter Zijlstra
maintainerPATCH v3 47/57 perf: Simplify perf_pmu_register()
In reply to Peter Zijlstra--- kernel/events/core.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-)
kernel/events/core.c +16 -24
@@ -11344,22 +11344,23 @@ void __perf_pmu_unregister(struct pmu *pfree_pmu_context (pmu); } -int perf_pmu_register (struct pmu *pmu,const char *name,int type) +DEFINE_FREE(pmu_unregister, struct pmu *, if (_T) __perf_pmu_unregister(_T)) + +int perf_pmu_register (struct pmu *_pmu,const char *name,int type) {int cpu, ret, max =PERF_TYPE_MAX ; - pmu->type =-1 ; + _pmu->type =-1 ; + +guard (mutex)(&pmus_lock); + struct pmu *pmu __free(pmu_unregister) = _pmu; -mutex_lock (&pmus_lock); - ret = -ENOMEM ; pmu->pmu_disable_count =alloc_percpu (int);if (!pmu->pmu_disable_count) - goto unlock; +return -ENOMEM ; -if (WARN_ONCE (!name,"Can not register anonymous pmu.\n" )) { - ret = -EINVAL ; - goto free; - } +if (WARN_ONCE (!name,"Can not register anonymous pmu.\n" )) +return -EINVAL ; pmu->name = name;
@@ -11368,7 +11369,7 @@ int perf_pmu_register(struct pmu *pmu, c ret =idr_alloc (&pmu_idr, pmu, max,0 ,GFP_KERNEL );if (ret <0 ) - goto free; +return ret;WARN_ON (type >=0 && ret != type);
@@ -11377,13 +11378,12 @@ int perf_pmu_register(struct pmu *pmu, cif (pmu_bus_running && !pmu->dev) { ret =pmu_dev_alloc (pmu);if (ret) - goto free; +return ret; } - ret = -ENOMEM ; pmu->cpu_pmu_context = alloc_percpu(struct perf_cpu_pmu_context);if (!pmu->cpu_pmu_context) - goto free; +return -ENOMEM ;for_each_possible_cpu (cpu) {struct perf_cpu_pmu_context *cpc;
@@ -11423,21 +11423,14 @@ int perf_pmu_register(struct pmu *pmu, clist_add_rcu (&pmu->entry, &pmus);atomic_set (&pmu->exclusive_cnt,0 ); - ret =0 ; -unlock: -mutex_unlock (&pmus_lock); - -return ret; - -free: -__perf_pmu_unregister (pmu); - goto unlock; +no_free_ptr (pmu);// let it rip +return 0 ; }EXPORT_SYMBOL_GPL (perf_pmu_register);void perf_pmu_unregister (struct pmu *pmu) { -mutex_lock (&pmus_lock); +guard (mutex)(&pmus_lock);list_del_rcu (&pmu->entry);/*
@@ -11448,7 +11441,6 @@ void perf_pmu_unregister(struct pmu *pmusynchronize_rcu ();__perf_pmu_unregister (pmu); -mutex_unlock (&pmus_lock); }EXPORT_SYMBOL_GPL (perf_pmu_unregister);
Peter Zijlstra
maintainerPATCH v3 48/57 perf: Simplify perf_init_event()
In reply to Peter Zijlstra--- kernel/events/core.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-)
kernel/events/core.c +12 -19
@@ -11504,10 +11504,10 @@ static int perf_try_init_event(struct pmstatic struct pmu *perf_init_event (struct perf_event *event) {bool extended_type = false; -int idx, type, ret;struct pmu *pmu; +int type, ret; - idx =srcu_read_lock (&pmus_srcu); +guard (srcu)(&pmus_srcu);/* * Save original type before calling pmu->event_init() since certain
@@ -11520,7 +11520,7 @@ static struct pmu *perf_init_event(struc pmu = event->parent->pmu; ret =perf_try_init_event (pmu, event);if (!ret) - goto unlock; +return pmu; }/*
@@ -11539,13 +11539,12 @@ static struct pmu *perf_init_event(struc } again: -rcu_read_lock (); - pmu =idr_find (&pmu_idr, type); -rcu_read_unlock (); +scoped_guard (rcu) + pmu =idr_find (&pmu_idr, type);if (pmu) {if (event->attr.type != type && type !=PERF_TYPE_RAW && !(pmu->capabilities &PERF_PMU_CAP_EXTENDED_HW_TYPE )) - goto fail; +return ERR_PTR (-ENOENT ); ret =perf_try_init_event (pmu, event);if (ret == -ENOENT && event->attr.type != type && !extended_type) {
@@ -11554,27 +11553,21 @@ static struct pmu *perf_init_event(struc }if (ret) - pmu =ERR_PTR (ret); +return ERR_PTR (ret); - goto unlock; +return pmu; }list_for_each_entry_rcu (pmu, &pmus, entry,lockdep_is_held (&pmus_srcu)) { ret =perf_try_init_event (pmu, event);if (!ret) - goto unlock; +return pmu; -if (ret != -ENOENT ) { - pmu =ERR_PTR (ret); - goto unlock; - } +if (ret != -ENOENT ) +return ERR_PTR (ret); } -fail: - pmu =ERR_PTR (-ENOENT ); -unlock: -srcu_read_unlock (&pmus_srcu, idx); -return pmu; +return ERR_PTR (-ENOENT ); }static void attach_sb_event (struct perf_event *event)
Peter Zijlstra
maintainerPATCH v3 49/57 perf: Simplify perf_event_alloc()
In reply to Peter Zijlstra--- kernel/events/core.c | 47 ++++++++++++++++++----------------------------- 1 file changed, 18 insertions(+), 29 deletions(-)
kernel/events/core.c +18 -29
@@ -5148,6 +5148,8 @@ static void __free_event(struct perf_evecall_rcu (&event->rcu_head, free_event_rcu); } +DEFINE_FREE(__free_event, struct perf_event *, if (_T) __free_event(_T)) + /* vs perf_event_alloc() success */static void _free_event (struct perf_event *event) {
@@ -11694,7 +11696,6 @@ perf_event_alloc(struct perf_event_attr void *context, int cgroup_fd) {struct pmu *pmu; -struct perf_event *event;struct hw_perf_event *hwc;long err = -EINVAL ;int node;
@@ -11709,8 +11710,8 @@ perf_event_alloc(struct perf_event_attr } node = (cpu >=0 ) ?cpu_to_node (cpu) :-1 ; - event =kmem_cache_alloc_node (perf_event_cache,GFP_KERNEL | __GFP_ZERO, - node); + struct perf_event *event __free(__free_event) = +kmem_cache_alloc_node (perf_event_cache,GFP_KERNEL | __GFP_ZERO, node);if (!event)return ERR_PTR (-ENOMEM );
@@ -11815,51 +11816,43 @@ perf_event_alloc(struct perf_event_attr* See perf_output_read(). */ if (attr->inherit && (attr->sample_type &PERF_SAMPLE_READ )) - goto err; +return ERR_PTR (-EINVAL );if (!has_branch_stack (event)) event->attr.branch_sample_type =0 ; pmu =perf_init_event (event); -if (IS_ERR (pmu)) { - err =PTR_ERR (pmu); - goto err; - } +if (IS_ERR (pmu)) +return (void *)pmu;/* * Disallow uncore-task events. Similarly, disallow uncore-cgroup * events (they don't make sense as the cgroup will be different * on other CPUs in the uncore mask). */ -if (pmu->task_ctx_nr == perf_invalid_context && (task || cgroup_fd !=-1 )) { - err = -EINVAL ; - goto err; - } +if (pmu->task_ctx_nr == perf_invalid_context && (task || cgroup_fd !=-1 )) +return ERR_PTR (-EINVAL );if (event->attr.aux_output && - !(pmu->capabilities &PERF_PMU_CAP_AUX_OUTPUT )) { - err = -EOPNOTSUPP ; - goto err; - } + !(pmu->capabilities &PERF_PMU_CAP_AUX_OUTPUT )) +return ERR_PTR (-EOPNOTSUPP );if (cgroup_fd !=-1 ) { err =perf_cgroup_connect (cgroup_fd, event, attr, group_leader);if (err) - goto err; +return ERR_PTR (err); } err =exclusive_event_init (event);if (err) - goto err; +return ERR_PTR (err);if (has_addr_filter (event)) { event->addr_filter_ranges =kcalloc (pmu->nr_addr_filters,sizeof (struct perf_addr_filter_range ),GFP_KERNEL ); -if (!event->addr_filter_ranges) { - err = -ENOMEM ; - goto err; - } +if (!event->addr_filter_ranges) +return ERR_PTR (-ENOMEM );/* * Clone the parent's vma offsets: they are valid until exec()
@@ -11883,22 +11876,18 @@ perf_event_alloc(struct perf_event_attrif (event->attr.sample_type &PERF_SAMPLE_CALLCHAIN ) { err =get_callchain_buffers (attr->sample_max_stack);if (err) - goto err; +return ERR_PTR (err); } } err =security_perf_event_alloc (event);if (err) - goto err; +return ERR_PTR (err);/* symmetric to unaccount_event() in _free_event() */ account_event (event); -return event; - -err: -__free_event (event); -return ERR_PTR (err); +return_ptr (event); } static int perf_copy_attr(struct perf_event_attr __user *uattr,
Peter Zijlstra
maintainerPATCH v3 50/57 perf: Simplify sys_perf_event_open()
In reply to Peter Zijlstra--- include/linux/file.h | 3 kernel/events/core.c | 483 +++++++++++++++++++++++---------------------------- 2 files changed, 222 insertions(+), 264 deletions(-)
include/linux/file.h +3 -0
@@ -84,6 +84,7 @@ static inline void fdput_pos(struct fd f } DEFINE_CLASS(fd, struct fd, fdput(_T), fdget(fd), int fd) +DEFINE_FREE(fdput, struct fd, fdput(_T))extern int f_dupfd (unsigned int from,struct file *file,unsigned flags);extern int replace_fd (unsigned fd,struct file *file,unsigned flags);
@@ -96,6 +97,8 @@ extern void put_unused_fd(unsigned int f DEFINE_CLASS(get_unused_fd, int, if (_T >= 0) put_unused_fd(_T), get_unused_fd_flags(flags), unsigned flags) +#define no_free_fd (fd) ({ int __fd = (fd); (fd) = -1; __fd; }) +extern void fd_install (unsigned int fd,struct file *file); extern int __receive_fd(struct file *file, int __user *ufd,
kernel/events/core.c +219 -264
617 lines not on this page · raw message
Peter Zijlstra
maintainerPATCH v3 51/57 perf: Simplify perf_event_create_kernel_counter()
In reply to Peter Zijlstra--- kernel/events/core.c | 79 ++++++++++++++++----------------------------------- 1 file changed, 26 insertions(+), 53 deletions(-)
kernel/events/core.c +26 -53
@@ -12569,12 +12569,6 @@ perf_event_create_kernel_counter(structperf_overflow_handler_t overflow_handler, void *context) { - struct perf_event_pmu_context *pmu_ctx; -struct perf_event_context *ctx; -struct perf_event *event; -struct pmu *pmu; -int err; -/* * Grouping is not supported for kernel events, neither is 'AUX', * make sure the caller's intentions are adjusted.
@@ -12582,16 +12576,16 @@ perf_event_create_kernel_counter(structif (attr->aux_output)return ERR_PTR (-EINVAL ); - event =perf_event_alloc (attr, cpu, task,NULL ,NULL , + + struct perf_event *event __free(free_event) = +perf_event_alloc (attr, cpu, task,NULL ,NULL , overflow_handler, context,-1 ); -if (IS_ERR (event)) { - err =PTR_ERR (event); - goto err; - } +if (IS_ERR (event)) +return event;/* Mark owner so we could distinguish it from user events. */ event->owner =TASK_TOMBSTONE ; - pmu = event->pmu; +struct pmu *pmu = event->pmu;if (pmu->task_ctx_nr == perf_sw_context) event->event_caps |=PERF_EV_CAP_SOFTWARE ;
@@ -12599,25 +12593,21 @@ perf_event_create_kernel_counter(struct/* * Get the target context (task or percpu): */ - ctx =find_get_context (task, event); -if (IS_ERR (ctx)) { - err =PTR_ERR (ctx); - goto err_alloc; - } +CLASS (find_get_ctx, ctx)(task, event); +if (IS_ERR (ctx)) +return (void *)ctx;WARN_ON_ONCE (ctx->parent_ctx); -mutex_lock (&ctx->mutex); -if (ctx->task ==TASK_TOMBSTONE ) { - err = -ESRCH ; - goto err_unlock; - } +guard (mutex)(&ctx->mutex); - pmu_ctx =find_get_pmu_context (pmu, ctx, event); -if (IS_ERR (pmu_ctx)) { - err =PTR_ERR (pmu_ctx); - goto err_unlock; - } - event->pmu_ctx = pmu_ctx; +if (ctx->task ==TASK_TOMBSTONE ) +return ERR_PTR (-ESRCH ); + + + struct perf_event_pmu_context *pmu_ctx __free(put_pmu_ctx) = +find_get_pmu_context (pmu, ctx, event); +if (!pmu_ctx) +return ERR_PTR (-ENOMEM );if (!task) {/*
@@ -12628,34 +12618,17 @@ perf_event_create_kernel_counter(struct*/ struct perf_cpu_context *cpuctx = container_of(ctx, struct perf_cpu_context, ctx); -if (!cpuctx->online) { - err = -ENODEV ; - goto err_pmu_ctx; - } +if (!cpuctx->online) +return ERR_PTR (-ENODEV ); } -if (!exclusive_event_installable (event, ctx)) { - err = -EBUSY ; - goto err_pmu_ctx; - } +if (!exclusive_event_installable (event, ctx)) +return ERR_PTR (-EBUSY ); + + event->pmu_ctx =no_free_ptr (pmu_ctx); +perf_install_in_context (get_ctx (ctx), event, event->cpu); -perf_install_in_context (ctx, event, event->cpu); -perf_unpin_context (ctx); -mutex_unlock (&ctx->mutex); - -return event; - -err_pmu_ctx: -put_pmu_ctx (pmu_ctx); - event->pmu_ctx =NULL ;/* _free_event() */ -err_unlock: -mutex_unlock (&ctx->mutex); -perf_unpin_context (ctx); -put_ctx (ctx); -err_alloc: -free_event (event); -err: -return ERR_PTR (err); +return_ptr (event); }EXPORT_SYMBOL_GPL (perf_event_create_kernel_counter);
Peter Zijlstra
maintainerPATCH v3 52/57 perf: Simplify perf_event_init_context()
In reply to Peter Zijlstra--- kernel/events/core.c | 29 +++++++++++------------------ 1 file changed, 11 insertions(+), 18 deletions(-)
kernel/events/core.c +11 -18
@@ -1450,6 +1450,10 @@ static void perf_unpin_context(struct peraw_spin_unlock_irqrestore (&ctx->lock, flags); } +DEFINE_CLASS(pin_task_ctx, struct perf_event_context *, + if (_T) { perf_unpin_context(_T); put_ctx(_T); }, + perf_pin_task_context(task), struct task_struct *task) +/* * Update the record of the current time in a context. */
@@ -7939,18 +7943,13 @@ static void perf_event_addr_filters_execvoid perf_event_exec (void ) { -struct perf_event_context *ctx; - - ctx =perf_pin_task_context (current); +CLASS (pin_task_ctx, ctx)(current);if (!ctx)return ;perf_event_enable_on_exec (ctx);perf_event_remove_on_exec (ctx);perf_iterate_ctx (ctx, perf_event_addr_filters_exec,NULL , true); - -perf_unpin_context (ctx); -put_ctx (ctx); }struct remote_output {
@@ -13226,8 +13225,7 @@ inherit_task_group(struct perf_event *ev*/ static int perf_event_init_context (struct task_struct *child,u64 clone_flags) { -struct perf_event_context *child_ctx, *parent_ctx; -struct perf_event_context *cloned_ctx; +struct perf_event_context *child_ctx, *cloned_ctx;struct perf_event *event;struct task_struct *parent = current;int inherited_all =1 ;
@@ -13241,7 +13239,7 @@ static int perf_event_init_context(struc* If the parent's context is a clone, pin it so it won't get * swapped under us. */ - parent_ctx =perf_pin_task_context (parent); +CLASS (pin_task_ctx, parent_ctx)(parent);if (!parent_ctx)return 0 ;
@@ -13256,7 +13254,7 @@ static int perf_event_init_context(struc* Lock the parent list. No need to lock the child - not PID * hashed yet and not running, so nobody can access it. */ -mutex_lock (&parent_ctx->mutex); +guard (mutex)(&parent_ctx->mutex);/* * We dont have to disable NMIs - we are only looking at
@@ -13266,7 +13264,7 @@ static int perf_event_init_context(struc ret =inherit_task_group (event, parent, parent_ctx, child, clone_flags, &inherited_all);if (ret) - goto out_unlock; +return ret; }/*
@@ -13282,7 +13280,7 @@ static int perf_event_init_context(struc ret =inherit_task_group (event, parent, parent_ctx, child, clone_flags, &inherited_all);if (ret) - goto out_unlock; +return ret; }raw_spin_lock_irqsave (&parent_ctx->lock, flags);
@@ -13310,13 +13308,8 @@ static int perf_event_init_context(struc }raw_spin_unlock_irqrestore (&parent_ctx->lock, flags); -out_unlock: -mutex_unlock (&parent_ctx->mutex); - -perf_unpin_context (parent_ctx); -put_ctx (parent_ctx); -return ret; +return 0 ; }/*
Peter Zijlstra
maintainerPATCH v3 53/57 perf: Simplify perf_event_sysfs_init()
In reply to Peter Zijlstra--- kernel/events/core.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
kernel/events/core.c +3 -7
@@ -13503,11 +13503,11 @@ static int __init perf_event_sysfs_init(struct pmu *pmu;int ret; -mutex_lock (&pmus_lock); +guard (mutex)(&pmus_lock); ret =bus_register (&pmu_bus);if (ret) - goto unlock; +return ret;list_for_each_entry (pmu, &pmus, entry) {if (pmu->dev)
@@ -13517,12 +13517,8 @@ static int __init perf_event_sysfs_init(WARN (ret,"Failed to register pmu: %s, reason %d\n" , pmu->name, ret); } pmu_bus_running =1 ; - ret =0 ; -unlock: -mutex_unlock (&pmus_lock); - -return ret; +return 0 ; }device_initcall (perf_event_sysfs_init);
Peter Zijlstra
maintainerPATCH v3 54/57 perf: Misc cleanups
In reply to Peter Zijlstra--- kernel/events/core.c | 64 +++++++++++++++++++-------------------------------- 1 file changed, 25 insertions(+), 39 deletions(-)
kernel/events/core.c +25 -39
@@ -1274,13 +1274,11 @@ perf_event_ctx_lock_nested(struct perf_estruct perf_event_context *ctx; again: -rcu_read_lock (); - ctx =READ_ONCE (event->ctx); -if (!refcount_inc_not_zero (&ctx->refcount)) { -rcu_read_unlock (); - goto again; + scoped_guard (rcu) { + ctx =READ_ONCE (event->ctx); +if (!refcount_inc_not_zero (&ctx->refcount)) + goto again; } -rcu_read_unlock ();mutex_lock_nested (&ctx->mutex, nesting);if (event->ctx != ctx) {
@@ -2254,7 +2252,7 @@ event_sched_out(struct perf_event *event*/ list_del_init (&event->active_list); -perf_pmu_disable (event->pmu); +guard (perf_pmu_disable)(event->pmu); event->pmu->del (event,0 ); event->oncpu =-1 ;
@@ -2288,8 +2286,6 @@ event_sched_out(struct perf_event *event ctx->nr_freq--;if (event->attr.exclusive || !cpc->active_oncpu) cpc->exclusive =0 ; - -perf_pmu_enable (event->pmu); } static void
@@ -3219,7 +3215,8 @@ static void __pmu_ctx_sched_out(struct pif (!event_type)return ; -perf_pmu_disable (pmu); +guard (perf_pmu_disable)(pmu); +if (event_type &EVENT_PINNED ) { list_for_each_entry_safe(event, tmp, &pmu_ctx->pinned_active,
@@ -3239,7 +3236,6 @@ static void __pmu_ctx_sched_out(struct p*/ pmu_ctx->rotate_necessary =0 ; } -perf_pmu_enable (pmu); } static void
@@ -3586,13 +3582,10 @@ static void __perf_pmu_sched_task(structif (WARN_ON_ONCE (!pmu->sched_task))return ; -perf_ctx_lock (cpuctx, cpuctx->task_ctx); -perf_pmu_disable (pmu); +guard (perf_ctx_lock)(cpuctx, cpuctx->task_ctx); +guard (perf_pmu_disable)(pmu); pmu->sched_task (cpc->task_epc, sched_in); - -perf_pmu_enable (pmu); -perf_ctx_unlock (cpuctx, cpuctx->task_ctx); } static void perf_pmu_sched_task(struct task_struct *prev,
@@ -12655,8 +12648,6 @@ static void __perf_pmu_install_event(strstruct perf_event_context *ctx, int cpu, struct perf_event *event) { - struct perf_event_pmu_context *epc; -/* * Now that the events are unused, put their old ctx and grab a * reference on the new context.
@@ -12665,8 +12656,7 @@ static void __perf_pmu_install_event(strget_ctx (ctx); event->cpu = cpu; - epc =find_get_pmu_context (pmu, ctx, event); - event->pmu_ctx = epc; + event->pmu_ctx =find_get_pmu_context (pmu, ctx, event);if (event->state >=PERF_EVENT_STATE_OFF ) event->state =PERF_EVENT_STATE_INACTIVE ;
@@ -12815,12 +12805,12 @@ perf_event_exit_event(struct perf_eventstatic void perf_event_exit_task_context (struct task_struct *child) { -struct perf_event_context *child_ctx, *clone_ctx =NULL ; +struct perf_event_context *clone_ctx =NULL ;struct perf_event *child_event, *next;WARN_ON_ONCE (child != current); - child_ctx =perf_pin_task_context (child); +CLASS (pin_task_ctx, child_ctx)(child);if (!child_ctx)return ;
@@ -12834,27 +12824,27 @@ static void perf_event_exit_task_context* without ctx::mutex (it cannot because of the move_group double mutex * lock thing). See the comments in perf_install_in_context(). */ -mutex_lock (&child_ctx->mutex); +guard (mutex)(&child_ctx->mutex);/* * In a single ctx::lock section, de-schedule the events and detach the * context from the task such that we cannot ever get it scheduled back * in. */ -raw_spin_lock_irq (&child_ctx->lock); -task_ctx_sched_out (child_ctx,EVENT_ALL ); + scoped_guard (raw_spinlock_irq, &child_ctx->lock) { +task_ctx_sched_out (child_ctx,EVENT_ALL ); -/* -* Now that the context is inactive, destroy the task <-> ctx relation -* and mark the context dead. -*/ -RCU_INIT_POINTER (child->perf_event_ctxp,NULL ); -put_ctx (child_ctx);/* cannot be last */ -WRITE_ONCE (child_ctx->task,TASK_TOMBSTONE ); -put_task_struct (current);/* cannot be last */ +/* +* Now that the context is inactive, destroy the task <-> ctx +* relation and mark the context dead. +*/ +RCU_INIT_POINTER (child->perf_event_ctxp,NULL ); +put_ctx (child_ctx);/* cannot be last */ +WRITE_ONCE (child_ctx->task,TASK_TOMBSTONE ); +put_task_struct (current);/* cannot be last */ - clone_ctx =unclone_ctx (child_ctx); -raw_spin_unlock_irq (&child_ctx->lock); + clone_ctx =unclone_ctx (child_ctx); + }if (clone_ctx)put_ctx (clone_ctx);
@@ -12868,10 +12858,6 @@ static void perf_event_exit_task_contextlist_for_each_entry_safe (child_event, next, &child_ctx->event_list, event_entry)perf_event_exit_event (child_event, child_ctx); - -mutex_unlock (&child_ctx->mutex); - -put_ctx (child_ctx); }/*
Peter Zijlstra
maintainerPATCH v3 55/57 perf: Simplify find_get_context()
In reply to Peter Zijlstra--- kernel/events/core.c | 46 ++++++++++++++++++---------------------------- 1 file changed, 18 insertions(+), 28 deletions(-)
kernel/events/core.c +18 -28
@@ -1202,6 +1202,8 @@ static void put_ctx(struct perf_event_co } } +DEFINE_FREE(put_ctx, struct perf_event_context *, if (_T) put_ctx(_T)) +/* * Because of perf_event::ctx migration in sys_perf_event_open::move_group and * perf_pmu_migrate_context() we need some magic.
@@ -4718,41 +4720,29 @@ find_get_context(struct task_struct *tasif (clone_ctx)put_ctx (clone_ctx); }else { - ctx =alloc_perf_context (task); - err = -ENOMEM ; -if (!ctx) - goto errout; - - err =0 ; -mutex_lock (&task->perf_event_mutex); -/* -* If it has already passed perf_event_exit_task(). -* we must see PF_EXITING, it takes this mutex too. -*/ -if (task->flags &PF_EXITING ) - err = -ESRCH ; -else if (task->perf_event_ctxp) - err = -EAGAIN ; -else { -get_ctx (ctx); - ++ctx->pin_count; -rcu_assign_pointer (task->perf_event_ctxp, ctx); - } -mutex_unlock (&task->perf_event_mutex); + struct perf_event_context *new __free(put_ctx) = +alloc_perf_context (task); +if (!new) +return ERR_PTR (-ENOMEM ); -if (unlikely (err)) { -put_ctx (ctx); + scoped_guard (mutex, &task->perf_event_mutex) { +/* +* If it has already passed perf_event_exit_task(). +* we must see PF_EXITING, it takes this mutex too. +*/ +if (task->flags &PF_EXITING ) +return ERR_PTR (-ESRCH ); -if (err == -EAGAIN ) +if (task->perf_event_ctxp) goto retry; - goto errout; + + ctx =get_ctx (no_free_ptr (new)); + ++ctx->pin_count; +rcu_assign_pointer (task->perf_event_ctxp, ctx); } }return ctx; - -errout: -return ERR_PTR (err); } DEFINE_CLASS(find_get_ctx, struct perf_event_context *,
Peter Zijlstra
maintainerPATCH v3 56/57 perf: Simplify perf_pmu_output_stop()
In reply to Peter Zijlstra--- kernel/events/core.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
kernel/events/core.c +3 -5
@@ -7977,7 +7977,8 @@ static void perf_pmu_output_stop(structint err, cpu; restart: -rcu_read_lock (); +/* cannot have a label in front of a decl */ ; +guard (rcu)();list_for_each_entry_rcu (iter, &event->rb->event_list, rb_entry) {/* * For per-CPU events, we need to make sure that neither they
@@ -7993,12 +7994,9 @@ static void perf_pmu_output_stop(structcontinue ; err =cpu_function_call (cpu, __perf_pmu_output_stop, event); -if (err == -EAGAIN ) { -rcu_read_unlock (); +if (err == -EAGAIN ) goto restart; - } } -rcu_read_unlock (); }/*
Re: [PATCH v3 56/57] perf: Simplify perf_pmu_output_stop()
This patch looks completely broken to me.
You now do
if (err == -EAGAIN)
goto restart;*within* the RCU-guarded section, and the "goto restart" will guard it again.
So no. Sending out a series of 57 patches that can have these kinds of bugs in it is not ok. By patch 56 (which just happened to come in fairly early for me), all sane peoples eyes have glazed over and they don't react to this kind of breakage any more.
Linus
Quoted 25 lines · Peter Zijlstra
kernel/events/core.c +3
-5@@ -7977,7 +7977,8 @@ static void perf_pmu_output_stop(structint err, cpu; restart: -rcu_read_lock (); +/* cannot have a label in front of a decl */ ; +guard (rcu)();list_for_each_entry_rcu (iter, &event->rb->event_list, rb_entry) {/* * For per-CPU events, we need to make sure that neither they @@ -7993,12 +7994,9 @@ static void perf_pmu_output_stop(structcontinue ; err =cpu_function_call (cpu, __perf_pmu_output_stop, event); -if (err == -EAGAIN ) { -rcu_read_unlock (); +if (err == -EAGAIN ) goto restart; - } } -rcu_read_unlock (); }
Re: [PATCH v3 56/57] perf: Simplify perf_pmu_output_stop()
Quoted 8 lines · Linus Torvalds
This patch looks completely broken to me.
You now do
if (err == -EAGAIN) goto restart;*within* the RCU-guarded section, and the "goto restart" will guard it again.
What if we require that all guarded sections have explicit scoping? E.g. drop the current version of guard() and rename scoped_guard() => guard(). And then figure out macro magic to guard an entire function? E.g. something like
static void perf_pmu_output_stop(struct perf_event *event) fn_guard(rcu)
{
...}
or just "guard(rcu)" if possible. IIUC, function scopes like that will be possible once -Wdeclaration-after-statement goes away.
Bugs aside, IMO guards that are buried in the middle of a function and implicitly scoped to the function are all too easy to overlook. Requiring explicit scoping would make bugs like this easier to spot since the goto would jump out of scope (and I assume prematurely release the resource/lock?). As a bonus, annotating the function itself would also serve as documentation.
The only downside is that the code for function-scoped locks that are acquired partway through the function would be more verbose and/or cumbersome to write, but that can be mitigated to some extent, e.g. by moving the locked portion to a separate helper.
Quoted 26 lines · Peter Zijlstra
Quoted 25 lines
kernel/events/core.c +3
-5@@ -7977,7 +7977,8 @@ static void perf_pmu_output_stop(structint err, cpu; restart: -rcu_read_lock (); +/* cannot have a label in front of a decl */ ; +guard (rcu)();list_for_each_entry_rcu (iter, &event->rb->event_list, rb_entry) {/* * For per-CPU events, we need to make sure that neither they @@ -7993,12 +7994,9 @@ static void perf_pmu_output_stop(structcontinue ; err =cpu_function_call (cpu, __perf_pmu_output_stop, event); -if (err == -EAGAIN ) { -rcu_read_unlock (); +if (err == -EAGAIN ) goto restart; - } } -rcu_read_unlock (); }
Re: [PATCH v3 56/57] perf: Simplify perf_pmu_output_stop()
Quoted 4 lines · Sean Christopherson
What if we require that all guarded sections have explicit scoping? E.g. drop the current version of
guard()and renamescoped_guard()=>guard(). And then figure out macro magic to guard an entire function?
Hmm. I didn't love the excessive scoping, but most of the cases I was thinking about were for the freeing part, not the locking part.
I agree that explicit scoping might be a good idea for locks as a rule, but that "entire function" case _is_ a special case, and I don't see how to do it sanely with a scoping guard.
Unless you accept ugly syntax like
int fn(..)
{ scoped_guard(rcu)() {
...
}}which is just a violation of our usual scoping indentation rules.
Of course, at that point, the "scoped" part doesn't actually buy us anything either, so you'd probably just be better off listing all the guarding locks, and make it be
int fn(..)
{ guard(rcu)(); guard(mutex)(&mymutex); {
...
}}or whatever.
Ugly, ugly.
End result: I think the non-explicitly scoped syntax is pretty much required for sane use. The scoped version just causes too much indentation (or forces us to have the above kind of special "we don't indent this" rules).
Quoted 2 lines
IIUC, function scopes like that will be possible once -Wdeclaration-after-statement goes away.
Well, "-Wdeclaration-after-statement" already went away early in Peter's series, because without that you can't sanely do the normal "__free()" cleanup thng.
But no, it doesn't help the C syntax case.
If you were to wrap a whole function with a macro, you need to do some rather ugly things. They are ugly things that we already do: see our whole "SYSCALL_DEFINEx()" set of macros, so it's not *impossible*, but it's not possible with normal C syntax (and the normal C preprocessor).
Of course, one way to do the "whole function scope" is to just do it in the caller, and not using the cleanup attribute at all.
IOW, we could have things like
#define WRAP(a,b,c) \
({ __typeof__(b) __ret; a; __ret = (b); c; __ret; })and then you can do things like
#define guard_fn_mutex(mutex, fn) \
WRAP(mutex_lock(mutex), fn, mutex_unlock(mutex))or
#define rcu_read_action(x) WRAP(rcu_read_lock(), x, rcu_read_unlock())
and now you can easily guard the call-site (or any simple expression that doesn't include any non-local control flow). Nothing new and fancy required.
But when you don't want to do the wrapping in the caller, you do want to have a non-scoping guard at the top of the function, I suspect.
Linus
Peter Zijlstra
maintainerRe: [PATCH v3 56/57] perf: Simplify perf_pmu_output_stop()
In reply to Linus TorvaldsQuoted 8 lines · Linus Torvalds
This patch looks completely broken to me.
You now do
if (err == -EAGAIN) goto restart;*within* the RCU-guarded section, and the "goto restart" will guard it again.
restart:
guard(rcu)();
list_for_each_entry_rcu(iter, &head, rb_entry) {
...
if (err == -EAGAIN)
goto restart;
}So the restart is *before* the variable exists, eg. it's out-of-scope.
per the last email's guard.c, if changed like so:
void main(void)
{
int done = 0;
restart:
lock_guard(spin, moo, &lock);
for (;!done;) {
done = 1;
goto restart;
}
}$ gcc -O2 -o guard guard.c && ./guard spin_lock spin_unlock spin_lock spin_unlock
Which is exactly the expected result.
Peter Zijlstra
maintainerPATCH v3 57/57 perf: Simplify perf_install_in_context()
In reply to Peter Zijlstra--- kernel/events/core.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
kernel/events/core.c +2 -6
@@ -2876,7 +2876,7 @@ perf_install_in_context(struct perf_evenif (!task_function_call (task, __perf_install_in_context, event))return ; -raw_spin_lock_irq (&ctx->lock); +guard (raw_spinlock_irq)(&ctx->lock); task = ctx->task;if (WARN_ON_ONCE (task ==TASK_TOMBSTONE )) {/*
@@ -2884,19 +2884,15 @@ perf_install_in_context(struct perf_even* cannot happen), and we hold ctx->mutex, which serializes us * against perf_event_exit_task_context(). */ -raw_spin_unlock_irq (&ctx->lock);return ; }/* * If the task is not running, ctx->lock will avoid it becoming so, * thus we can safely install the event. */ -if (task_curr (task)) { -raw_spin_unlock_irq (&ctx->lock); +if (task_curr (task)) goto again; - }add_event_to_ctx (event, ctx); -raw_spin_unlock_irq (&ctx->lock); }/*
Peter Zijlstra
maintainerQuoted 19 lines · Peter Zijlstra
Hi,
After a wee bit of bike-shedding on the syntax/API of the thing I think we're in a reasonable shape.
There's still the
no_free_ptr()vstake_ptr()thing, but that can be easily sorted with a little script over the patches if we reach consensus.I've taken to converting
kernel/sched/core.candkernel/events/core.cto see how well this stuff actually works. Unlike last time, I've split them up into a gazillion little patches. Both for my sanity -- bisect is genius when you're trying to debug stuff in the middle of the night as well as reviewer sanity.These are by no means 'complete' convertions, I've mostly focussed on functions that had 'goto' in them. Since that's a large part of why I started on all this.
x86_x64-defconfig boots and passes perf test. I'll go figure out how to point syzcaller at it.
Quoted 8 lines
The patches have gone through a few cycles of 0day over the weekend, and mostly builds fine now.
Dan Carpenter poked me about how sparse doesn't yet understand the
__cleanup__attribute and seems to suffer from decl-after-stmt issues, so that might be something that needs attention.Anyway, does this look like something we can live with?
Forgot to mention; also available at:
git://git.kernel.org/pub/scm/linux/kernel/git/peterz/queue.git core/guards2
(core/guards also exists, but that is a previous version that I kept because I wanted to discuss some 'interesting' clang build fails)
Quoted 29 lines
--- Makefile | 6 +- arch/arm64/kernel/vdso32/Makefile | 2 - drivers/dma/ioat/dma.c | 12 +- include/linux/cleanup.h | 167 ++++ include/linux/compiler-clang.h | 9 + include/linux/compiler_attributes.h | 6 + include/linux/cpu.h | 2 + include/linux/device.h | 7 + include/linux/file.h | 11 + include/linux/irqflags.h | 7 + include/linux/mutex.h | 4 + include/linux/percpu.h | 4 + include/linux/perf_event.h | 14 +- include/linux/preempt.h | 5 + include/linux/rcupdate.h | 3 + include/linux/rwsem.h | 8 + include/linux/sched/task.h | 2 + include/linux/slab.h | 3 + include/linux/spinlock.h | 31 + include/linux/srcu.h | 5 + kernel/events/core.c | 1642 +++++++++++++++-------------------- kernel/sched/core.c | 931 +++++++++----------- kernel/sched/sched.h | 40 + scripts/checkpatch.pl | 2 +- security/apparmor/include/lib.h | 6 +- 25 files changed, 1433 insertions(+), 1496 deletions(-)
Quoted 3 lines · Peter Zijlstra
These are by no means 'complete' convertions, I've mostly focussed on functions that had 'goto' in them. Since that's a large part of why I started on all this.
So I found what looks like two patches being completely broken because the conversion was fundamentally misguided.
And honestly, by the end of walking through the patches I was just scanning so quickly that I might have missed some.
Let's make the rule be that
- max 10 patches in the series so that I think they were actually
thought about, and people can actually review them
- only clear "goto out of scope" conversions. When you see a
"continue" or a "goto repeat", you TAKE A BIG STEP BACK.
Because this is all clever, and I am now happy with the syntax, but I am *not* happy with that 60-patch monster series with what looks like horrible bugs.
Linus
[RFC] Expanding Scope-based Resource Management
In reply to Peter ZijlstraHi Peter,
I greatly admire the Scope-based Resource Management infrastructure you introduced to the Linux kernel, as it elegantly aligns with the Resource Acquisition Is Initialization (RAII) programming idiom, improving code safety and maintainability.
I am interested in driving a comprehensive conversion of traditional manual lock/unlock patterns to use guard/scoped_guard, starting with the sched module.
Before proceeding, I’d like to confirm if you believe this effort is valuable and whether you’d support such a conversion.
Best,
Jemmy