[RFC,03/11] powerpc: kvm: add interface to control kvm function on a core
diff mbox

Message ID 1413487800-7162-4-git-send-email-kernelfans@gmail.com
State RFC
Headers show

Commit Message

Pingfan Liu Oct. 16, 2014, 7:29 p.m. UTC
When kvm is enabled on a core, we migrate all external irq to primary
thread. Since currently, the kvmirq logic is handled by the primary
hwthread.

Todo: this patch lacks re-enable of irqbalance when kvm is disable on
the core

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/sysfs.c            | 39 ++++++++++++++++++++++++++++++++++
 arch/powerpc/sysdev/xics/xics-common.c | 12 +++++++++++
 2 files changed, 51 insertions(+)

Comments

Preeti U Murthy Oct. 27, 2014, 4:04 a.m. UTC | #1
Hi Liu,

On 10/17/2014 12:59 AM, kernelfans@gmail.com wrote:
> When kvm is enabled on a core, we migrate all external irq to primary
> thread. Since currently, the kvmirq logic is handled by the primary
> hwthread.
> 
> Todo: this patch lacks re-enable of irqbalance when kvm is disable on
> the core

Why is a sysfs file introduced to trigger irq migration? Why is it not
done during kvm module insert ? And similarly spread interrupts when the
module is removed? Isn't this a saner way ?
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/sysfs.c            | 39 ++++++++++++++++++++++++++++++++++
>  arch/powerpc/sysdev/xics/xics-common.c | 12 +++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index 67fd2fd..a2595dd 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -552,6 +552,45 @@ static void sysfs_create_dscr_default(void)
>  	if (cpu_has_feature(CPU_FTR_DSCR))
>  		err = device_create_file(cpu_subsys.dev_root, &dev_attr_dscr_default);
>  }
> +
> +#ifdef CONFIG_KVMPPC_ENABLE_SECONDARY
> +#define NR_CORES	(CONFIG_NR_CPUS/threads_per_core)
> +static DECLARE_BITMAP(kvm_on_core, NR_CORES) __read_mostly
> +
> +static ssize_t show_kvm_enable(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +}
> +
> +static ssize_t __used store_kvm_enable(struct device *dev,
> +		struct device_attribute *attr, const char *buf,
> +		size_t count)
> +{
> +	struct cpumask stop_cpus;
> +	unsigned long core, thr;
> +
> +	sscanf(buf, "%lx", &core);
> +	if (core > NR_CORES)
> +		return -1;
> +	if (!test_bit(core, &kvm_on_core))
> +		for (thr = 1; thr< threads_per_core; thr++)
> +			if (cpu_online(thr * threads_per_core + thr))
> +				cpumask_set_cpu(thr * threads_per_core + thr, &stop_cpus);

What is the above logic trying to do? Did you mean
cpu_online(threads_per_core * core + thr) ?

> +
> +	stop_machine(xics_migrate_irqs_away_secondary, NULL, &stop_cpus);
> +	set_bit(core, &kvm_on_core);
> +	return count;
> +}
> +
> +static DEVICE_ATTR(kvm_enable, 0600,
> +	show_kvm_enable, store_kvm_enable);
> +
> +static void sysfs_create_kvm_enable(void)
> +{
> +	device_create_file(cpu_subsys.dev_root, &dev_attr_kvm_enable);
> +}
> +#endif
> +
>  #endif /* CONFIG_PPC64 */
>  
>  #ifdef HAS_PPC_PMC_PA6T
> diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
> index fe0cca4..68b33d8 100644
> --- a/arch/powerpc/sysdev/xics/xics-common.c
> +++ b/arch/powerpc/sysdev/xics/xics-common.c
> @@ -258,6 +258,18 @@ unlock:
>  		raw_spin_unlock_irqrestore(&desc->lock, flags);
>  	}
>  }
> +
> +int xics_migrate_irqs_away_secondary(void *data)
> +{
> +	int cpu = smp_processor_id();
> +	if(cpu%thread_per_core != 0) {
> +		WARN(condition, format...);
> +		return 0;
> +	}
> +	/* In fact, if we can migrate the primary, it will be more fine */
> +	xics_migrate_irqs_away();

Isn't the aim of the patch to migrate irqs away from the secondary onto
the primary? But from above it looks like we are returning when we find
out that we are secondary threads, isn't it?

> +	return 0;
> +}
>  #endif /* CONFIG_HOTPLUG_CPU */

Note that xics_migrate_irqs_away() is defined under CONFIG_CPU_HOTPLUG.
But we will need this option on PowerKVM even when hotplug is not
configured in.

Regards
Preeti U Murthy
>  #ifdef CONFIG_SMP
>
Srikar Dronamraju Nov. 12, 2014, 1:01 p.m. UTC | #2
* kernelfans@gmail.com <kernelfans@gmail.com> [2014-10-16 15:29:52]:

> When kvm is enabled on a core, we migrate all external irq to primary
> thread. Since currently, the kvmirq logic is handled by the primary
> hwthread.
> 
> Todo: this patch lacks re-enable of irqbalance when kvm is disable on
> the core
> 
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/sysfs.c            | 39 ++++++++++++++++++++++++++++++++++
>  arch/powerpc/sysdev/xics/xics-common.c | 12 +++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
> index 67fd2fd..a2595dd 100644
> --- a/arch/powerpc/kernel/sysfs.c
> +++ b/arch/powerpc/kernel/sysfs.c
> @@ -552,6 +552,45 @@ static void sysfs_create_dscr_default(void)
>  	if (cpu_has_feature(CPU_FTR_DSCR))
>  		err = device_create_file(cpu_subsys.dev_root, &dev_attr_dscr_default);
>  }
> +
> +#ifdef CONFIG_KVMPPC_ENABLE_SECONDARY
> +#define NR_CORES	(CONFIG_NR_CPUS/threads_per_core)
> +static DECLARE_BITMAP(kvm_on_core, NR_CORES) __read_mostly
> +
> +static ssize_t show_kvm_enable(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +}
> +
> +static ssize_t __used store_kvm_enable(struct device *dev,
> +		struct device_attribute *attr, const char *buf,
> +		size_t count)
> +{
> +	struct cpumask stop_cpus;
> +	unsigned long core, thr;
> +
> +	sscanf(buf, "%lx", &core);
> +	if (core > NR_CORES)
> +		return -1;
> +	if (!test_bit(core, &kvm_on_core))
> +		for (thr = 1; thr< threads_per_core; thr++)
> +			if (cpu_online(thr * threads_per_core + thr))
> +				cpumask_set_cpu(thr * threads_per_core + thr, &stop_cpus);

Shouldnt this be 
			if (cpu_online(core * threads_per_core + thr))
				cpumask_set_cpu(core * threads_per_core + thr, &stop_cpus);
?
Pingfan Liu Nov. 18, 2014, 5:17 a.m. UTC | #3
On Mon, Oct 27, 2014 at 12:04 PM, Preeti U Murthy
<preeti@linux.vnet.ibm.com> wrote:
> Hi Liu,
>
> On 10/17/2014 12:59 AM, kernelfans@gmail.com wrote:
>> When kvm is enabled on a core, we migrate all external irq to primary
>> thread. Since currently, the kvmirq logic is handled by the primary
>> hwthread.
>>
>> Todo: this patch lacks re-enable of irqbalance when kvm is disable on
>> the core
>
> Why is a sysfs file introduced to trigger irq migration? Why is it not
> done during kvm module insert ? And similarly spread interrupts when the
> module is removed? Isn't this a saner way ?

Consider the scene: coreA and coreB, we want to enable KVM on coreA,
while keeping coreB unchanged.
In fact, I try to acheive something un-symmetric on the platform. Do
you think it is an justification?
>>
>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>> ---
>>  arch/powerpc/kernel/sysfs.c            | 39 ++++++++++++++++++++++++++++++++++
>>  arch/powerpc/sysdev/xics/xics-common.c | 12 +++++++++++
>>  2 files changed, 51 insertions(+)
>>
>> diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
>> index 67fd2fd..a2595dd 100644
>> --- a/arch/powerpc/kernel/sysfs.c
>> +++ b/arch/powerpc/kernel/sysfs.c
>> @@ -552,6 +552,45 @@ static void sysfs_create_dscr_default(void)
>>       if (cpu_has_feature(CPU_FTR_DSCR))
>>               err = device_create_file(cpu_subsys.dev_root, &dev_attr_dscr_default);
>>  }
>> +
>> +#ifdef CONFIG_KVMPPC_ENABLE_SECONDARY
>> +#define NR_CORES     (CONFIG_NR_CPUS/threads_per_core)
>> +static DECLARE_BITMAP(kvm_on_core, NR_CORES) __read_mostly
>> +
>> +static ssize_t show_kvm_enable(struct device *dev,
>> +             struct device_attribute *attr, char *buf)
>> +{
>> +}
>> +
>> +static ssize_t __used store_kvm_enable(struct device *dev,
>> +             struct device_attribute *attr, const char *buf,
>> +             size_t count)
>> +{
>> +     struct cpumask stop_cpus;
>> +     unsigned long core, thr;
>> +
>> +     sscanf(buf, "%lx", &core);
>> +     if (core > NR_CORES)
>> +             return -1;
>> +     if (!test_bit(core, &kvm_on_core))
>> +             for (thr = 1; thr< threads_per_core; thr++)
>> +                     if (cpu_online(thr * threads_per_core + thr))
>> +                             cpumask_set_cpu(thr * threads_per_core + thr, &stop_cpus);
>
> What is the above logic trying to do? Did you mean
> cpu_online(threads_per_core * core + thr) ?
>
Yeah. My mistake, should be cpumask_set_cpu(core * threads_per_core +
thr, &stop_cpus)

>> +
>> +     stop_machine(xics_migrate_irqs_away_secondary, NULL, &stop_cpus);
>> +     set_bit(core, &kvm_on_core);
>> +     return count;
>> +}
>> +
>> +static DEVICE_ATTR(kvm_enable, 0600,
>> +     show_kvm_enable, store_kvm_enable);
>> +
>> +static void sysfs_create_kvm_enable(void)
>> +{
>> +     device_create_file(cpu_subsys.dev_root, &dev_attr_kvm_enable);
>> +}
>> +#endif
>> +
>>  #endif /* CONFIG_PPC64 */
>>
>>  #ifdef HAS_PPC_PMC_PA6T
>> diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
>> index fe0cca4..68b33d8 100644
>> --- a/arch/powerpc/sysdev/xics/xics-common.c
>> +++ b/arch/powerpc/sysdev/xics/xics-common.c
>> @@ -258,6 +258,18 @@ unlock:
>>               raw_spin_unlock_irqrestore(&desc->lock, flags);
>>       }
>>  }
>> +
>> +int xics_migrate_irqs_away_secondary(void *data)
>> +{
>> +     int cpu = smp_processor_id();
>> +     if(cpu%thread_per_core != 0) {
>> +             WARN(condition, format...);
>> +             return 0;
>> +     }
>> +     /* In fact, if we can migrate the primary, it will be more fine */
>> +     ();
>
> Isn't the aim of the patch to migrate irqs away from the secondary onto
> the primary? But from above it looks like we are returning when we find
> out that we are secondary threads, isn't it?
>
Yes, will fix in next version.

>> +     return 0;
>> +}
>>  #endif /* CONFIG_HOTPLUG_CPU */
>
> Note that xics_migrate_irqs_away() is defined under CONFIG_CPU_HOTPLUG.
> But we will need this option on PowerKVM even when hotplug is not
> configured in.
>
Yes, will fix the dependency in next version

Thx,
Fan

> Regards
> Preeti U Murthy
>>  #ifdef CONFIG_SMP
>>
>

Patch
diff mbox

diff --git a/arch/powerpc/kernel/sysfs.c b/arch/powerpc/kernel/sysfs.c
index 67fd2fd..a2595dd 100644
--- a/arch/powerpc/kernel/sysfs.c
+++ b/arch/powerpc/kernel/sysfs.c
@@ -552,6 +552,45 @@  static void sysfs_create_dscr_default(void)
 	if (cpu_has_feature(CPU_FTR_DSCR))
 		err = device_create_file(cpu_subsys.dev_root, &dev_attr_dscr_default);
 }
+
+#ifdef CONFIG_KVMPPC_ENABLE_SECONDARY
+#define NR_CORES	(CONFIG_NR_CPUS/threads_per_core)
+static DECLARE_BITMAP(kvm_on_core, NR_CORES) __read_mostly
+
+static ssize_t show_kvm_enable(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+}
+
+static ssize_t __used store_kvm_enable(struct device *dev,
+		struct device_attribute *attr, const char *buf,
+		size_t count)
+{
+	struct cpumask stop_cpus;
+	unsigned long core, thr;
+
+	sscanf(buf, "%lx", &core);
+	if (core > NR_CORES)
+		return -1;
+	if (!test_bit(core, &kvm_on_core))
+		for (thr = 1; thr< threads_per_core; thr++)
+			if (cpu_online(thr * threads_per_core + thr))
+				cpumask_set_cpu(thr * threads_per_core + thr, &stop_cpus);
+
+	stop_machine(xics_migrate_irqs_away_secondary, NULL, &stop_cpus);
+	set_bit(core, &kvm_on_core);
+	return count;
+}
+
+static DEVICE_ATTR(kvm_enable, 0600,
+	show_kvm_enable, store_kvm_enable);
+
+static void sysfs_create_kvm_enable(void)
+{
+	device_create_file(cpu_subsys.dev_root, &dev_attr_kvm_enable);
+}
+#endif
+
 #endif /* CONFIG_PPC64 */
 
 #ifdef HAS_PPC_PMC_PA6T
diff --git a/arch/powerpc/sysdev/xics/xics-common.c b/arch/powerpc/sysdev/xics/xics-common.c
index fe0cca4..68b33d8 100644
--- a/arch/powerpc/sysdev/xics/xics-common.c
+++ b/arch/powerpc/sysdev/xics/xics-common.c
@@ -258,6 +258,18 @@  unlock:
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
 	}
 }
+
+int xics_migrate_irqs_away_secondary(void *data)
+{
+	int cpu = smp_processor_id();
+	if(cpu%thread_per_core != 0) {
+		WARN(condition, format...);
+		return 0;
+	}
+	/* In fact, if we can migrate the primary, it will be more fine */
+	xics_migrate_irqs_away();
+	return 0;
+}
 #endif /* CONFIG_HOTPLUG_CPU */
 
 #ifdef CONFIG_SMP