Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1432046 > unrolled thread

[PATCH 0/3] implement vcpu preempted check

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] implement vcpu preempted check Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-27 15:50 +0200
    [PATCH 2/3] powerpc/spinlock: support vcpu preempted check Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-27 15:50 +0200
      Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check Peter Zijlstra <peterz@infradead.org> - 2016-06-27 16:20 +0200
        Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-28 05:30 +0200
      Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check Boqun Feng <boqun.feng@gmail.com> - 2016-06-27 17:00 +0200
        Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-28 05:50 +0200
          Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check Boqun Feng <boqun.feng@gmail.com> - 2016-06-28 07:10 +0200
            Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-28 07:40 +0200
    [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-27 15:50 +0200
      Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check  interface Peter Zijlstra <peterz@infradead.org> - 2016-06-27 16:10 +0200
        Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check  interface Peter Zijlstra <peterz@infradead.org> - 2016-06-27 16:10 +0200
          Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-28 05:20 +0200
        Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check  interface Heiko Carstens <heiko.carstens@de.ibm.com> - 2016-06-28 09:10 +0200
          Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-28 11:50 +0200
      Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check  interface Boqun Feng <boqun.feng@gmail.com> - 2016-06-27 16:10 +0200
        Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-28 05:20 +0200

#1432046 — [PATCH 0/3] implement vcpu preempted check

This is to fix some bad issues on an over-committed guest.

test-caes:
perf record -a perf bench sched messaging -g 400 -p && perf report

18.09%  sched-messaging  [kernel.vmlinux]  [k] osq_lock
12.28%  sched-messaging  [kernel.vmlinux]  [k] rwsem_spin_on_owner
 5.27%  sched-messaging  [kernel.vmlinux]  [k] mutex_unlock
 3.89%  sched-messaging  [kernel.vmlinux]  [k] wait_consider_task
 3.64%  sched-messaging  [kernel.vmlinux]  [k] _raw_write_lock_irq
 3.41%  sched-messaging  [kernel.vmlinux]  [k] mutex_spin_on_owner.is
 2.49%  sched-messaging  [kernel.vmlinux]  [k] system_call

osq takes a long time with preemption disabled which is really bad.

This is because vCPU A hold the osq lock and yield out, vCPU B wait
per_cpu node->locked to be set. IOW, vCPU B wait vCPU A to run and
unlock the osq lock. Even there is need_resched(), it did not help on
such scenario.

we may also need fix other XXX_spin_on_owner later based on this patch set.
these spin_on_onwer variant cause rcu stall.

Pan Xinhui (3):
  powerpc/spinlock: support vcpu preempted check
  locking/osq: Drop the overload of osq_lock()
  kernel/sched: introduce vcpu preempted check interface

 arch/powerpc/include/asm/lppaca.h   |  6 ++++++
 arch/powerpc/include/asm/spinlock.h | 15 +++++++++++++++
 include/linux/sched.h               |  9 +++++++++
 kernel/locking/osq_lock.c           | 16 +++++++++++++++-
 4 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.4.11

[toc] | [next] | [standalone]

#1432047 — [PATCH 2/3] powerpc/spinlock: support vcpu preempted check

This is to fix some holder preemption issues. Spinning at one
vcpu which is preempted is meaningless.

Kernel need such interfaces, So lets support it.

We also should suooprt both the shared and dedicated mode.
So add lppaca_dedicated_proc macro in lppaca.h

Suggested-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/lppaca.h   |  6 ++++++
 arch/powerpc/include/asm/spinlock.h | 15 +++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/arch/powerpc/include/asm/lppaca.h b/arch/powerpc/include/asm/lppaca.h
index d0a2a2f..0a263d3 100644
--- a/arch/powerpc/include/asm/lppaca.h
+++ b/arch/powerpc/include/asm/lppaca.h
@@ -111,12 +111,18 @@ extern struct lppaca lppaca[];
  * we will have to transition to something better.
  */
 #define LPPACA_OLD_SHARED_PROC		2
+#define LPPACA_OLD_DEDICATED_PROC      (1 << 6)
 
 static inline bool lppaca_shared_proc(struct lppaca *l)
 {
 	return !!(l->__old_status & LPPACA_OLD_SHARED_PROC);
 }
 
+static inline bool lppaca_dedicated_proc(struct lppaca *l)
+{
+	return !!(l->__old_status & LPPACA_OLD_DEDICATED_PROC);
+}
+
 /*
  * SLB shadow buffer structure as defined in the PAPR.  The save_area
  * contains adjacent ESID and VSID pairs for each shadowed SLB.  The
diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
index 523673d..ae938ee 100644
--- a/arch/powerpc/include/asm/spinlock.h
+++ b/arch/powerpc/include/asm/spinlock.h
@@ -52,6 +52,21 @@
 #define SYNC_IO
 #endif
 
+/* For fixing some spinning issues in a guest.
+ * kernel would check if vcpu is preempted during a spin loop.
+ * we support that.
+ */
+#define arch_vcpu_is_preempted arch_vcpu_is_preempted
+static inline bool arch_vcpu_is_preempted(int cpu)
+{
+	struct lppaca *lp = &lppaca_of(cpu);
+
+	if (unlikely(!(lppaca_shared_proc(lp) ||
+			lppaca_dedicated_proc(lp))))
+		return false;
+	return !!(be32_to_cpu(lp->yield_count) & 1);
+}
+
 static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
 {
 	return lock.slock == 0;
-- 
2.4.11

[toc] | [prev] | [next] | [standalone]

#1432075 — Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check

On Mon, Jun 27, 2016 at 01:41:29PM -0400, Pan Xinhui wrote:
> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
> index 523673d..ae938ee 100644
> --- a/arch/powerpc/include/asm/spinlock.h
> +++ b/arch/powerpc/include/asm/spinlock.h
> @@ -52,6 +52,21 @@
>  #define SYNC_IO
>  #endif
>  
> +/* For fixing some spinning issues in a guest.
> + * kernel would check if vcpu is preempted during a spin loop.
> + * we support that.
> + */

If you look around in that file you'll notice that the above comment
style is inconsistent.

Nor is the comment really clarifying things, for one you fail to mention
the problem by its known name. You also forget to explain how this
interface will help. How about something like this:

/*
 * In order to deal with a various lock holder preemption issues provide
 * an interface to see if a vCPU is currently running or not.
 *
 * This allows us to terminate optimistic spin loops and block,
 * analogous to the native optimistic spin heuristic of testing if the
 * lock owner task is running or not.
 */

Also, since you now have a useful comment, which is not architecture
specific, I would place it with the common vcpu_is_preempted()
definition in sched.h.

Hmm?

> +#define arch_vcpu_is_preempted arch_vcpu_is_preempted
> +static inline bool arch_vcpu_is_preempted(int cpu)
> +{
> +	struct lppaca *lp = &lppaca_of(cpu);
> +
> +	if (unlikely(!(lppaca_shared_proc(lp) ||
> +			lppaca_dedicated_proc(lp))))
> +		return false;
> +	return !!(be32_to_cpu(lp->yield_count) & 1);
> +}
> +
>  static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
>  {
>  	return lock.slock == 0;
> -- 
> 2.4.11
> 

[toc] | [prev] | [next] | [standalone]

#1432434 — Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check

On 2016年06月27日 22:17, Peter Zijlstra wrote:
> On Mon, Jun 27, 2016 at 01:41:29PM -0400, Pan Xinhui wrote:
>> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
>> index 523673d..ae938ee 100644
>> --- a/arch/powerpc/include/asm/spinlock.h
>> +++ b/arch/powerpc/include/asm/spinlock.h
>> @@ -52,6 +52,21 @@
>>   #define SYNC_IO
>>   #endif
>>
>> +/* For fixing some spinning issues in a guest.
>> + * kernel would check if vcpu is preempted during a spin loop.
>> + * we support that.
>> + */
>
> If you look around in that file you'll notice that the above comment
> style is inconsistent.
>
> Nor is the comment really clarifying things, for one you fail to mention
> the problem by its known name. You also forget to explain how this
> interface will help. How about something like this:
>
> /*
>   * In order to deal with a various lock holder preemption issues provide
>   * an interface to see if a vCPU is currently running or not.
>   *
>   * This allows us to terminate optimistic spin loops and block,
>   * analogous to the native optimistic spin heuristic of testing if the
>   * lock owner task is running or not.
>   */
thanks!!!

>
> Also, since you now have a useful comment, which is not architecture
> specific, I would place it with the common vcpu_is_preempted()
> definition in sched.h.
>
agree with you. Will do that. I will also add Suggested-by with you.
thanks

> Hmm?
>
>> +#define arch_vcpu_is_preempted arch_vcpu_is_preempted
>> +static inline bool arch_vcpu_is_preempted(int cpu)
>> +{
>> +	struct lppaca *lp = &lppaca_of(cpu);
>> +
>> +	if (unlikely(!(lppaca_shared_proc(lp) ||
>> +			lppaca_dedicated_proc(lp))))
>> +		return false;
>> +	return !!(be32_to_cpu(lp->yield_count) & 1);
>> +}
>> +
>>   static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
>>   {
>>   	return lock.slock == 0;
>> --
>> 2.4.11
>>
>

[toc] | [prev] | [next] | [standalone]

#1432100 — Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check

[Multipart message — attachments visible in raw view] — view raw

Hi Xinhui,

On Mon, Jun 27, 2016 at 01:41:29PM -0400, Pan Xinhui wrote:
> This is to fix some holder preemption issues. Spinning at one
> vcpu which is preempted is meaningless.
> 
> Kernel need such interfaces, So lets support it.
> 
> We also should suooprt both the shared and dedicated mode.
> So add lppaca_dedicated_proc macro in lppaca.h
> 
> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
> ---
>  arch/powerpc/include/asm/lppaca.h   |  6 ++++++
>  arch/powerpc/include/asm/spinlock.h | 15 +++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/lppaca.h b/arch/powerpc/include/asm/lppaca.h
> index d0a2a2f..0a263d3 100644
> --- a/arch/powerpc/include/asm/lppaca.h
> +++ b/arch/powerpc/include/asm/lppaca.h
> @@ -111,12 +111,18 @@ extern struct lppaca lppaca[];
>   * we will have to transition to something better.
>   */
>  #define LPPACA_OLD_SHARED_PROC		2
> +#define LPPACA_OLD_DEDICATED_PROC      (1 << 6)
>  

I think you should describe a little bit about the magic number here,
i.e. what document/specification says this should work, and how this
works.

>  static inline bool lppaca_shared_proc(struct lppaca *l)
>  {
>  	return !!(l->__old_status & LPPACA_OLD_SHARED_PROC);
>  }
>  
> +static inline bool lppaca_dedicated_proc(struct lppaca *l)
> +{
> +	return !!(l->__old_status & LPPACA_OLD_DEDICATED_PROC);
> +}
> +
>  /*
>   * SLB shadow buffer structure as defined in the PAPR.  The save_area
>   * contains adjacent ESID and VSID pairs for each shadowed SLB.  The
> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
> index 523673d..ae938ee 100644
> --- a/arch/powerpc/include/asm/spinlock.h
> +++ b/arch/powerpc/include/asm/spinlock.h
> @@ -52,6 +52,21 @@
>  #define SYNC_IO
>  #endif
>  
> +/* For fixing some spinning issues in a guest.
> + * kernel would check if vcpu is preempted during a spin loop.
> + * we support that.
> + */
> +#define arch_vcpu_is_preempted arch_vcpu_is_preempted
> +static inline bool arch_vcpu_is_preempted(int cpu)

This function should be guarded by #ifdef PPC_PSERIES .. #endif, right?
Because if the kernel is not compiled with guest support,
vcpu_is_preempted() should always be false, right?

> +{
> +	struct lppaca *lp = &lppaca_of(cpu);
> +
> +	if (unlikely(!(lppaca_shared_proc(lp) ||
> +			lppaca_dedicated_proc(lp))))

Do you want to detect whether we are running in a guest(ie. pseries
kernel) here? Then I wonder whether "machine_is(pseries)" works here.

Regards,
Boqun

> +		return false;
> +	return !!(be32_to_cpu(lp->yield_count) & 1);
> +}
> +
>  static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
>  {
>  	return lock.slock == 0;
> -- 
> 2.4.11
> 

[toc] | [prev] | [next] | [standalone]

#1432440 — Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check

On 2016年06月27日 22:58, Boqun Feng wrote:
> Hi Xinhui,
>
> On Mon, Jun 27, 2016 at 01:41:29PM -0400, Pan Xinhui wrote:
>> This is to fix some holder preemption issues. Spinning at one
>> vcpu which is preempted is meaningless.
>>
>> Kernel need such interfaces, So lets support it.
>>
>> We also should suooprt both the shared and dedicated mode.
>> So add lppaca_dedicated_proc macro in lppaca.h
>>
>> Suggested-by: Boqun Feng <boqun.feng@gmail.com>
>> Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
>> ---
>>   arch/powerpc/include/asm/lppaca.h   |  6 ++++++
>>   arch/powerpc/include/asm/spinlock.h | 15 +++++++++++++++
>>   2 files changed, 21 insertions(+)
>>
>> diff --git a/arch/powerpc/include/asm/lppaca.h b/arch/powerpc/include/asm/lppaca.h
>> index d0a2a2f..0a263d3 100644
>> --- a/arch/powerpc/include/asm/lppaca.h
>> +++ b/arch/powerpc/include/asm/lppaca.h
>> @@ -111,12 +111,18 @@ extern struct lppaca lppaca[];
>>    * we will have to transition to something better.
>>    */
>>   #define LPPACA_OLD_SHARED_PROC		2
>> +#define LPPACA_OLD_DEDICATED_PROC      (1 << 6)
>>
>
> I think you should describe a little bit about the magic number here,
right.

> i.e. what document/specification says this should work, and how this
> works.
>
yep, I need add some comments here. for example, this bit is firmware reserved...
thanks, will do that.

>>   static inline bool lppaca_shared_proc(struct lppaca *l)
>>   {
>>   	return !!(l->__old_status & LPPACA_OLD_SHARED_PROC);
>>   }
>>
>> +static inline bool lppaca_dedicated_proc(struct lppaca *l)
>> +{
>> +	return !!(l->__old_status & LPPACA_OLD_DEDICATED_PROC);
>> +}
>> +
>>   /*
>>    * SLB shadow buffer structure as defined in the PAPR.  The save_area
>>    * contains adjacent ESID and VSID pairs for each shadowed SLB.  The
>> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
>> index 523673d..ae938ee 100644
>> --- a/arch/powerpc/include/asm/spinlock.h
>> +++ b/arch/powerpc/include/asm/spinlock.h
>> @@ -52,6 +52,21 @@
>>   #define SYNC_IO
>>   #endif
>>
>> +/* For fixing some spinning issues in a guest.
>> + * kernel would check if vcpu is preempted during a spin loop.
>> + * we support that.
>> + */
>> +#define arch_vcpu_is_preempted arch_vcpu_is_preempted
>> +static inline bool arch_vcpu_is_preempted(int cpu)
>
> This function should be guarded by #ifdef PPC_PSERIES .. #endif, right?
> Because if the kernel is not compiled with guest support,
> vcpu_is_preempted() should always be false, right?
>
oh, I forgot that. thanks for pointing it out.

>> +{
>> +	struct lppaca *lp = &lppaca_of(cpu);
>> +
>> +	if (unlikely(!(lppaca_shared_proc(lp) ||
>> +			lppaca_dedicated_proc(lp))))
>
> Do you want to detect whether we are running in a guest(ie. pseries
> kernel) here? Then I wonder whether "machine_is(pseries)" works here.
>
I tried as you said yesterday. but .h file has dependencies.
As you said, if we add #ifdef PPC_PSERIES, this is not a big problem. only powernv will be affected as they are built into same kernel img.

> Regards,
> Boqun
>
>> +		return false;
>> +	return !!(be32_to_cpu(lp->yield_count) & 1);
>> +}
>> +
>>   static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
>>   {
>>   	return lock.slock == 0;
>> --
>> 2.4.11
>>

[toc] | [prev] | [next] | [standalone]

#1432478 — Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check

[Multipart message — attachments visible in raw view] — view raw

On Tue, Jun 28, 2016 at 11:39:18AM +0800, xinhui wrote:
[snip]
> > > +{
> > > +	struct lppaca *lp = &lppaca_of(cpu);
> > > +
> > > +	if (unlikely(!(lppaca_shared_proc(lp) ||
> > > +			lppaca_dedicated_proc(lp))))
> > 
> > Do you want to detect whether we are running in a guest(ie. pseries
> > kernel) here? Then I wonder whether "machine_is(pseries)" works here.
> > 
> I tried as you said yesterday. but .h file has dependencies.
> As you said, if we add #ifdef PPC_PSERIES, this is not a big problem. only powernv will be affected as they are built into same kernel img.
> 

I never said this it not a big problem ;-)

The problem here is that we only need to detect the vcpu preemption in
a guest, and there could be several ways we can detect whether the
kernel is running in a guest. It's worthwhile to try find the best one
for this. Besides, it's really better that you can make sure we are
runing out of options before you introduce something like
lppaca_dedicated_proc().

I have a feeling that yield_count is non-zero only if we are running in
a guest, if so, we can use this and save several loads. But surely we
need the confirmation from ppc maintainers.

Regards,
Boqun

[toc] | [prev] | [next] | [standalone]

#1432489 — Re: [PATCH 2/3] powerpc/spinlock: support vcpu preempted check

On 2016年06月28日 13:03, Boqun Feng wrote:
> On Tue, Jun 28, 2016 at 11:39:18AM +0800, xinhui wrote:
> [snip]
>>>> +{
>>>> +	struct lppaca *lp = &lppaca_of(cpu);
>>>> +
>>>> +	if (unlikely(!(lppaca_shared_proc(lp) ||
>>>> +			lppaca_dedicated_proc(lp))))
>>>
>>> Do you want to detect whether we are running in a guest(ie. pseries
>>> kernel) here? Then I wonder whether "machine_is(pseries)" works here.
>>>
>> I tried as you said yesterday. but .h file has dependencies.
>> As you said, if we add #ifdef PPC_PSERIES, this is not a big problem. only powernv will be affected as they are built into same kernel img.
>>
>
> I never said this it not a big problem ;-)
>
> The problem here is that we only need to detect the vcpu preemption in
> a guest, and there could be several ways we can detect whether the
> kernel is running in a guest. It's worthwhile to try find the best one
> for this. Besides, it's really better that you can make sure we are
> runing out of options before you introduce something like
> lppaca_dedicated_proc().
>
> I have a feeling that yield_count is non-zero only if we are running in
> a guest, if so, we can use this and save several loads. But surely we
> need the confirmation from ppc maintainers.
>
yes, on powernv, print the lppaca.yield_count and it is always zero. looks like only hypervisor and os can touch/modify it.


> Regards,
> Boqun
>

[toc] | [prev] | [next] | [standalone]

#1432048 — [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface

this supports to fix lock holder preempted issue which run as a guest

for kernel users, we could use bool vcpu_is_preempted(int cpu) to detech
if one vcpu is preempted or not.

The default implementation is a macrodefined by false. So compiler can
wrap it out if arch dose not support such vcpu pteempted check.

archs can implement it by define arch_vcpu_is_preempted().

Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
---
 include/linux/sched.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6e42ada..dc0a9c3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -3293,6 +3293,15 @@ static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
 
 #endif /* CONFIG_SMP */
 
+#ifdef arch_vcpu_is_preempted
+static inline bool vcpu_is_preempted(int cpu)
+{
+	return arch_vcpu_is_preempted(cpu);
+}
+#else
+#define vcpu_is_preempted(cpu)	false
+#endif
+
 extern long sched_setaffinity(pid_t pid, const struct cpumask *new_mask);
 extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
 
-- 
2.4.11

[toc] | [prev] | [next] | [standalone]

#1432061 — Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface

On Mon, Jun 27, 2016 at 01:41:28PM -0400, Pan Xinhui wrote:
> +++ b/include/linux/sched.h
> @@ -3293,6 +3293,15 @@ static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
>  
>  #endif /* CONFIG_SMP */
>  
> +#ifdef arch_vcpu_is_preempted
> +static inline bool vcpu_is_preempted(int cpu)
> +{
> +	return arch_vcpu_is_preempted(cpu);
> +}
> +#else
> +#define vcpu_is_preempted(cpu)	false
> +#endif

#ifndef vcpu_is_preempted
#define vcpu_is_preempted(cpu)		(false)
#endif

Is so much simpler...

Also, please Cc the virt list so that other interested parties can
comment, and maybe also the s390 folks.

[toc] | [prev] | [next] | [standalone]

#1432067 — Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface

On Mon, Jun 27, 2016 at 04:00:43PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 27, 2016 at 01:41:28PM -0400, Pan Xinhui wrote:
> > +++ b/include/linux/sched.h
> > @@ -3293,6 +3293,15 @@ static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
> >  
> >  #endif /* CONFIG_SMP */
> >  
> > +#ifdef arch_vcpu_is_preempted
> > +static inline bool vcpu_is_preempted(int cpu)
> > +{
> > +	return arch_vcpu_is_preempted(cpu);
> > +}
> > +#else
> > +#define vcpu_is_preempted(cpu)	false
> > +#endif
> 
> #ifndef vcpu_is_preempted
> #define vcpu_is_preempted(cpu)		(false)
> #endif
> 
> Is so much simpler...
> 
> Also, please Cc the virt list so that other interested parties can
> comment, and maybe also the s390 folks.

And before you hurry off to post again, add a patch doing
mutex_spin_on_owner() and rwsem_spin_in_owner().

[toc] | [prev] | [next] | [standalone]

#1432430 — Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface

On 2016年06月27日 22:02, Peter Zijlstra wrote:
> On Mon, Jun 27, 2016 at 04:00:43PM +0200, Peter Zijlstra wrote:
>> On Mon, Jun 27, 2016 at 01:41:28PM -0400, Pan Xinhui wrote:
>>> +++ b/include/linux/sched.h
>>> @@ -3293,6 +3293,15 @@ static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
>>>
>>>   #endif /* CONFIG_SMP */
>>>
>>> +#ifdef arch_vcpu_is_preempted
>>> +static inline bool vcpu_is_preempted(int cpu)
>>> +{
>>> +	return arch_vcpu_is_preempted(cpu);
>>> +}
>>> +#else
>>> +#define vcpu_is_preempted(cpu)	false
>>> +#endif
>>
>> #ifndef vcpu_is_preempted
>> #define vcpu_is_preempted(cpu)		(false)
>> #endif
>>
>> Is so much simpler...
>>
fair enough.

>> Also, please Cc the virt list so that other interested parties can
>> comment, and maybe also the s390 folks.
>
oh. I forgot that. maybe we need cc more.

root@ltcalpine2-lp13:~/linux# find ./arch -name kvm
./arch/arm/kvm
./arch/arm64/kvm
./arch/mips/kvm
./arch/powerpc/kvm
./arch/s390/kvm
./arch/tile/kvm
./arch/x86/kvm

> And before you hurry off to post again, add a patch doing
> mutex_spin_on_owner() and rwsem_spin_in_owner().
>
will do that.

thanks for your suggestion. :)

[toc] | [prev] | [next] | [standalone]

#1432559 — Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface

On Mon, Jun 27, 2016 at 04:00:43PM +0200, Peter Zijlstra wrote:
> On Mon, Jun 27, 2016 at 01:41:28PM -0400, Pan Xinhui wrote:
> > +++ b/include/linux/sched.h
> > @@ -3293,6 +3293,15 @@ static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
> >  
> >  #endif /* CONFIG_SMP */
> >  
> > +#ifdef arch_vcpu_is_preempted
> > +static inline bool vcpu_is_preempted(int cpu)
> > +{
> > +	return arch_vcpu_is_preempted(cpu);
> > +}
> > +#else
> > +#define vcpu_is_preempted(cpu)	false
> > +#endif
> 
> #ifndef vcpu_is_preempted
> #define vcpu_is_preempted(cpu)		(false)
> #endif
> 
> Is so much simpler...
> 
> Also, please Cc the virt list so that other interested parties can
> comment, and maybe also the s390 folks.

The s390 implementation would be to simply use cpu_is_preempted() from
arch/s390/lib/spinlock.c.
It's nice that there will be a common code function for this!

[toc] | [prev] | [next] | [standalone]

#1432695 — Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface

On 2016年06月28日 15:00, Heiko Carstens wrote:
> On Mon, Jun 27, 2016 at 04:00:43PM +0200, Peter Zijlstra wrote:
>> On Mon, Jun 27, 2016 at 01:41:28PM -0400, Pan Xinhui wrote:
>>> +++ b/include/linux/sched.h
>>> @@ -3293,6 +3293,15 @@ static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
>>>
>>>   #endif /* CONFIG_SMP */
>>>
>>> +#ifdef arch_vcpu_is_preempted
>>> +static inline bool vcpu_is_preempted(int cpu)
>>> +{
>>> +	return arch_vcpu_is_preempted(cpu);
>>> +}
>>> +#else
>>> +#define vcpu_is_preempted(cpu)	false
>>> +#endif
>>
>> #ifndef vcpu_is_preempted
>> #define vcpu_is_preempted(cpu)		(false)
>> #endif
>>
>> Is so much simpler...
>>
>> Also, please Cc the virt list so that other interested parties can
>> comment, and maybe also the s390 folks.
>
> The s390 implementation would be to simply use cpu_is_preempted() from
> arch/s390/lib/spinlock.c.
that's great.

> It's nice that there will be a common code function for this!
>

[toc] | [prev] | [next] | [standalone]

#1432063 — Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jun 27, 2016 at 01:41:28PM -0400, Pan Xinhui wrote:
> this supports to fix lock holder preempted issue which run as a guest
> 
> for kernel users, we could use bool vcpu_is_preempted(int cpu) to detech
> if one vcpu is preempted or not.
> 
> The default implementation is a macrodefined by false. So compiler can
> wrap it out if arch dose not support such vcpu pteempted check.
> 
> archs can implement it by define arch_vcpu_is_preempted().
> 
> Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
> ---
>  include/linux/sched.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 6e42ada..dc0a9c3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -3293,6 +3293,15 @@ static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
>  
>  #endif /* CONFIG_SMP */
>  
> +#ifdef arch_vcpu_is_preempted
> +static inline bool vcpu_is_preempted(int cpu)
> +{
> +	return arch_vcpu_is_preempted(cpu);
> +}
> +#else
> +#define vcpu_is_preempted(cpu)	false
> +#endif
> +

I think you are missing Peter's comment here. We can

#ifndef vcpu_is_preempted
#define vcpu_is_preempted(cpu)	fasle
#endif

And different archs implement their own versions of vcpu_is_preempted(),
IOW, no need for an arch_vcpu_is_preempted().

Regards,
Boqun

>  extern long sched_setaffinity(pid_t pid, const struct cpumask *new_mask);
>  extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
>  
> -- 
> 2.4.11
> 

[toc] | [prev] | [next] | [standalone]

#1432431 — Re: [PATCH 1/3] kernel/sched: introduce vcpu preempted check interface

On 2016年06月27日 22:05, Boqun Feng wrote:
> On Mon, Jun 27, 2016 at 01:41:28PM -0400, Pan Xinhui wrote:
>> this supports to fix lock holder preempted issue which run as a guest
>>
>> for kernel users, we could use bool vcpu_is_preempted(int cpu) to detech
>> if one vcpu is preempted or not.
>>
>> The default implementation is a macrodefined by false. So compiler can
>> wrap it out if arch dose not support such vcpu pteempted check.
>>
>> archs can implement it by define arch_vcpu_is_preempted().
>>
>> Signed-off-by: Pan Xinhui <xinhui.pan@linux.vnet.ibm.com>
>> ---
>>   include/linux/sched.h | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> index 6e42ada..dc0a9c3 100644
>> --- a/include/linux/sched.h
>> +++ b/include/linux/sched.h
>> @@ -3293,6 +3293,15 @@ static inline void set_task_cpu(struct task_struct *p, unsigned int cpu)
>>
>>   #endif /* CONFIG_SMP */
>>
>> +#ifdef arch_vcpu_is_preempted
>> +static inline bool vcpu_is_preempted(int cpu)
>> +{
>> +	return arch_vcpu_is_preempted(cpu);
>> +}
>> +#else
>> +#define vcpu_is_preempted(cpu)	false
>> +#endif
>> +
>
> I think you are missing Peter's comment here. We can
>
> #ifndef vcpu_is_preempted
> #define vcpu_is_preempted(cpu)	fasle
> #endif
>
> And different archs implement their own versions of vcpu_is_preempted(),
> IOW, no need for an arch_vcpu_is_preempted().
>
yes, right.
just vcpu_is_preempted, no arch_vcpu_is_preempted..
thanks

> Regards,
> Boqun
>
>>   extern long sched_setaffinity(pid_t pid, const struct cpumask *new_mask);
>>   extern long sched_getaffinity(pid_t pid, struct cpumask *mask);
>>
>> --
>> 2.4.11
>>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web