2013-08-30 00:24:04

by Toshi Kani

[permalink] [raw]
Subject: [PATCH v2 0/4] Unify CPU hotplug lock interface

lock_device_hotplug() was recently introduced to serialize CPU & Memory
online/offline and hotplug operations, along with sysfs online interface
restructure (commit 4f3549d7). With this new locking scheme,
cpu_hotplug_driver_lock() is redundant and is no longer necessary.

This patchset makes sure that lock_device_hotplug() covers all CPU online/
offline interfaces, and then removes cpu_hotplug_driver_lock().

v2:
- Rebased to the pm tree, bleeding-edge.
- Changed patch 2/4 to use lock_device_hotplug_sysfs().

---
Toshi Kani (4):
hotplug, x86: Fix online state in cpu0 debug interface
hotplug, x86: Add hotplug lock to missing places
hotplug, x86: Disable ARCH_CPU_PROBE_RELEASE on x86
hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()

---
arch/powerpc/kernel/smp.c | 12 ----------
arch/powerpc/platforms/pseries/dlpar.c | 40 +++++++++++++---------------------
arch/x86/Kconfig | 4 ----
arch/x86/kernel/smpboot.c | 21 ------------------
arch/x86/kernel/topology.c | 11 ++++++----
drivers/base/cpu.c | 34 +++++++++++++++++++----------
include/linux/cpu.h | 13 -----------
7 files changed, 45 insertions(+), 90 deletions(-)


2013-08-30 00:23:55

by Toshi Kani

[permalink] [raw]
Subject: [PATCH v2 3/4] hotplug, x86: Disable ARCH_CPU_PROBE_RELEASE on x86

Commit d7c53c9e enabled ARCH_CPU_PROBE_RELEASE on x86 in order to
serialize CPU online/offline operations. Although it is the config
option to enable CPU hotplug test interfaces, probe & release, it is
also the option to enable cpu_hotplug_driver_lock() as well. Therefore,
this option had to be enabled on x86 with dummy arch_cpu_probe() and
arch_cpu_release().

Since then, lock_device_hotplug() was introduced to serialize CPU
online/offline & hotplug operations. Therefore, this config option
is no longer required for the serialization. This patch disables
this config option on x86 and revert the changes made by commit
d7c53c9e.

Signed-off-by: Toshi Kani <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
---
arch/x86/Kconfig | 4 ----
arch/x86/kernel/smpboot.c | 21 ---------------------
2 files changed, 25 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index b32ebf9..c87e49a 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -255,10 +255,6 @@ config ARCH_HWEIGHT_CFLAGS
default "-fcall-saved-ecx -fcall-saved-edx" if X86_32
default "-fcall-saved-rdi -fcall-saved-rsi -fcall-saved-rdx -fcall-saved-rcx -fcall-saved-r8 -fcall-saved-r9 -fcall-saved-r10 -fcall-saved-r11" if X86_64

-config ARCH_CPU_PROBE_RELEASE
- def_bool y
- depends on HOTPLUG_CPU
-
config ARCH_SUPPORTS_UPROBES
def_bool y

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index aecc98a..5b24a9d 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -82,27 +82,6 @@
/* State of each CPU */
DEFINE_PER_CPU(int, cpu_state) = { 0 };

-#ifdef CONFIG_HOTPLUG_CPU
-/*
- * We need this for trampoline_base protection from concurrent accesses when
- * off- and onlining cores wildly.
- */
-static DEFINE_MUTEX(x86_cpu_hotplug_driver_mutex);
-
-void cpu_hotplug_driver_lock(void)
-{
- mutex_lock(&x86_cpu_hotplug_driver_mutex);
-}
-
-void cpu_hotplug_driver_unlock(void)
-{
- mutex_unlock(&x86_cpu_hotplug_driver_mutex);
-}
-
-ssize_t arch_cpu_probe(const char *buf, size_t count) { return -1; }
-ssize_t arch_cpu_release(const char *buf, size_t count) { return -1; }
-#endif
-
/* Number of siblings per CPU package */
int smp_num_siblings = 1;
EXPORT_SYMBOL(smp_num_siblings);

2013-08-30 00:23:57

by Toshi Kani

[permalink] [raw]
Subject: [PATCH v2 4/4] hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()

cpu_hotplug_driver_lock() serializes CPU online/offline operations
when ARCH_CPU_PROBE_RELEASE is set. This lock interface is no longer
necessary with the following reason:

- lock_device_hotplug() now protects CPU online/offline operations,
including the probe & release interfaces enabled by
ARCH_CPU_PROBE_RELEASE. The use of cpu_hotplug_driver_lock() is
redundant.
- cpu_hotplug_driver_lock() is only valid when ARCH_CPU_PROBE_RELEASE
is defined, which is misleading and is only enabled on powerpc.

This patch removes the cpu_hotplug_driver_lock() interface. As
a result, ARCH_CPU_PROBE_RELEASE only enables / disables the cpu
probe & release interface as intended. There is no functional change
in this patch.

Signed-off-by: Toshi Kani <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
Reviewed-by: Nathan Fontenot <[email protected]>
---
Performed build test only on powerpc.
---
arch/powerpc/kernel/smp.c | 12 ----------
arch/powerpc/platforms/pseries/dlpar.c | 40 ++++++++++++--------------------
arch/x86/kernel/topology.c | 2 --
drivers/base/cpu.c | 10 +-------
include/linux/cpu.h | 13 ----------
5 files changed, 16 insertions(+), 61 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 38b0ba6..1667269 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -763,18 +763,6 @@ void __cpu_die(unsigned int cpu)
smp_ops->cpu_die(cpu);
}

-static DEFINE_MUTEX(powerpc_cpu_hotplug_driver_mutex);
-
-void cpu_hotplug_driver_lock()
-{
- mutex_lock(&powerpc_cpu_hotplug_driver_mutex);
-}
-
-void cpu_hotplug_driver_unlock()
-{
- mutex_unlock(&powerpc_cpu_hotplug_driver_mutex);
-}
-
void cpu_die(void)
{
if (ppc_md.cpu_die)
diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
index a1a7b9a..e39325d 100644
--- a/arch/powerpc/platforms/pseries/dlpar.c
+++ b/arch/powerpc/platforms/pseries/dlpar.c
@@ -387,18 +387,13 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
char *cpu_name;
int rc;

- cpu_hotplug_driver_lock();
rc = strict_strtoul(buf, 0, &drc_index);
- if (rc) {
- rc = -EINVAL;
- goto out;
- }
+ if (rc)
+ return -EINVAL;

dn = dlpar_configure_connector(drc_index);
- if (!dn) {
- rc = -EINVAL;
- goto out;
- }
+ if (!dn)
+ return -EINVAL;

/* configure-connector reports cpus as living in the base
* directory of the device tree. CPUs actually live in the
@@ -407,8 +402,7 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
cpu_name = kasprintf(GFP_KERNEL, "/cpus%s", dn->full_name);
if (!cpu_name) {
dlpar_free_cc_nodes(dn);
- rc = -ENOMEM;
- goto out;
+ return -ENOMEM;
}

kfree(dn->full_name);
@@ -417,22 +411,21 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
rc = dlpar_acquire_drc(drc_index);
if (rc) {
dlpar_free_cc_nodes(dn);
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}

rc = dlpar_attach_node(dn);
if (rc) {
dlpar_release_drc(drc_index);
dlpar_free_cc_nodes(dn);
- goto out;
+ return rc;
}

rc = dlpar_online_cpu(dn);
-out:
- cpu_hotplug_driver_unlock();
+ if (rc)
+ return rc;

- return rc ? rc : count;
+ return count;
}

static int dlpar_offline_cpu(struct device_node *dn)
@@ -505,30 +498,27 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
return -EINVAL;
}

- cpu_hotplug_driver_lock();
rc = dlpar_offline_cpu(dn);
if (rc) {
of_node_put(dn);
- rc = -EINVAL;
- goto out;
+ return -EINVAL;
}

rc = dlpar_release_drc(*drc_index);
if (rc) {
of_node_put(dn);
- goto out;
+ return rc;
}

rc = dlpar_detach_node(dn);
if (rc) {
dlpar_acquire_drc(*drc_index);
- goto out;
+ return rc;
}

of_node_put(dn);
-out:
- cpu_hotplug_driver_unlock();
- return rc ? rc : count;
+
+ return count;
}

static int __init pseries_dlpar_init(void)
diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
index a3f35eb..649b010 100644
--- a/arch/x86/kernel/topology.c
+++ b/arch/x86/kernel/topology.c
@@ -66,7 +66,6 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
return -EINVAL;

lock_device_hotplug();
- cpu_hotplug_driver_lock();

switch (action) {
case 0:
@@ -91,7 +90,6 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
ret = -EINVAL;
}

- cpu_hotplug_driver_unlock();
unlock_device_hotplug();

return ret;
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index dc78e45..9745f03 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -46,8 +46,6 @@ static int __ref cpu_subsys_online(struct device *dev)
int from_nid, to_nid;
int ret;

- cpu_hotplug_driver_lock();
-
from_nid = cpu_to_node(cpuid);
ret = cpu_up(cpuid);
/*
@@ -58,18 +56,12 @@ static int __ref cpu_subsys_online(struct device *dev)
if (from_nid != to_nid)
change_cpu_under_node(cpu, from_nid, to_nid);

- cpu_hotplug_driver_unlock();
return ret;
}

static int cpu_subsys_offline(struct device *dev)
{
- int ret;
-
- cpu_hotplug_driver_lock();
- ret = cpu_down(dev->id);
- cpu_hotplug_driver_unlock();
- return ret;
+ return cpu_down(dev->id);
}

void unregister_cpu(struct cpu *cpu)
diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 801ff9e..3434ef7 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -185,19 +185,6 @@ extern void cpu_hotplug_enable(void);
void clear_tasks_mm_cpumask(int cpu);
int cpu_down(unsigned int cpu);

-#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
-extern void cpu_hotplug_driver_lock(void);
-extern void cpu_hotplug_driver_unlock(void);
-#else
-static inline void cpu_hotplug_driver_lock(void)
-{
-}
-
-static inline void cpu_hotplug_driver_unlock(void)
-{
-}
-#endif
-
#else /* CONFIG_HOTPLUG_CPU */

static inline void cpu_hotplug_begin(void) {}

2013-08-30 00:23:53

by Toshi Kani

[permalink] [raw]
Subject: [PATCH v2 1/4] hotplug, x86: Fix online state in cpu0 debug interface

_debug_hotplug_cpu() is a debug interface that puts cpu0 offline during
boot-up when CONFIG_DEBUG_HOTPLUG_CPU0 is set. After cpu0 is put offline
in this interface, however, /sys/devices/system/cpu/cpu0/online still
shows 1 (online).

This patch fixes _debug_hotplug_cpu() to update dev->offline when CPU
online/offline operation succeeded.

Signed-off-by: Toshi Kani <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
---
arch/x86/kernel/topology.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
index 6e60b5f..5823bbd 100644
--- a/arch/x86/kernel/topology.c
+++ b/arch/x86/kernel/topology.c
@@ -72,16 +72,19 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
ret = cpu_down(cpu);
if (!ret) {
pr_info("CPU %u is now offline\n", cpu);
+ dev->offline = true;
kobject_uevent(&dev->kobj, KOBJ_OFFLINE);
} else
pr_debug("Can't offline CPU%d.\n", cpu);
break;
case 1:
ret = cpu_up(cpu);
- if (!ret)
+ if (!ret) {
+ dev->offline = false;
kobject_uevent(&dev->kobj, KOBJ_ONLINE);
- else
+ } else {
pr_debug("Can't online CPU%d.\n", cpu);
+ }
break;
default:
ret = -EINVAL;

2013-08-30 00:25:01

by Toshi Kani

[permalink] [raw]
Subject: [PATCH v2 2/4] hotplug, x86: Add hotplug lock to missing places

lock_device_hotplug[_sysfs]() serializes CPU & Memory online/offline
and hotplug operations. However, this lock is not held in the debug
interfaces below that initiate CPU online/offline operations.

- _debug_hotplug_cpu(), cpu0 hotplug test interface enabled by
CONFIG_DEBUG_HOTPLUG_CPU0.
- cpu_probe_store() and cpu_release_store(), cpu hotplug test interface
enabled by CONFIG_ARCH_CPU_PROBE_RELEASE.

This patch changes the above interfaces to hold lock_device_hotplug().

Signed-off-by: Toshi Kani <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Acked-by: Rafael J. Wysocki <[email protected]>
---
arch/x86/kernel/topology.c | 2 ++
drivers/base/cpu.c | 24 ++++++++++++++++++++++--
2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
index 5823bbd..a3f35eb 100644
--- a/arch/x86/kernel/topology.c
+++ b/arch/x86/kernel/topology.c
@@ -65,6 +65,7 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
if (!cpu_is_hotpluggable(cpu))
return -EINVAL;

+ lock_device_hotplug();
cpu_hotplug_driver_lock();

switch (action) {
@@ -91,6 +92,7 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
}

cpu_hotplug_driver_unlock();
+ unlock_device_hotplug();

return ret;
}
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 4cf0717..dc78e45 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -89,7 +89,17 @@ static ssize_t cpu_probe_store(struct device *dev,
const char *buf,
size_t count)
{
- return arch_cpu_probe(buf, count);
+ ssize_t cnt;
+ int ret;
+
+ ret = lock_device_hotplug_sysfs();
+ if (ret)
+ return ret;
+
+ cnt = arch_cpu_probe(buf, count);
+
+ unlock_device_hotplug();
+ return cnt;
}

static ssize_t cpu_release_store(struct device *dev,
@@ -97,7 +107,17 @@ static ssize_t cpu_release_store(struct device *dev,
const char *buf,
size_t count)
{
- return arch_cpu_release(buf, count);
+ ssize_t cnt;
+ int ret;
+
+ ret = lock_device_hotplug_sysfs();
+ if (ret)
+ return ret;
+
+ cnt = arch_cpu_release(buf, count);
+
+ unlock_device_hotplug();
+ return cnt;
}

static DEVICE_ATTR(probe, S_IWUSR, NULL, cpu_probe_store);

2013-08-30 02:45:07

by Yasuaki Ishimatsu

[permalink] [raw]
Subject: Re: [PATCH v2 0/4] Unify CPU hotplug lock interface

(2013/08/30 9:22), Toshi Kani wrote:
> lock_device_hotplug() was recently introduced to serialize CPU & Memory
> online/offline and hotplug operations, along with sysfs online interface
> restructure (commit 4f3549d7). With this new locking scheme,
> cpu_hotplug_driver_lock() is redundant and is no longer necessary.
>
> This patchset makes sure that lock_device_hotplug() covers all CPU online/
> offline interfaces, and then removes cpu_hotplug_driver_lock().
>
> v2:
> - Rebased to the pm tree, bleeding-edge.
> - Changed patch 2/4 to use lock_device_hotplug_sysfs().
>
> ---
> Toshi Kani (4):
> hotplug, x86: Fix online state in cpu0 debug interface
> hotplug, x86: Add hotplug lock to missing places
> hotplug, x86: Disable ARCH_CPU_PROBE_RELEASE on x86
> hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()
>
> ---
The patch-set looks good to me.

Acked-by: Yasuaki Ishimatsu <[email protected]>

Thanks,
Yasuaki Ishimatsu


> arch/powerpc/kernel/smp.c | 12 ----------
> arch/powerpc/platforms/pseries/dlpar.c | 40 +++++++++++++---------------------
> arch/x86/Kconfig | 4 ----
> arch/x86/kernel/smpboot.c | 21 ------------------
> arch/x86/kernel/topology.c | 11 ++++++----
> drivers/base/cpu.c | 34 +++++++++++++++++++----------
> include/linux/cpu.h | 13 -----------
> 7 files changed, 45 insertions(+), 90 deletions(-)
>

2013-08-30 12:03:56

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH v2 0/4] Unify CPU hotplug lock interface

On Friday, August 30, 2013 11:44:06 AM Yasuaki Ishimatsu wrote:
> (2013/08/30 9:22), Toshi Kani wrote:
> > lock_device_hotplug() was recently introduced to serialize CPU & Memory
> > online/offline and hotplug operations, along with sysfs online interface
> > restructure (commit 4f3549d7). With this new locking scheme,
> > cpu_hotplug_driver_lock() is redundant and is no longer necessary.
> >
> > This patchset makes sure that lock_device_hotplug() covers all CPU online/
> > offline interfaces, and then removes cpu_hotplug_driver_lock().
> >
> > v2:
> > - Rebased to the pm tree, bleeding-edge.
> > - Changed patch 2/4 to use lock_device_hotplug_sysfs().
> >
> > ---
> > Toshi Kani (4):
> > hotplug, x86: Fix online state in cpu0 debug interface
> > hotplug, x86: Add hotplug lock to missing places
> > hotplug, x86: Disable ARCH_CPU_PROBE_RELEASE on x86
> > hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()
> >
> > ---
> The patch-set looks good to me.
>
> Acked-by: Yasuaki Ishimatsu <[email protected]>

Thanks! I'll tentatively queue up this series for 3.12 (for the second
half of the merge window).

Thanks,
Rafael

2013-08-30 14:35:09

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH v2 0/4] Unify CPU hotplug lock interface

On Fri, 2013-08-30 at 11:44 +0900, Yasuaki Ishimatsu wrote:
> (2013/08/30 9:22), Toshi Kani wrote:
> > lock_device_hotplug() was recently introduced to serialize CPU & Memory
> > online/offline and hotplug operations, along with sysfs online interface
> > restructure (commit 4f3549d7). With this new locking scheme,
> > cpu_hotplug_driver_lock() is redundant and is no longer necessary.
> >
> > This patchset makes sure that lock_device_hotplug() covers all CPU online/
> > offline interfaces, and then removes cpu_hotplug_driver_lock().
> >
> > v2:
> > - Rebased to the pm tree, bleeding-edge.
> > - Changed patch 2/4 to use lock_device_hotplug_sysfs().
> >
> > ---
> > Toshi Kani (4):
> > hotplug, x86: Fix online state in cpu0 debug interface
> > hotplug, x86: Add hotplug lock to missing places
> > hotplug, x86: Disable ARCH_CPU_PROBE_RELEASE on x86
> > hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()
> >
> > ---
> The patch-set looks good to me.
>
> Acked-by: Yasuaki Ishimatsu <[email protected]>

Thanks Yasuaki!
-Toshi

2013-08-30 14:37:13

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH v2 0/4] Unify CPU hotplug lock interface

On Fri, 2013-08-30 at 14:14 +0200, Rafael J. Wysocki wrote:
> On Friday, August 30, 2013 11:44:06 AM Yasuaki Ishimatsu wrote:
> > (2013/08/30 9:22), Toshi Kani wrote:
> > > lock_device_hotplug() was recently introduced to serialize CPU & Memory
> > > online/offline and hotplug operations, along with sysfs online interface
> > > restructure (commit 4f3549d7). With this new locking scheme,
> > > cpu_hotplug_driver_lock() is redundant and is no longer necessary.
> > >
> > > This patchset makes sure that lock_device_hotplug() covers all CPU online/
> > > offline interfaces, and then removes cpu_hotplug_driver_lock().
> > >
> > > v2:
> > > - Rebased to the pm tree, bleeding-edge.
> > > - Changed patch 2/4 to use lock_device_hotplug_sysfs().
> > >
> > > ---
> > > Toshi Kani (4):
> > > hotplug, x86: Fix online state in cpu0 debug interface
> > > hotplug, x86: Add hotplug lock to missing places
> > > hotplug, x86: Disable ARCH_CPU_PROBE_RELEASE on x86
> > > hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()
> > >
> > > ---
> > The patch-set looks good to me.
> >
> > Acked-by: Yasuaki Ishimatsu <[email protected]>
>
> Thanks! I'll tentatively queue up this series for 3.12 (for the second
> half of the merge window).

Thanks Rafael!
-Toshi

2013-08-31 00:13:40

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH v2 1/4] hotplug, x86: Fix online state in cpu0 debug interface

Hi Peter,

Any objections here?

On Thursday, August 29, 2013 06:22:06 PM Toshi Kani wrote:
> _debug_hotplug_cpu() is a debug interface that puts cpu0 offline during
> boot-up when CONFIG_DEBUG_HOTPLUG_CPU0 is set. After cpu0 is put offline
> in this interface, however, /sys/devices/system/cpu/cpu0/online still
> shows 1 (online).
>
> This patch fixes _debug_hotplug_cpu() to update dev->offline when CPU
> online/offline operation succeeded.
>
> Signed-off-by: Toshi Kani <[email protected]>
> Acked-by: Rafael J. Wysocki <[email protected]>
> ---
> arch/x86/kernel/topology.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
> index 6e60b5f..5823bbd 100644
> --- a/arch/x86/kernel/topology.c
> +++ b/arch/x86/kernel/topology.c
> @@ -72,16 +72,19 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
> ret = cpu_down(cpu);
> if (!ret) {
> pr_info("CPU %u is now offline\n", cpu);
> + dev->offline = true;
> kobject_uevent(&dev->kobj, KOBJ_OFFLINE);
> } else
> pr_debug("Can't offline CPU%d.\n", cpu);
> break;
> case 1:
> ret = cpu_up(cpu);
> - if (!ret)
> + if (!ret) {
> + dev->offline = false;
> kobject_uevent(&dev->kobj, KOBJ_ONLINE);
> - else
> + } else {
> pr_debug("Can't online CPU%d.\n", cpu);
> + }
> break;
> default:
> ret = -EINVAL;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2013-09-25 08:23:05

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()

Hi,

On Thursday, August 29, 2013 06:22:09 PM Toshi Kani wrote:
> cpu_hotplug_driver_lock() serializes CPU online/offline operations
> when ARCH_CPU_PROBE_RELEASE is set. This lock interface is no longer
> necessary with the following reason:
>
> - lock_device_hotplug() now protects CPU online/offline operations,
> including the probe & release interfaces enabled by
> ARCH_CPU_PROBE_RELEASE. The use of cpu_hotplug_driver_lock() is
> redundant.
> - cpu_hotplug_driver_lock() is only valid when ARCH_CPU_PROBE_RELEASE
> is defined, which is misleading and is only enabled on powerpc.
>
> This patch removes the cpu_hotplug_driver_lock() interface. As
> a result, ARCH_CPU_PROBE_RELEASE only enables / disables the cpu
> probe & release interface as intended. There is no functional change
> in this patch.
>
> Signed-off-by: Toshi Kani <[email protected]>
> Acked-by: Rafael J. Wysocki <[email protected]>
> Reviewed-by: Nathan Fontenot <[email protected]>

Can you please rebase this patch on top of 3.12-rc2? It doesn't apply for
me any more.

Thanks,
Rafael


> ---
> Performed build test only on powerpc.
> ---
> arch/powerpc/kernel/smp.c | 12 ----------
> arch/powerpc/platforms/pseries/dlpar.c | 40 ++++++++++++--------------------
> arch/x86/kernel/topology.c | 2 --
> drivers/base/cpu.c | 10 +-------
> include/linux/cpu.h | 13 ----------
> 5 files changed, 16 insertions(+), 61 deletions(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 38b0ba6..1667269 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -763,18 +763,6 @@ void __cpu_die(unsigned int cpu)
> smp_ops->cpu_die(cpu);
> }
>
> -static DEFINE_MUTEX(powerpc_cpu_hotplug_driver_mutex);
> -
> -void cpu_hotplug_driver_lock()
> -{
> - mutex_lock(&powerpc_cpu_hotplug_driver_mutex);
> -}
> -
> -void cpu_hotplug_driver_unlock()
> -{
> - mutex_unlock(&powerpc_cpu_hotplug_driver_mutex);
> -}
> -
> void cpu_die(void)
> {
> if (ppc_md.cpu_die)
> diff --git a/arch/powerpc/platforms/pseries/dlpar.c b/arch/powerpc/platforms/pseries/dlpar.c
> index a1a7b9a..e39325d 100644
> --- a/arch/powerpc/platforms/pseries/dlpar.c
> +++ b/arch/powerpc/platforms/pseries/dlpar.c
> @@ -387,18 +387,13 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
> char *cpu_name;
> int rc;
>
> - cpu_hotplug_driver_lock();
> rc = strict_strtoul(buf, 0, &drc_index);
> - if (rc) {
> - rc = -EINVAL;
> - goto out;
> - }
> + if (rc)
> + return -EINVAL;
>
> dn = dlpar_configure_connector(drc_index);
> - if (!dn) {
> - rc = -EINVAL;
> - goto out;
> - }
> + if (!dn)
> + return -EINVAL;
>
> /* configure-connector reports cpus as living in the base
> * directory of the device tree. CPUs actually live in the
> @@ -407,8 +402,7 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
> cpu_name = kasprintf(GFP_KERNEL, "/cpus%s", dn->full_name);
> if (!cpu_name) {
> dlpar_free_cc_nodes(dn);
> - rc = -ENOMEM;
> - goto out;
> + return -ENOMEM;
> }
>
> kfree(dn->full_name);
> @@ -417,22 +411,21 @@ static ssize_t dlpar_cpu_probe(const char *buf, size_t count)
> rc = dlpar_acquire_drc(drc_index);
> if (rc) {
> dlpar_free_cc_nodes(dn);
> - rc = -EINVAL;
> - goto out;
> + return -EINVAL;
> }
>
> rc = dlpar_attach_node(dn);
> if (rc) {
> dlpar_release_drc(drc_index);
> dlpar_free_cc_nodes(dn);
> - goto out;
> + return rc;
> }
>
> rc = dlpar_online_cpu(dn);
> -out:
> - cpu_hotplug_driver_unlock();
> + if (rc)
> + return rc;
>
> - return rc ? rc : count;
> + return count;
> }
>
> static int dlpar_offline_cpu(struct device_node *dn)
> @@ -505,30 +498,27 @@ static ssize_t dlpar_cpu_release(const char *buf, size_t count)
> return -EINVAL;
> }
>
> - cpu_hotplug_driver_lock();
> rc = dlpar_offline_cpu(dn);
> if (rc) {
> of_node_put(dn);
> - rc = -EINVAL;
> - goto out;
> + return -EINVAL;
> }
>
> rc = dlpar_release_drc(*drc_index);
> if (rc) {
> of_node_put(dn);
> - goto out;
> + return rc;
> }
>
> rc = dlpar_detach_node(dn);
> if (rc) {
> dlpar_acquire_drc(*drc_index);
> - goto out;
> + return rc;
> }
>
> of_node_put(dn);
> -out:
> - cpu_hotplug_driver_unlock();
> - return rc ? rc : count;
> +
> + return count;
> }
>
> static int __init pseries_dlpar_init(void)
> diff --git a/arch/x86/kernel/topology.c b/arch/x86/kernel/topology.c
> index a3f35eb..649b010 100644
> --- a/arch/x86/kernel/topology.c
> +++ b/arch/x86/kernel/topology.c
> @@ -66,7 +66,6 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
> return -EINVAL;
>
> lock_device_hotplug();
> - cpu_hotplug_driver_lock();
>
> switch (action) {
> case 0:
> @@ -91,7 +90,6 @@ int __ref _debug_hotplug_cpu(int cpu, int action)
> ret = -EINVAL;
> }
>
> - cpu_hotplug_driver_unlock();
> unlock_device_hotplug();
>
> return ret;
> diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
> index dc78e45..9745f03 100644
> --- a/drivers/base/cpu.c
> +++ b/drivers/base/cpu.c
> @@ -46,8 +46,6 @@ static int __ref cpu_subsys_online(struct device *dev)
> int from_nid, to_nid;
> int ret;
>
> - cpu_hotplug_driver_lock();
> -
> from_nid = cpu_to_node(cpuid);
> ret = cpu_up(cpuid);
> /*
> @@ -58,18 +56,12 @@ static int __ref cpu_subsys_online(struct device *dev)
> if (from_nid != to_nid)
> change_cpu_under_node(cpu, from_nid, to_nid);
>
> - cpu_hotplug_driver_unlock();
> return ret;
> }
>
> static int cpu_subsys_offline(struct device *dev)
> {
> - int ret;
> -
> - cpu_hotplug_driver_lock();
> - ret = cpu_down(dev->id);
> - cpu_hotplug_driver_unlock();
> - return ret;
> + return cpu_down(dev->id);
> }
>
> void unregister_cpu(struct cpu *cpu)
> diff --git a/include/linux/cpu.h b/include/linux/cpu.h
> index 801ff9e..3434ef7 100644
> --- a/include/linux/cpu.h
> +++ b/include/linux/cpu.h
> @@ -185,19 +185,6 @@ extern void cpu_hotplug_enable(void);
> void clear_tasks_mm_cpumask(int cpu);
> int cpu_down(unsigned int cpu);
>
> -#ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
> -extern void cpu_hotplug_driver_lock(void);
> -extern void cpu_hotplug_driver_unlock(void);
> -#else
> -static inline void cpu_hotplug_driver_lock(void)
> -{
> -}
> -
> -static inline void cpu_hotplug_driver_unlock(void)
> -{
> -}
> -#endif
> -
> #else /* CONFIG_HOTPLUG_CPU */
>
> static inline void cpu_hotplug_begin(void) {}
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2013-09-25 20:47:03

by Toshi Kani

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] hotplug, powerpc, x86: Remove cpu_hotplug_driver_lock()

On Wed, 2013-09-25 at 08:27 +0000, Rafael J. Wysocki wrote:
> Hi,
>
> On Thursday, August 29, 2013 06:22:09 PM Toshi Kani wrote:
> > cpu_hotplug_driver_lock() serializes CPU online/offline operations
> > when ARCH_CPU_PROBE_RELEASE is set. This lock interface is no longer
> > necessary with the following reason:
> >
> > - lock_device_hotplug() now protects CPU online/offline operations,
> > including the probe & release interfaces enabled by
> > ARCH_CPU_PROBE_RELEASE. The use of cpu_hotplug_driver_lock() is
> > redundant.
> > - cpu_hotplug_driver_lock() is only valid when ARCH_CPU_PROBE_RELEASE
> > is defined, which is misleading and is only enabled on powerpc.
> >
> > This patch removes the cpu_hotplug_driver_lock() interface. As
> > a result, ARCH_CPU_PROBE_RELEASE only enables / disables the cpu
> > probe & release interface as intended. There is no functional change
> > in this patch.
> >
> > Signed-off-by: Toshi Kani <[email protected]>
> > Acked-by: Rafael J. Wysocki <[email protected]>
> > Reviewed-by: Nathan Fontenot <[email protected]>
>
> Can you please rebase this patch on top of 3.12-rc2? It doesn't apply for
> me any more.

Yes, I will send this patch on top of 3.12-rc2.

Thanks,
-Toshi