* [PATCH] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node()
@ 2024-07-21 16:45 Uros Bizjak
2024-07-21 20:45 ` Waiman Long
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Uros Bizjak @ 2024-07-21 16:45 UTC (permalink / raw)
To: linux-kernel
Cc: Uros Bizjak, Peter Zijlstra, Ingo Molnar, Will Deacon,
Waiman Long, Boqun Feng, Bibo Mao
"enum vcpu_state" is not compatible with "u8" type for all targets,
resulting in:
error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *'
for LoongArch. Correct the type of "old" variable to "u8".
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Will Deacon <will@kernel.org>
Cc: Waiman Long <longman@redhat.com>
Cc: Boqun Feng <boqun.feng@gmail.com>
Fixes: fea0e1820b51 ("locking/pvqspinlock: Use try_cmpxchg() in qspinlock_paravirt.h")
Reported-by: Bibo Mao <maobibo@loongson.cn>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Closes: https://lore.kernel.org/lkml/20240719024010.3296488-1-maobibo@loongson.cn/
---
kernel/locking/qspinlock_paravirt.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
index f5a36e67b593..ac2e22502741 100644
--- a/kernel/locking/qspinlock_paravirt.h
+++ b/kernel/locking/qspinlock_paravirt.h
@@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
{
struct pv_node *pn = (struct pv_node *)node;
- enum vcpu_state old = vcpu_halted;
+ u8 old = vcpu_halted;
/*
* If the vCPU is indeed halted, advance its state to match that of
* pv_wait_node(). If OTOH this fails, the vCPU was running and will
--
2.42.0
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node()
2024-07-21 16:45 [PATCH] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node() Uros Bizjak
@ 2024-07-21 20:45 ` Waiman Long
2024-07-22 6:54 ` maobibo
2024-07-29 10:24 ` [tip: locking/urgent] " tip-bot2 for Uros Bizjak
2 siblings, 0 replies; 6+ messages in thread
From: Waiman Long @ 2024-07-21 20:45 UTC (permalink / raw)
To: Uros Bizjak, linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng, Bibo Mao
On 7/21/24 12:45, Uros Bizjak wrote:
> "enum vcpu_state" is not compatible with "u8" type for all targets,
> resulting in:
>
> error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *'
>
> for LoongArch. Correct the type of "old" variable to "u8".
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Fixes: fea0e1820b51 ("locking/pvqspinlock: Use try_cmpxchg() in qspinlock_paravirt.h")
> Reported-by: Bibo Mao <maobibo@loongson.cn>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Closes: https://lore.kernel.org/lkml/20240719024010.3296488-1-maobibo@loongson.cn/
> ---
> kernel/locking/qspinlock_paravirt.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
> index f5a36e67b593..ac2e22502741 100644
> --- a/kernel/locking/qspinlock_paravirt.h
> +++ b/kernel/locking/qspinlock_paravirt.h
> @@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
> static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
> {
> struct pv_node *pn = (struct pv_node *)node;
> - enum vcpu_state old = vcpu_halted;
> + u8 old = vcpu_halted;
> /*
> * If the vCPU is indeed halted, advance its state to match that of
> * pv_wait_node(). If OTOH this fails, the vCPU was running and will
Acked-by: Waiman Long <longman@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node()
2024-07-21 16:45 [PATCH] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node() Uros Bizjak
2024-07-21 20:45 ` Waiman Long
@ 2024-07-22 6:54 ` maobibo
2024-07-22 15:13 ` Waiman Long
2024-07-29 10:24 ` [tip: locking/urgent] " tip-bot2 for Uros Bizjak
2 siblings, 1 reply; 6+ messages in thread
From: maobibo @ 2024-07-22 6:54 UTC (permalink / raw)
To: Uros Bizjak, linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Waiman Long, Boqun Feng
Uros,
Sorry for late reply because of weekend time. This modification works
well for me.
In later I want to define pv_node::state as int type on LoongArch
because there is only int32/int64 cmpxchg is better supported on the
system, however that is another issue.
Tested-by:Bibo Mao <maobibo@loongson.cn>
On 2024/7/22 上午12:45, Uros Bizjak wrote:
> "enum vcpu_state" is not compatible with "u8" type for all targets,
> resulting in:
>
> error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *'
>
> for LoongArch. Correct the type of "old" variable to "u8".
>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: Waiman Long <longman@redhat.com>
> Cc: Boqun Feng <boqun.feng@gmail.com>
> Fixes: fea0e1820b51 ("locking/pvqspinlock: Use try_cmpxchg() in qspinlock_paravirt.h")
> Reported-by: Bibo Mao <maobibo@loongson.cn>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Closes: https://lore.kernel.org/lkml/20240719024010.3296488-1-maobibo@loongson.cn/
> ---
> kernel/locking/qspinlock_paravirt.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
> index f5a36e67b593..ac2e22502741 100644
> --- a/kernel/locking/qspinlock_paravirt.h
> +++ b/kernel/locking/qspinlock_paravirt.h
> @@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
> static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
> {
> struct pv_node *pn = (struct pv_node *)node;
> - enum vcpu_state old = vcpu_halted;
> + u8 old = vcpu_halted;
> /*
> * If the vCPU is indeed halted, advance its state to match that of
> * pv_wait_node(). If OTOH this fails, the vCPU was running and will
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node()
2024-07-22 6:54 ` maobibo
@ 2024-07-22 15:13 ` Waiman Long
2024-07-23 1:13 ` maobibo
0 siblings, 1 reply; 6+ messages in thread
From: Waiman Long @ 2024-07-22 15:13 UTC (permalink / raw)
To: maobibo, Uros Bizjak, linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng
On 7/22/24 02:54, maobibo wrote:
> Uros,
>
> Sorry for late reply because of weekend time. This modification works
> well for me.
>
> In later I want to define pv_node::state as int type on LoongArch
> because there is only int32/int64 cmpxchg is better supported on the
> system, however that is another issue.
I thought about changing pv_node::state to int, but that requires
changing the pv_wait() call to use int too. That doesn't work because
pv_wait() is also used to monitor the state of the lock byte of the
qspinlock. In essence, we can't use pvqspinlock if 8-bit cmpxchg isn't
supported.
Cheers,
Longman
>
> Tested-by:Bibo Mao <maobibo@loongson.cn>
>
> On 2024/7/22 上午12:45, Uros Bizjak wrote:
>> "enum vcpu_state" is not compatible with "u8" type for all targets,
>> resulting in:
>>
>> error: initialization of 'u8 *' {aka 'unsigned char *'} from
>> incompatible pointer type 'enum vcpu_state *'
>>
>> for LoongArch. Correct the type of "old" variable to "u8".
>>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Ingo Molnar <mingo@kernel.org>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Waiman Long <longman@redhat.com>
>> Cc: Boqun Feng <boqun.feng@gmail.com>
>> Fixes: fea0e1820b51 ("locking/pvqspinlock: Use try_cmpxchg() in
>> qspinlock_paravirt.h")
>> Reported-by: Bibo Mao <maobibo@loongson.cn>
>> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
>> Closes:
>> https://lore.kernel.org/lkml/20240719024010.3296488-1-maobibo@loongson.cn/
>> ---
>> kernel/locking/qspinlock_paravirt.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/locking/qspinlock_paravirt.h
>> b/kernel/locking/qspinlock_paravirt.h
>> index f5a36e67b593..ac2e22502741 100644
>> --- a/kernel/locking/qspinlock_paravirt.h
>> +++ b/kernel/locking/qspinlock_paravirt.h
>> @@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock
>> *node, struct mcs_spinlock *prev)
>> static void pv_kick_node(struct qspinlock *lock, struct
>> mcs_spinlock *node)
>> {
>> struct pv_node *pn = (struct pv_node *)node;
>> - enum vcpu_state old = vcpu_halted;
>> + u8 old = vcpu_halted;
>> /*
>> * If the vCPU is indeed halted, advance its state to match
>> that of
>> * pv_wait_node(). If OTOH this fails, the vCPU was running and
>> will
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node()
2024-07-22 15:13 ` Waiman Long
@ 2024-07-23 1:13 ` maobibo
0 siblings, 0 replies; 6+ messages in thread
From: maobibo @ 2024-07-23 1:13 UTC (permalink / raw)
To: Waiman Long, Uros Bizjak, linux-kernel
Cc: Peter Zijlstra, Ingo Molnar, Will Deacon, Boqun Feng
On 2024/7/22 下午11:13, Waiman Long wrote:
>
> On 7/22/24 02:54, maobibo wrote:
>> Uros,
>>
>> Sorry for late reply because of weekend time. This modification works
>> well for me.
>>
>> In later I want to define pv_node::state as int type on LoongArch
>> because there is only int32/int64 cmpxchg is better supported on the
>> system, however that is another issue.
>
> I thought about changing pv_node::state to int, but that requires
> changing the pv_wait() call to use int too. That doesn't work because
> pv_wait() is also used to monitor the state of the lock byte of the
> qspinlock. In essence, we can't use pvqspinlock if 8-bit cmpxchg isn't
> supported.
Hi Longman,
yes, your are right. pv_wait also monitors the state of the qspinlock
lock byte, so the parameter type about pv_wait must be u8.
LoongArch supports byte cmpxchg, only that word cmpxchg has higher
efficiency. By the draft kernel compiling test, pvqspinlock can improve
performance greatly when pCPU is shared by multiple vCPUs.
And thanks for your remaindering.
Regards
Bibo Mao
>
> Cheers,
> Longman
>
>>
>> Tested-by:Bibo Mao <maobibo@loongson.cn>
>>
>> On 2024/7/22 上午12:45, Uros Bizjak wrote:
>>> "enum vcpu_state" is not compatible with "u8" type for all targets,
>>> resulting in:
>>>
>>> error: initialization of 'u8 *' {aka 'unsigned char *'} from
>>> incompatible pointer type 'enum vcpu_state *'
>>>
>>> for LoongArch. Correct the type of "old" variable to "u8".
>>>
>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>> Cc: Ingo Molnar <mingo@kernel.org>
>>> Cc: Will Deacon <will@kernel.org>
>>> Cc: Waiman Long <longman@redhat.com>
>>> Cc: Boqun Feng <boqun.feng@gmail.com>
>>> Fixes: fea0e1820b51 ("locking/pvqspinlock: Use try_cmpxchg() in
>>> qspinlock_paravirt.h")
>>> Reported-by: Bibo Mao <maobibo@loongson.cn>
>>> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
>>> Closes:
>>> https://lore.kernel.org/lkml/20240719024010.3296488-1-maobibo@loongson.cn/
>>>
>>> ---
>>> kernel/locking/qspinlock_paravirt.h | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/locking/qspinlock_paravirt.h
>>> b/kernel/locking/qspinlock_paravirt.h
>>> index f5a36e67b593..ac2e22502741 100644
>>> --- a/kernel/locking/qspinlock_paravirt.h
>>> +++ b/kernel/locking/qspinlock_paravirt.h
>>> @@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock
>>> *node, struct mcs_spinlock *prev)
>>> static void pv_kick_node(struct qspinlock *lock, struct
>>> mcs_spinlock *node)
>>> {
>>> struct pv_node *pn = (struct pv_node *)node;
>>> - enum vcpu_state old = vcpu_halted;
>>> + u8 old = vcpu_halted;
>>> /*
>>> * If the vCPU is indeed halted, advance its state to match
>>> that of
>>> * pv_wait_node(). If OTOH this fails, the vCPU was running and
>>> will
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip: locking/urgent] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node()
2024-07-21 16:45 [PATCH] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node() Uros Bizjak
2024-07-21 20:45 ` Waiman Long
2024-07-22 6:54 ` maobibo
@ 2024-07-29 10:24 ` tip-bot2 for Uros Bizjak
2 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Uros Bizjak @ 2024-07-29 10:24 UTC (permalink / raw)
To: linux-tip-commits
Cc: Bibo Mao, Uros Bizjak, Peter Zijlstra (Intel),
Waiman Long, x86, linux-kernel
The following commit has been merged into the locking/urgent branch of tip:
Commit-ID: 6623b0217d0c9bed80bfa43b778ce1c0eb03b497
Gitweb: https://git.kernel.org/tip/6623b0217d0c9bed80bfa43b778ce1c0eb03b497
Author: Uros Bizjak <ubizjak@gmail.com>
AuthorDate: Sun, 21 Jul 2024 18:45:41 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 29 Jul 2024 12:16:21 +02:00
locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node()
"enum vcpu_state" is not compatible with "u8" type for all targets,
resulting in:
error: initialization of 'u8 *' {aka 'unsigned char *'} from incompatible pointer type 'enum vcpu_state *'
for LoongArch. Correct the type of "old" variable to "u8".
Fixes: fea0e1820b51 ("locking/pvqspinlock: Use try_cmpxchg() in qspinlock_paravirt.h")
Closes: https://lore.kernel.org/lkml/20240719024010.3296488-1-maobibo@loongson.cn/
Reported-by: Bibo Mao <maobibo@loongson.cn>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Waiman Long <longman@redhat.com>
Link: https://lore.kernel.org/r/20240721164552.50175-1-ubizjak@gmail.com
---
kernel/locking/qspinlock_paravirt.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/locking/qspinlock_paravirt.h b/kernel/locking/qspinlock_paravirt.h
index f5a36e6..ac2e225 100644
--- a/kernel/locking/qspinlock_paravirt.h
+++ b/kernel/locking/qspinlock_paravirt.h
@@ -357,7 +357,7 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev)
static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)
{
struct pv_node *pn = (struct pv_node *)node;
- enum vcpu_state old = vcpu_halted;
+ u8 old = vcpu_halted;
/*
* If the vCPU is indeed halted, advance its state to match that of
* pv_wait_node(). If OTOH this fails, the vCPU was running and will
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-07-29 10:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-21 16:45 [PATCH] locking/pvqspinlock: Correct the type of "old" variable in pv_kick_node() Uros Bizjak
2024-07-21 20:45 ` Waiman Long
2024-07-22 6:54 ` maobibo
2024-07-22 15:13 ` Waiman Long
2024-07-23 1:13 ` maobibo
2024-07-29 10:24 ` [tip: locking/urgent] " tip-bot2 for Uros Bizjak
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®