| Message ID | 20240314072029.16937-5-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/spinlock: make recursive spinlocks a dedicated type | expand |
On 14.03.2024 08:20, 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> Reviewed-by: Jan Beulich <jbeulich@suse.com> with two remarks: > --- a/xen/common/spinlock.c > +++ b/xen/common/spinlock.c > @@ -475,15 +475,31 @@ 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) ) > { > lock->recurse_cpu = SPINLOCK_NO_CPU; > - spin_unlock(lock); > + _spin_unlock(lock); This looks like an unrelated change. I think I can guess the purpose, but it would be nice if such along-the-way changes could be mentioned in the description. > --- a/xen/include/xen/spinlock.h > +++ b/xen/include/xen/spinlock.h > @@ -272,7 +272,15 @@ static always_inline void spin_lock_if(bool condition, spinlock_t *l) > */ > bool _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)); \ Perhaps in the context of another patch in the series I think I had pointed out that the outer pair of parentheses is unnecessary in constructs like this. I'd be fine adjusting this while committing, as long as for the other item above you'd provide some text to add. Jan
On 18.03.24 15:43, Jan Beulich wrote: > On 14.03.2024 08:20, 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> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > with two remarks: > >> --- a/xen/common/spinlock.c >> +++ b/xen/common/spinlock.c >> @@ -475,15 +475,31 @@ 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) ) >> { >> lock->recurse_cpu = SPINLOCK_NO_CPU; >> - spin_unlock(lock); >> + _spin_unlock(lock); > > This looks like an unrelated change. I think I can guess the purpose, but > it would be nice if such along-the-way changes could be mentioned in the > description. I think it would be better to move that change to patch 3. > >> --- a/xen/include/xen/spinlock.h >> +++ b/xen/include/xen/spinlock.h >> @@ -272,7 +272,15 @@ static always_inline void spin_lock_if(bool condition, spinlock_t *l) >> */ >> bool _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)); \ > > Perhaps in the context of another patch in the series I think I had > pointed out that the outer pair of parentheses is unnecessary in > constructs like this. Oh, this one slipped through, sorry for that. Will fix it in the next iteration. Juergen
On 18.03.2024 16:55, Jürgen Groß wrote: > On 18.03.24 15:43, Jan Beulich wrote: >> On 14.03.2024 08:20, 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> >> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> with two remarks: >> >>> --- a/xen/common/spinlock.c >>> +++ b/xen/common/spinlock.c >>> @@ -475,15 +475,31 @@ 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) ) >>> { >>> lock->recurse_cpu = SPINLOCK_NO_CPU; >>> - spin_unlock(lock); >>> + _spin_unlock(lock); >> >> This looks like an unrelated change. I think I can guess the purpose, but >> it would be nice if such along-the-way changes could be mentioned in the >> description. > > I think it would be better to move that change to patch 3. Hmm, it would be a secondary change there, too. I was actually meaning to commit patches 2-5, but if things want moving around I guess I better wait with doing so? Jan
On 18.03.24 16:59, Jan Beulich wrote: > On 18.03.2024 16:55, Jürgen Groß wrote: >> On 18.03.24 15:43, Jan Beulich wrote: >>> On 14.03.2024 08:20, 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> >>> >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> with two remarks: >>> >>>> --- a/xen/common/spinlock.c >>>> +++ b/xen/common/spinlock.c >>>> @@ -475,15 +475,31 @@ 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) ) >>>> { >>>> lock->recurse_cpu = SPINLOCK_NO_CPU; >>>> - spin_unlock(lock); >>>> + _spin_unlock(lock); >>> >>> This looks like an unrelated change. I think I can guess the purpose, but >>> it would be nice if such along-the-way changes could be mentioned in the >>> description. >> >> I think it would be better to move that change to patch 3. > > Hmm, it would be a secondary change there, too. I was actually meaning to > commit patches 2-5, but if things want moving around I guess I better > wait with doing so? Hmm, maybe just drop this hunk and let patch 7 handle it? Juergen
On 18.03.2024 17:05, Jürgen Groß wrote: > On 18.03.24 16:59, Jan Beulich wrote: >> On 18.03.2024 16:55, Jürgen Groß wrote: >>> On 18.03.24 15:43, Jan Beulich wrote: >>>> On 14.03.2024 08:20, 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> >>>> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>> with two remarks: >>>> >>>>> --- a/xen/common/spinlock.c >>>>> +++ b/xen/common/spinlock.c >>>>> @@ -475,15 +475,31 @@ 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) ) >>>>> { >>>>> lock->recurse_cpu = SPINLOCK_NO_CPU; >>>>> - spin_unlock(lock); >>>>> + _spin_unlock(lock); >>>> >>>> This looks like an unrelated change. I think I can guess the purpose, but >>>> it would be nice if such along-the-way changes could be mentioned in the >>>> description. >>> >>> I think it would be better to move that change to patch 3. >> >> Hmm, it would be a secondary change there, too. I was actually meaning to >> commit patches 2-5, but if things want moving around I guess I better >> wait with doing so? > > Hmm, maybe just drop this hunk and let patch 7 handle it? Ah yes, that seem more logical to me. I take it you don't mean "hunk" though, but really just this one line change. Jan
On 18.03.24 17:08, Jan Beulich wrote: > On 18.03.2024 17:05, Jürgen Groß wrote: >> On 18.03.24 16:59, Jan Beulich wrote: >>> On 18.03.2024 16:55, Jürgen Groß wrote: >>>> On 18.03.24 15:43, Jan Beulich wrote: >>>>> On 14.03.2024 08:20, 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> >>>>> >>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>>> with two remarks: >>>>> >>>>>> --- a/xen/common/spinlock.c >>>>>> +++ b/xen/common/spinlock.c >>>>>> @@ -475,15 +475,31 @@ 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) ) >>>>>> { >>>>>> lock->recurse_cpu = SPINLOCK_NO_CPU; >>>>>> - spin_unlock(lock); >>>>>> + _spin_unlock(lock); >>>>> >>>>> This looks like an unrelated change. I think I can guess the purpose, but >>>>> it would be nice if such along-the-way changes could be mentioned in the >>>>> description. >>>> >>>> I think it would be better to move that change to patch 3. >>> >>> Hmm, it would be a secondary change there, too. I was actually meaning to >>> commit patches 2-5, but if things want moving around I guess I better >>> wait with doing so? >> >> Hmm, maybe just drop this hunk and let patch 7 handle it? > > Ah yes, that seem more logical to me. I take it you don't mean "hunk" > though, but really just this one line change. Oh yes, of course. Juergen
diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c index 11e13e1259..5ef0ac7f89 100644 --- a/xen/common/spinlock.c +++ b/xen/common/spinlock.c @@ -475,15 +475,31 @@ 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) ) { lock->recurse_cpu = SPINLOCK_NO_CPU; - spin_unlock(lock); + _spin_unlock(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 eca17b55b4..ccd5f8cc14 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -1161,16 +1161,14 @@ unsigned long console_lock_recursive_irqsave(void) { unsigned long flags; - local_irq_save(flags); - rspin_lock(&console_lock); + rspin_lock_irqsave(&console_lock, flags); return flags; } void console_unlock_recursive_irqrestore(unsigned long flags) { - rspin_unlock(&console_lock); - local_irq_restore(flags); + rspin_unlock_irqrestore(&console_lock, flags); } void console_force_unlock(void) diff --git a/xen/include/xen/spinlock.h b/xen/include/xen/spinlock.h index 50f6580f52..afa24c8e29 100644 --- a/xen/include/xen/spinlock.h +++ b/xen/include/xen/spinlock.h @@ -272,7 +272,15 @@ static always_inline void spin_lock_if(bool condition, spinlock_t *l) */ bool _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)); \ + block_lock_speculation(); \ + }) +unsigned long _rspin_lock_irqsave(rspinlock_t *lock); void _rspin_unlock(rspinlock_t *lock); +void _rspin_unlock_irqrestore(rspinlock_t *lock, unsigned long flags); static always_inline void rspin_lock(rspinlock_t *lock) { @@ -282,5 +290,6 @@ static always_inline void rspin_lock(rspinlock_t *lock) #define rspin_trylock(l) lock_evaluate_nospec(_rspin_trylock(l)) #define rspin_unlock(l) _rspin_unlock(l) +#define rspin_unlock_irqrestore(l, f) _rspin_unlock_irqrestore(l, f) #endif /* __SPINLOCK_H__ */
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 V5: - avoid MISRA violation (Julien Grall) - keep wrapper functions (Jan Beulich) --- xen/common/spinlock.c | 18 +++++++++++++++++- xen/drivers/char/console.c | 6 ++---- xen/include/xen/spinlock.h | 9 +++++++++ 3 files changed, 28 insertions(+), 5 deletions(-)