| Message ID | 20231212094725.22184-6-jgross@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/spinlock: make recursive spinlocks a dedicated type | expand |
Hi Juergen, On 12/12/2023 09:47, Juergen Gross wrote: > Instead of special casing rspin_lock_irqsave() and > rspin_unlock_irqrestore() for the console lock, add those functions > to spinlock handling and use them where needed. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V2: > - new patch > --- > xen/arch/x86/traps.c | 14 ++++++++------ > xen/common/spinlock.c | 16 ++++++++++++++++ > xen/drivers/char/console.c | 18 +----------------- > xen/include/xen/console.h | 5 +++-- > xen/include/xen/spinlock.h | 7 +++++++ > 5 files changed, 35 insertions(+), 25 deletions(-) > > diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c > index 7724306116..21227877b3 100644 > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -647,13 +647,15 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs) > void show_execution_state(const struct cpu_user_regs *regs) > { > /* Prevent interleaving of output. */ > - unsigned long flags = console_lock_recursive_irqsave(); > + unsigned long flags; > + > + rspin_lock_irqsave(&console_lock, flags); > > show_registers(regs); > show_code(regs); > show_stack(regs); > > - console_unlock_recursive_irqrestore(flags); > + rspin_unlock_irqrestore(&console_lock, flags); > } > > void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs) > @@ -663,7 +665,7 @@ void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs) > > void vcpu_show_execution_state(struct vcpu *v) > { > - unsigned long flags = 0; > + unsigned long flags; > > if ( test_bit(_VPF_down, &v->pause_flags) ) > { > @@ -698,7 +700,7 @@ void vcpu_show_execution_state(struct vcpu *v) > #endif > > /* Prevent interleaving of output. */ > - flags = console_lock_recursive_irqsave(); > + rspin_lock_irqsave(&console_lock, flags); > > vcpu_show_registers(v); > > @@ -708,7 +710,7 @@ void vcpu_show_execution_state(struct vcpu *v) > * Stop interleaving prevention: The necessary P2M lookups involve > * locking, which has to occur with IRQs enabled. > */ > - console_unlock_recursive_irqrestore(flags); > + rspin_unlock_irqrestore(&console_lock, flags); > > show_hvm_stack(v, &v->arch.user_regs); > } > @@ -717,7 +719,7 @@ void vcpu_show_execution_state(struct vcpu *v) > if ( guest_kernel_mode(v, &v->arch.user_regs) ) > show_guest_stack(v, &v->arch.user_regs); > > - console_unlock_recursive_irqrestore(flags); > + rspin_unlock_irqrestore(&console_lock, flags); > } > > #ifdef CONFIG_HVM > diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c > index 422a7fb1db..c1a9ba1304 100644 > --- a/xen/common/spinlock.c > +++ b/xen/common/spinlock.c > @@ -475,6 +475,16 @@ void rspin_lock(rspinlock_t *lock) > lock->recurse_cnt++; > } > > +unsigned long __rspin_lock_irqsave(rspinlock_t *lock) This is going to be a problem with MISRA (see Rule 21.1). Can you move the double underscore to the end? Alternatively, I am not sure I see the benefits of the function here. So maybe we can fold the code in the macro below? Other than that, the rest of the patch LGTM. Cheers,
On 12.12.23 14:03, Julien Grall wrote: > Hi Juergen, > > On 12/12/2023 09:47, Juergen Gross wrote: >> Instead of special casing rspin_lock_irqsave() and >> rspin_unlock_irqrestore() for the console lock, add those functions >> to spinlock handling and use them where needed. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> V2: >> - new patch >> --- >> xen/arch/x86/traps.c | 14 ++++++++------ >> xen/common/spinlock.c | 16 ++++++++++++++++ >> xen/drivers/char/console.c | 18 +----------------- >> xen/include/xen/console.h | 5 +++-- >> xen/include/xen/spinlock.h | 7 +++++++ >> 5 files changed, 35 insertions(+), 25 deletions(-) >> >> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c >> index 7724306116..21227877b3 100644 >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -647,13 +647,15 @@ void show_stack_overflow(unsigned int cpu, const struct >> cpu_user_regs *regs) >> void show_execution_state(const struct cpu_user_regs *regs) >> { >> /* Prevent interleaving of output. */ >> - unsigned long flags = console_lock_recursive_irqsave(); >> + unsigned long flags; >> + >> + rspin_lock_irqsave(&console_lock, flags); >> show_registers(regs); >> show_code(regs); >> show_stack(regs); >> - console_unlock_recursive_irqrestore(flags); >> + rspin_unlock_irqrestore(&console_lock, flags); >> } >> void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs) >> @@ -663,7 +665,7 @@ void cf_check show_execution_state_nonconst(struct >> cpu_user_regs *regs) >> void vcpu_show_execution_state(struct vcpu *v) >> { >> - unsigned long flags = 0; >> + unsigned long flags; >> if ( test_bit(_VPF_down, &v->pause_flags) ) >> { >> @@ -698,7 +700,7 @@ void vcpu_show_execution_state(struct vcpu *v) >> #endif >> /* Prevent interleaving of output. */ >> - flags = console_lock_recursive_irqsave(); >> + rspin_lock_irqsave(&console_lock, flags); >> vcpu_show_registers(v); >> @@ -708,7 +710,7 @@ void vcpu_show_execution_state(struct vcpu *v) >> * Stop interleaving prevention: The necessary P2M lookups involve >> * locking, which has to occur with IRQs enabled. >> */ >> - console_unlock_recursive_irqrestore(flags); >> + rspin_unlock_irqrestore(&console_lock, flags); >> show_hvm_stack(v, &v->arch.user_regs); >> } >> @@ -717,7 +719,7 @@ void vcpu_show_execution_state(struct vcpu *v) >> if ( guest_kernel_mode(v, &v->arch.user_regs) ) >> show_guest_stack(v, &v->arch.user_regs); >> - console_unlock_recursive_irqrestore(flags); >> + rspin_unlock_irqrestore(&console_lock, flags); >> } >> #ifdef CONFIG_HVM >> diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c >> index 422a7fb1db..c1a9ba1304 100644 >> --- a/xen/common/spinlock.c >> +++ b/xen/common/spinlock.c >> @@ -475,6 +475,16 @@ void rspin_lock(rspinlock_t *lock) >> lock->recurse_cnt++; >> } >> +unsigned long __rspin_lock_irqsave(rspinlock_t *lock) > > This is going to be a problem with MISRA (see Rule 21.1). Can you move the > double underscore to the end? Alternatively, I am not sure I see the benefits of > the function here. So maybe we can fold the code in the macro below? I think I'll follow the common pattern and just rename the function as you suggest. Juergen
On 12.12.2023 10:47, Juergen Gross wrote: > Instead of special casing rspin_lock_irqsave() and > rspin_unlock_irqrestore() for the console lock, add those functions > to spinlock handling and use them where needed. > > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > V2: > - new patch In how far is this a necessary part of the series? > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -647,13 +647,15 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs) > void show_execution_state(const struct cpu_user_regs *regs) > { > /* Prevent interleaving of output. */ > - unsigned long flags = console_lock_recursive_irqsave(); > + unsigned long flags; > + > + rspin_lock_irqsave(&console_lock, flags); > > show_registers(regs); > show_code(regs); > show_stack(regs); > > - console_unlock_recursive_irqrestore(flags); > + rspin_unlock_irqrestore(&console_lock, flags); > } > > void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs) > @@ -663,7 +665,7 @@ void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs) > > void vcpu_show_execution_state(struct vcpu *v) > { > - unsigned long flags = 0; > + unsigned long flags; > > if ( test_bit(_VPF_down, &v->pause_flags) ) > { > @@ -698,7 +700,7 @@ void vcpu_show_execution_state(struct vcpu *v) > #endif > > /* Prevent interleaving of output. */ > - flags = console_lock_recursive_irqsave(); > + rspin_lock_irqsave(&console_lock, flags); > > vcpu_show_registers(v); > > @@ -708,7 +710,7 @@ void vcpu_show_execution_state(struct vcpu *v) > * Stop interleaving prevention: The necessary P2M lookups involve > * locking, which has to occur with IRQs enabled. > */ > - console_unlock_recursive_irqrestore(flags); > + rspin_unlock_irqrestore(&console_lock, flags); > > show_hvm_stack(v, &v->arch.user_regs); > } > @@ -717,7 +719,7 @@ void vcpu_show_execution_state(struct vcpu *v) > if ( guest_kernel_mode(v, &v->arch.user_regs) ) > show_guest_stack(v, &v->arch.user_regs); > > - console_unlock_recursive_irqrestore(flags); > + rspin_unlock_irqrestore(&console_lock, flags); > } > I view these as layering violations; ... > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -120,7 +120,7 @@ static int __read_mostly sercon_handle = -1; > int8_t __read_mostly opt_console_xen; /* console=xen */ > #endif > > -static DEFINE_RSPINLOCK(console_lock); > +DEFINE_RSPINLOCK(console_lock); ... this should remain static. The question therefore just is whether to omit this patch or ... > @@ -1158,22 +1158,6 @@ void console_end_log_everything(void) > atomic_dec(&print_everything); > } > > -unsigned long console_lock_recursive_irqsave(void) > -{ > - unsigned long flags; > - > - local_irq_save(flags); > - rspin_lock(&console_lock); > - > - return flags; > -} > - > -void console_unlock_recursive_irqrestore(unsigned long flags) > -{ > - rspin_unlock(&console_lock); > - local_irq_restore(flags); > -} ... whether to retain these two functions as thin wrappers around the new, more generic construct. Jan
On 28.02.24 16:09, Jan Beulich wrote: > On 12.12.2023 10:47, Juergen Gross wrote: >> Instead of special casing rspin_lock_irqsave() and >> rspin_unlock_irqrestore() for the console lock, add those functions >> to spinlock handling and use them where needed. >> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> V2: >> - new patch > > In how far is this a necessary part of the series? Not really necessary. It just seemed wrong to have an open coded variant of rspin_lock_irqsave() and rspin_unlock_irqrestore(). > >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -647,13 +647,15 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs) >> void show_execution_state(const struct cpu_user_regs *regs) >> { >> /* Prevent interleaving of output. */ >> - unsigned long flags = console_lock_recursive_irqsave(); >> + unsigned long flags; >> + >> + rspin_lock_irqsave(&console_lock, flags); >> >> show_registers(regs); >> show_code(regs); >> show_stack(regs); >> >> - console_unlock_recursive_irqrestore(flags); >> + rspin_unlock_irqrestore(&console_lock, flags); >> } >> >> void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs) >> @@ -663,7 +665,7 @@ void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs) >> >> void vcpu_show_execution_state(struct vcpu *v) >> { >> - unsigned long flags = 0; >> + unsigned long flags; >> >> if ( test_bit(_VPF_down, &v->pause_flags) ) >> { >> @@ -698,7 +700,7 @@ void vcpu_show_execution_state(struct vcpu *v) >> #endif >> >> /* Prevent interleaving of output. */ >> - flags = console_lock_recursive_irqsave(); >> + rspin_lock_irqsave(&console_lock, flags); >> >> vcpu_show_registers(v); >> >> @@ -708,7 +710,7 @@ void vcpu_show_execution_state(struct vcpu *v) >> * Stop interleaving prevention: The necessary P2M lookups involve >> * locking, which has to occur with IRQs enabled. >> */ >> - console_unlock_recursive_irqrestore(flags); >> + rspin_unlock_irqrestore(&console_lock, flags); >> >> show_hvm_stack(v, &v->arch.user_regs); >> } >> @@ -717,7 +719,7 @@ void vcpu_show_execution_state(struct vcpu *v) >> if ( guest_kernel_mode(v, &v->arch.user_regs) ) >> show_guest_stack(v, &v->arch.user_regs); >> >> - console_unlock_recursive_irqrestore(flags); >> + rspin_unlock_irqrestore(&console_lock, flags); >> } >> > > I view these as layering violations; ... > >> --- a/xen/drivers/char/console.c >> +++ b/xen/drivers/char/console.c >> @@ -120,7 +120,7 @@ static int __read_mostly sercon_handle = -1; >> int8_t __read_mostly opt_console_xen; /* console=xen */ >> #endif >> >> -static DEFINE_RSPINLOCK(console_lock); >> +DEFINE_RSPINLOCK(console_lock); > > ... this should remain static. The question therefore just is whether > to omit this patch or ... > >> @@ -1158,22 +1158,6 @@ void console_end_log_everything(void) >> atomic_dec(&print_everything); >> } >> >> -unsigned long console_lock_recursive_irqsave(void) >> -{ >> - unsigned long flags; >> - >> - local_irq_save(flags); >> - rspin_lock(&console_lock); >> - >> - return flags; >> -} >> - >> -void console_unlock_recursive_irqrestore(unsigned long flags) >> -{ >> - rspin_unlock(&console_lock); >> - local_irq_restore(flags); >> -} > > ... whether to retain these two functions as thin wrappers around the > new, more generic construct. I'd vote for the latter. Juergen
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index 7724306116..21227877b3 100644 --- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -647,13 +647,15 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs) void show_execution_state(const struct cpu_user_regs *regs) { /* Prevent interleaving of output. */ - unsigned long flags = console_lock_recursive_irqsave(); + unsigned long flags; + + rspin_lock_irqsave(&console_lock, flags); show_registers(regs); show_code(regs); show_stack(regs); - console_unlock_recursive_irqrestore(flags); + rspin_unlock_irqrestore(&console_lock, flags); } void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs) @@ -663,7 +665,7 @@ void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs) void vcpu_show_execution_state(struct vcpu *v) { - unsigned long flags = 0; + unsigned long flags; if ( test_bit(_VPF_down, &v->pause_flags) ) { @@ -698,7 +700,7 @@ void vcpu_show_execution_state(struct vcpu *v) #endif /* Prevent interleaving of output. */ - flags = console_lock_recursive_irqsave(); + rspin_lock_irqsave(&console_lock, flags); vcpu_show_registers(v); @@ -708,7 +710,7 @@ void vcpu_show_execution_state(struct vcpu *v) * Stop interleaving prevention: The necessary P2M lookups involve * locking, which has to occur with IRQs enabled. */ - console_unlock_recursive_irqrestore(flags); + rspin_unlock_irqrestore(&console_lock, flags); show_hvm_stack(v, &v->arch.user_regs); } @@ -717,7 +719,7 @@ void vcpu_show_execution_state(struct vcpu *v) if ( guest_kernel_mode(v, &v->arch.user_regs) ) show_guest_stack(v, &v->arch.user_regs); - console_unlock_recursive_irqrestore(flags); + rspin_unlock_irqrestore(&console_lock, flags); } #ifdef CONFIG_HVM diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index 422a7fb1db..c1a9ba1304 100644 --- a/xen/common/spinlock.c +++ b/xen/common/spinlock.c @@ -475,6 +475,16 @@ void rspin_lock(rspinlock_t *lock) lock->recurse_cnt++; } +unsigned long __rspin_lock_irqsave(rspinlock_t *lock) +{ + unsigned long flags; + + local_irq_save(flags); + rspin_lock(lock); + + return flags; +} + void rspin_unlock(rspinlock_t *lock) { if ( likely(--lock->recurse_cnt == 0) ) @@ -484,6 +494,12 @@ void rspin_unlock(rspinlock_t *lock) } } +void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags) +{ + rspin_unlock(lock); + local_irq_restore(flags); +} + #ifdef CONFIG_DEBUG_LOCK_PROFILE struct lock_profile_anc { diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index f6f61dc5a1..1db2bbdb6a 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -120,7 +120,7 @@ static int __read_mostly sercon_handle = -1; int8_t __read_mostly opt_console_xen; /* console=xen */ #endif -static DEFINE_RSPINLOCK(console_lock); +DEFINE_RSPINLOCK(console_lock); /* * To control the amount of printing, thresholds are added. @@ -1158,22 +1158,6 @@ void console_end_log_everything(void) atomic_dec(&print_everything); } -unsigned long console_lock_recursive_irqsave(void) -{ - unsigned long flags; - - local_irq_save(flags); - rspin_lock(&console_lock); - - return flags; -} - -void console_unlock_recursive_irqrestore(unsigned long flags) -{ - rspin_unlock(&console_lock); - local_irq_restore(flags); -} - void console_force_unlock(void) { watchdog_disable(); diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h index 68759862e8..583c38f064 100644 --- a/xen/include/xen/console.h +++ b/xen/include/xen/console.h @@ -8,8 +8,11 @@ #define __CONSOLE_H__ #include <xen/inttypes.h> +#include <xen/spinlock.h> #include <public/xen.h> +extern rspinlock_t console_lock; + struct xen_sysctl_readconsole; long read_console_ring(struct xen_sysctl_readconsole *op); @@ -20,8 +23,6 @@ void console_init_postirq(void); void console_endboot(void); int console_has(const char *device); -unsigned long console_lock_recursive_irqsave(void); -void console_unlock_recursive_irqrestore(unsigned long flags); void console_force_unlock(void); void console_start_sync(void); diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h index ee536c302c..05b97c1e03 100644 --- a/xen/include/xen/spinlock.h +++ b/xen/include/xen/spinlock.h @@ -218,7 +218,14 @@ void _spin_barrier(spinlock_t *lock); */ int rspin_trylock(rspinlock_t *lock); void rspin_lock(rspinlock_t *lock); +#define rspin_lock_irqsave(l, f) \ + ({ \ + BUILD_BUG_ON(sizeof(f) != sizeof(unsigned long)); \ + ((f) = __rspin_lock_irqsave(l)); \ + }) +unsigned long __rspin_lock_irqsave(rspinlock_t *lock); void rspin_unlock(rspinlock_t *lock); +void rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags); #define spin_lock(l) _spin_lock(l) #define spin_lock_cb(l, c, d) _spin_lock_cb(l, c, d)
Instead of special casing rspin_lock_irqsave() and rspin_unlock_irqrestore() for the console lock, add those functions to spinlock handling and use them where needed. Signed-off-by: Juergen Gross <jgross@suse.com> --- V2: - new patch --- xen/arch/x86/traps.c | 14 ++++++++------ xen/common/spinlock.c | 16 ++++++++++++++++ xen/drivers/char/console.c | 18 +----------------- xen/include/xen/console.h | 5 +++-- xen/include/xen/spinlock.h | 7 +++++++ 5 files changed, 35 insertions(+), 25 deletions(-)