Advanced
Mailing List Archive

Mailing List Archive

  1. Home >
  2. Xen>
  3. Devel
[PATCH v5 04/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]()
jgross at suse
Mar 14, 2024, 12:20 AM
Post #1 of 7 (13 views)
Permalink
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(-)

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__ */
--
2.35.3
Re: [PATCH v5 04/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]() [ In reply to ]
jbeulich at suse
Mar 18, 2024, 7:43 AM
Post #2 of 7 (9 views)
Permalink
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
Re: [PATCH v5 04/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]() [ In reply to ]
jgross at suse
Mar 18, 2024, 8:55 AM
Post #3 of 7 (9 views)
Permalink
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
Re: [PATCH v5 04/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]() [ In reply to ]
jbeulich at suse
Mar 18, 2024, 8:59 AM
Post #4 of 7 (9 views)
Permalink
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
Re: [PATCH v5 04/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]() [ In reply to ]
jgross at suse
Mar 18, 2024, 9:05 AM
Post #5 of 7 (9 views)
Permalink
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
Re: [PATCH v5 04/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]() [ In reply to ]
jbeulich at suse
Mar 18, 2024, 9:08 AM
Post #6 of 7 (9 views)
Permalink
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
Re: [PATCH v5 04/13] xen/spinlock: add rspin_[un]lock_irq[save|restore]() [ In reply to ]
jgross at suse
Mar 18, 2024, 9:09 AM
Post #7 of 7 (9 views)
Permalink
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

©2024 GT.net. A Gossamer Threads company.
5th Floor, 455 Granville St., Vancouver, BC V6C 1T1, Canada | Legal