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

Groups > linux.kernel > #1431347 > unrolled thread

[PATCH 1/2] kernel/sched: introduce vcpu preempted interface

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] kernel/sched: introduce vcpu preempted interface Pan Xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-26 08:50 +0200
    Re: [PATCH 1/2] kernel/sched: introduce vcpu preempted interface Peter Zijlstra <peterz@infradead.org> - 2016-06-27 10:50 +0200
      Re: [PATCH 1/2] kernel/sched: introduce vcpu preempted interface xinhui <xinhui.pan@linux.vnet.ibm.com> - 2016-06-27 11:50 +0200

#1431347 — [PATCH 1/2] kernel/sched: introduce vcpu preempted interface

From: pan xinhui <xinhui.pan@linux.vnet.ibm.com>

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

two interfaces,
bool vcpu_is_preempted(int cpu);
unsigned int vcpu_get_yield_count(int cpu);
arch may need implement anyone of them.

some spinneris may also need call need_yield_to(int cpu, unsigned int
old_yield_count) to know if it need stop the spinning.

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

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6e42ada..9c565d2 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -3293,6 +3293,40 @@ 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
+static inline bool vcpu_is_preempted(int cpu)
+{
+	return 0;
+}
+#endif
+
+#ifdef arch_vcpu_get_yield_count
+static inline unsigned int vcpu_get_yield_count(int cpu)
+{
+	return arch_vcpu_get_yield_count(cpu);
+}
+#else
+static inline unsigned int vcpu_get_yield_count(int cpu)
+{
+	return 0;
+}
+#endif
+
+static inline bool
+need_yield_to(int vcpu, unsigned int old_yield_count)
+{
+	/* if we find the vcpu is preempted,
+	* then we may want to kick it, IOW, yield to it
+	*/
+	return vcpu_is_preempted(vcpu) ||
+		(vcpu_get_yield_count(vcpu) != old_yield_count);
+}
+
 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] | [next] | [standalone]

#1431823

On Sun, Jun 26, 2016 at 06:41:54AM -0400, Pan Xinhui wrote:

> +#ifdef arch_vcpu_is_preempted
> +static inline bool vcpu_is_preempted(int cpu)
> +{
> +	return arch_vcpu_is_preempted(cpu);
> +}
> +#else
> +static inline bool vcpu_is_preempted(int cpu)
> +{
> +	return 0;
> +}
> +#endif
> +
> +#ifdef arch_vcpu_get_yield_count
> +static inline unsigned int vcpu_get_yield_count(int cpu)
> +{
> +	return arch_vcpu_get_yield_count(cpu);
> +}
> +#else
> +static inline unsigned int vcpu_get_yield_count(int cpu)
> +{
> +	return 0;
> +}
> +#endif


Please, just do something like:

#ifndef vcpu_is_preempted
static inline bool vcpu_is_preempted(int cpu)
{
	return false;
}
#endif

No point in making it more complicated.

> +static inline bool
> +need_yield_to(int vcpu, unsigned int old_yield_count)

namespace... this thing should be called: vcpu_something()

> +{
> +	/* if we find the vcpu is preempted,
> +	* then we may want to kick it, IOW, yield to it
> +	*/
> +	return vcpu_is_preempted(vcpu) ||
> +		(vcpu_get_yield_count(vcpu) != old_yield_count);
> +}

And can you make doubly sure (and mention in the Changelog) that the OSQ
code compiles all this away when using these definitions.

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

#1431878

On 2016年06月27日 16:42, Peter Zijlstra wrote:
> On Sun, Jun 26, 2016 at 06:41:54AM -0400, Pan Xinhui wrote:
>
>> +#ifdef arch_vcpu_is_preempted
>> +static inline bool vcpu_is_preempted(int cpu)
>> +{
>> +	return arch_vcpu_is_preempted(cpu);
>> +}
>> +#else
>> +static inline bool vcpu_is_preempted(int cpu)
>> +{
>> +	return 0;
>> +}
>> +#endif
>> +
>> +#ifdef arch_vcpu_get_yield_count
>> +static inline unsigned int vcpu_get_yield_count(int cpu)
>> +{
>> +	return arch_vcpu_get_yield_count(cpu);
>> +}
>> +#else
>> +static inline unsigned int vcpu_get_yield_count(int cpu)
>> +{
>> +	return 0;
>> +}
>> +#endif
>
>
> Please, just do something like:
>
> #ifndef vcpu_is_preempted
> static inline bool vcpu_is_preempted(int cpu)
> {
> 	return false;
> }
> #endif
>
> No point in making it more complicated.
>
right, vcpu_is_preempted() is good enough to handle our osq issue.

>> +static inline bool
>> +need_yield_to(int vcpu, unsigned int old_yield_count)
>
> namespace... this thing should be called: vcpu_something()
>
>> +{
>> +	/* if we find the vcpu is preempted,
>> +	* then we may want to kick it, IOW, yield to it
>> +	*/
>> +	return vcpu_is_preempted(vcpu) ||
>> +		(vcpu_get_yield_count(vcpu) != old_yield_count);
>> +}
>
> And can you make doubly sure (and mention in the Changelog) that the OSQ
> code compiles all this away when using these definitions.
>
vimdiff shows the osq_lock.o has a little difference because osq read the yield_count and prev, even they are not used.

however, as you suggest above, IF I remove the vcpu_get_yield_count() and relevant code in osq_lock,  then the binary is same.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web