| Message ID | 20231120113842.5897-7-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/spinlock: make recursive spinlocks a dedicated type | expand |
On 20/11/2023 11:38, 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 e1356f696a..f72769e79b 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); This feels a bit weird because rspin_lock_irqsave() being lowercase hints that it's a either a function or behaves like one. For that it should take &flags instead. > > 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 26c667d3cc..17716fc4eb 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 369b2f9077..cc743b67ec 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 ab5c30c0da..dff0096b27 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 c99ee52458..53f0f72ac4 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)); \ > + }) If f is &flags, then s/f/*(f)/ would be needed in these 2 cases. On other matters if we had -Wconversion turned on by default that BUILD_BUG_ON() wouldn't be needed. Not that you can do it (I'm sure the codebase would explode everywhere if we switched it on), but might be something to consider in the future. > +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) Cheers, Alejandro
On 24.11.23 20:23, Alejandro Vallejo wrote: > On 20/11/2023 11:38, 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 e1356f696a..f72769e79b 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); > > This feels a bit weird because rspin_lock_irqsave() being lowercase > hints that it's a either a function or behaves like one. For that it > should take &flags instead. I don't think so. This is common behavior with the Linux kernel, and I think we should keep it. Especially as I believe rspin_lock_irqsave() and spin_lock_irqsave() should have a similar interface. Or would you want us to change more than 250 call sites of spin_lock_irqsave()? > >> 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 26c667d3cc..17716fc4eb 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 369b2f9077..cc743b67ec 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 ab5c30c0da..dff0096b27 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 c99ee52458..53f0f72ac4 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)); \ >> + }) > > If f is &flags, then s/f/*(f)/ would be needed in these 2 cases. > > On other matters if we had -Wconversion turned on by default that > BUILD_BUG_ON() wouldn't be needed. Not that you can do it (I'm sure the codebase > would explode everywhere if we switched it on), but might be > something to consider in the future. Again, this is like Linux kernel. And I don't think we want explicit casts everywhere when e.g. moving values between fixed sized fields and standard integer typed fields. Juergen
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c index e1356f696a..f72769e79b 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 26c667d3cc..17716fc4eb 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 369b2f9077..cc743b67ec 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 ab5c30c0da..dff0096b27 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 c99ee52458..53f0f72ac4 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(-)