[v1] LoongArch: Do not create sysfs control file for io master CPUs

[PATCH] LoongArch: Do not create sysfs control file for io master CPUs

Posted by Tiezhu Yang 3 years, 6 months ago

Now io master CPUs are not hotpluggable on LoongArch, in the current code,
only /sys/devices/system/cpu/cpu0/online is not created, let us set the
hotpluggable field of all the io master CPUs as 0, then prevent to create
sysfs control file for the other io master CPUs which confuses some user
space tools. This is similar with commit 9cce844abf07 ("MIPS: CPU#0 is not
hotpluggable").

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 arch/loongarch/kernel/smp.c      |  8 --------
 arch/loongarch/kernel/topology.c | 12 +++++++++++-
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
index b5fab30..ef89292 100644
--- a/arch/loongarch/kernel/smp.c
+++ b/arch/loongarch/kernel/smp.c
@@ -240,19 +240,11 @@ void loongson3_smp_finish(void)
 
 #ifdef CONFIG_HOTPLUG_CPU
 
-static bool io_master(int cpu)
-{
-	return test_bit(cpu, &loongson_sysconf.cores_io_master);
-}
-
 int loongson3_cpu_disable(void)
 {
 	unsigned long flags;
 	unsigned int cpu = smp_processor_id();
 
-	if (io_master(cpu))
-		return -EBUSY;
-
 #ifdef CONFIG_NUMA
 	numa_remove_cpu(cpu);
 #endif
diff --git a/arch/loongarch/kernel/topology.c b/arch/loongarch/kernel/topology.c
index ab1a75c..7e7a77f 100644
--- a/arch/loongarch/kernel/topology.c
+++ b/arch/loongarch/kernel/topology.c
@@ -5,6 +5,7 @@
 #include <linux/node.h>
 #include <linux/nodemask.h>
 #include <linux/percpu.h>
+#include <asm/bootinfo.h>
 
 static DEFINE_PER_CPU(struct cpu, cpu_devices);
 
@@ -33,6 +34,11 @@ void arch_unregister_cpu(int cpu)
 EXPORT_SYMBOL(arch_unregister_cpu);
 #endif
 
+static bool io_master(int cpu)
+{
+	return test_bit(cpu, &loongson_sysconf.cores_io_master);
+}
+
 static int __init topology_init(void)
 {
 	int i, ret;
@@ -40,7 +46,11 @@ static int __init topology_init(void)
 	for_each_present_cpu(i) {
 		struct cpu *c = &per_cpu(cpu_devices, i);
 
-		c->hotpluggable = !!i;
+		if (io_master(i))
+			c->hotpluggable = 0;
+		else
+			c->hotpluggable = 1;
+
 		ret = register_cpu(c, i);
 		if (ret < 0)
 			pr_warn("topology_init: register_cpu %d failed (%d)\n", i, ret);
-- 
2.1.0

Re: [PATCH] LoongArch: Do not create sysfs control file for io master CPUs

Posted by WANG Xuerui 3 years, 6 months ago

On 2022/10/8 16:59, Tiezhu Yang wrote:
> Now io master CPUs are not hotpluggable on LoongArch, in the current code,
> only /sys/devices/system/cpu/cpu0/online is not created, let us set the
> hotpluggable field of all the io master CPUs as 0, then prevent to create
> sysfs control file for the other io master CPUs which confuses some user
> space tools. This is similar with commit 9cce844abf07 ("MIPS: CPU#0 is not
> hotpluggable").
> 
> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> ---
>   arch/loongarch/kernel/smp.c      |  8 --------
>   arch/loongarch/kernel/topology.c | 12 +++++++++++-
>   2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
> index b5fab30..ef89292 100644
> --- a/arch/loongarch/kernel/smp.c
> +++ b/arch/loongarch/kernel/smp.c
> @@ -240,19 +240,11 @@ void loongson3_smp_finish(void)
>   
>   #ifdef CONFIG_HOTPLUG_CPU
>   
> -static bool io_master(int cpu)
> -{
> -	return test_bit(cpu, &loongson_sysconf.cores_io_master);
> -}
> -
>   int loongson3_cpu_disable(void)
>   {
>   	unsigned long flags;
>   	unsigned int cpu = smp_processor_id();
>   
> -	if (io_master(cpu))
> -		return -EBUSY;
> -

Could this get invoked from somewhere other than the sysfs entries that 
"confuse user-space tools", e.g. from somewhere else in kernel land? If 
so (or if we can't rule out the possibility) keeping the guard here 
might prove more prudent.

>   #ifdef CONFIG_NUMA
>   	numa_remove_cpu(cpu);
>   #endif
> diff --git a/arch/loongarch/kernel/topology.c b/arch/loongarch/kernel/topology.c
> index ab1a75c..7e7a77f 100644
> --- a/arch/loongarch/kernel/topology.c
> +++ b/arch/loongarch/kernel/topology.c
> @@ -5,6 +5,7 @@
>   #include <linux/node.h>
>   #include <linux/nodemask.h>
>   #include <linux/percpu.h>
> +#include <asm/bootinfo.h>
>   
>   static DEFINE_PER_CPU(struct cpu, cpu_devices);
>   
> @@ -33,6 +34,11 @@ void arch_unregister_cpu(int cpu)
>   EXPORT_SYMBOL(arch_unregister_cpu);
>   #endif
>   
> +static bool io_master(int cpu)
> +{
> +	return test_bit(cpu, &loongson_sysconf.cores_io_master);
> +}
> +
>   static int __init topology_init(void)
>   {
>   	int i, ret;
> @@ -40,7 +46,11 @@ static int __init topology_init(void)
>   	for_each_present_cpu(i) {
>   		struct cpu *c = &per_cpu(cpu_devices, i);
>   
> -		c->hotpluggable = !!i;
> +		if (io_master(i))
> +			c->hotpluggable = 0;
> +		else
> +			c->hotpluggable = 1;
> +

This is just "c->hotpluggable = !io_master(i);".

>   		ret = register_cpu(c, i);
>   		if (ret < 0)
>   			pr_warn("topology_init: register_cpu %d failed (%d)\n", i, ret);
Other changes should be okay as they are in line with the previous MIPS 
change you referenced, but let's see what Huacai thinks.

-- 
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/

Re: [PATCH] LoongArch: Do not create sysfs control file for io master CPUs

Posted by Tiezhu Yang 3 years, 6 months ago


On 10/08/2022 05:27 PM, WANG Xuerui wrote:
> On 2022/10/8 16:59, Tiezhu Yang wrote:
>> Now io master CPUs are not hotpluggable on LoongArch, in the current
>> code,
>> only /sys/devices/system/cpu/cpu0/online is not created, let us set the
>> hotpluggable field of all the io master CPUs as 0, then prevent to create
>> sysfs control file for the other io master CPUs which confuses some user
>> space tools. This is similar with commit 9cce844abf07 ("MIPS: CPU#0 is
>> not
>> hotpluggable").
>>
>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>> ---
>>   arch/loongarch/kernel/smp.c      |  8 --------
>>   arch/loongarch/kernel/topology.c | 12 +++++++++++-
>>   2 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
>> index b5fab30..ef89292 100644
>> --- a/arch/loongarch/kernel/smp.c
>> +++ b/arch/loongarch/kernel/smp.c
>> @@ -240,19 +240,11 @@ void loongson3_smp_finish(void)
>>     #ifdef CONFIG_HOTPLUG_CPU
>>   -static bool io_master(int cpu)
>> -{
>> -    return test_bit(cpu, &loongson_sysconf.cores_io_master);
>> -}
>> -
>>   int loongson3_cpu_disable(void)
>>   {
>>       unsigned long flags;
>>       unsigned int cpu = smp_processor_id();
>>   -    if (io_master(cpu))
>> -        return -EBUSY;
>> -
>
> Could this get invoked from somewhere other than the sysfs entries that
> "confuse user-space tools", e.g. from somewhere else in kernel land? If
> so (or if we can't rule out the possibility) keeping the guard here
> might prove more prudent.
>

If c->hotpluggable is 0, it will not generate a control file in sysfs
for this CPU, for example:

[root@linux loongson]# cat /sys/devices/system/cpu/cpu0/online
cat: /sys/devices/system/cpu/cpu0/online: No such file or directory
[root@linux loongson]# echo 0 > /sys/devices/system/cpu/cpu0/online
bash: /sys/devices/system/cpu/cpu0/online: Permission denied

So no need to check it here, just remove the code.

This was done in commit cbab54d9c2b2 ("MIPS: No need to check CPU 0 in
{loongson3,bmips,octeon}_cpu_disable()").

>>   #ifdef CONFIG_NUMA
>>       numa_remove_cpu(cpu);
>>   #endif
>> diff --git a/arch/loongarch/kernel/topology.c
>> b/arch/loongarch/kernel/topology.c
>> index ab1a75c..7e7a77f 100644
>> --- a/arch/loongarch/kernel/topology.c
>> +++ b/arch/loongarch/kernel/topology.c
>> @@ -5,6 +5,7 @@
>>   #include <linux/node.h>
>>   #include <linux/nodemask.h>
>>   #include <linux/percpu.h>
>> +#include <asm/bootinfo.h>
>>     static DEFINE_PER_CPU(struct cpu, cpu_devices);
>>   @@ -33,6 +34,11 @@ void arch_unregister_cpu(int cpu)
>>   EXPORT_SYMBOL(arch_unregister_cpu);
>>   #endif
>>   +static bool io_master(int cpu)
>> +{
>> +    return test_bit(cpu, &loongson_sysconf.cores_io_master);
>> +}
>> +
>>   static int __init topology_init(void)
>>   {
>>       int i, ret;
>> @@ -40,7 +46,11 @@ static int __init topology_init(void)
>>       for_each_present_cpu(i) {
>>           struct cpu *c = &per_cpu(cpu_devices, i);
>>   -        c->hotpluggable = !!i;
>> +        if (io_master(i))
>> +            c->hotpluggable = 0;
>> +        else
>> +            c->hotpluggable = 1;
>> +
>
> This is just "c->hotpluggable = !io_master(i);".
>

Yes, I am OK either way, if it is necessary to send v2,
please let me know.

>>           ret = register_cpu(c, i);
>>           if (ret < 0)
>>               pr_warn("topology_init: register_cpu %d failed (%d)\n",
>> i, ret);
> Other changes should be okay as they are in line with the previous MIPS
> change you referenced, but let's see what Huacai thinks.
>

Thanks,
Tiezhu

Re: [PATCH] LoongArch: Do not create sysfs control file for io master CPUs

Posted by Tiezhu Yang 3 years, 6 months ago


On 10/08/2022 05:51 PM, Tiezhu Yang wrote:
>
>
> On 10/08/2022 05:27 PM, WANG Xuerui wrote:
>> On 2022/10/8 16:59, Tiezhu Yang wrote:
>>> Now io master CPUs are not hotpluggable on LoongArch, in the current
>>> code,
>>> only /sys/devices/system/cpu/cpu0/online is not created, let us set the
>>> hotpluggable field of all the io master CPUs as 0, then prevent to
>>> create
>>> sysfs control file for the other io master CPUs which confuses some user
>>> space tools. This is similar with commit 9cce844abf07 ("MIPS: CPU#0 is
>>> not
>>> hotpluggable").
>>>
>>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
>>> ---
>>>   arch/loongarch/kernel/smp.c      |  8 --------
>>>   arch/loongarch/kernel/topology.c | 12 +++++++++++-
>>>   2 files changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
>>> index b5fab30..ef89292 100644
>>> --- a/arch/loongarch/kernel/smp.c
>>> +++ b/arch/loongarch/kernel/smp.c
>>> @@ -240,19 +240,11 @@ void loongson3_smp_finish(void)
>>>     #ifdef CONFIG_HOTPLUG_CPU
>>>   -static bool io_master(int cpu)
>>> -{
>>> -    return test_bit(cpu, &loongson_sysconf.cores_io_master);
>>> -}
>>> -
>>>   int loongson3_cpu_disable(void)
>>>   {
>>>       unsigned long flags;
>>>       unsigned int cpu = smp_processor_id();
>>>   -    if (io_master(cpu))
>>> -        return -EBUSY;
>>> -
>>
>> Could this get invoked from somewhere other than the sysfs entries that
>> "confuse user-space tools", e.g. from somewhere else in kernel land? If
>> so (or if we can't rule out the possibility) keeping the guard here
>> might prove more prudent.
>>

takedown_cpu()  kernel/cpu.c
take_cpu_down()  kernel/cpu.c
__cpu_disable()  arch/loongarch/include/asm/smp.h
loongson3_cpu_disable()  arch/loongarch/kernel/smp.c

So I think you are right, keeping the guard here might prove more 
prudent, then it is better move io_master() to a header file that
can be used in smp.c and topology.c.

Let us wait for more review comments, thank you.

>
> If c->hotpluggable is 0, it will not generate a control file in sysfs
> for this CPU, for example:
>
> [root@linux loongson]# cat /sys/devices/system/cpu/cpu0/online
> cat: /sys/devices/system/cpu/cpu0/online: No such file or directory
> [root@linux loongson]# echo 0 > /sys/devices/system/cpu/cpu0/online
> bash: /sys/devices/system/cpu/cpu0/online: Permission denied
>
> So no need to check it here, just remove the code.
>
> This was done in commit cbab54d9c2b2 ("MIPS: No need to check CPU 0 in
> {loongson3,bmips,octeon}_cpu_disable()").
>
>>>   #ifdef CONFIG_NUMA
>>>       numa_remove_cpu(cpu);
>>>   #endif
>>> diff --git a/arch/loongarch/kernel/topology.c
>>> b/arch/loongarch/kernel/topology.c
>>> index ab1a75c..7e7a77f 100644
>>> --- a/arch/loongarch/kernel/topology.c
>>> +++ b/arch/loongarch/kernel/topology.c
>>> @@ -5,6 +5,7 @@
>>>   #include <linux/node.h>
>>>   #include <linux/nodemask.h>
>>>   #include <linux/percpu.h>
>>> +#include <asm/bootinfo.h>
>>>     static DEFINE_PER_CPU(struct cpu, cpu_devices);
>>>   @@ -33,6 +34,11 @@ void arch_unregister_cpu(int cpu)
>>>   EXPORT_SYMBOL(arch_unregister_cpu);
>>>   #endif
>>>   +static bool io_master(int cpu)
>>> +{
>>> +    return test_bit(cpu, &loongson_sysconf.cores_io_master);
>>> +}
>>> +
>>>   static int __init topology_init(void)
>>>   {
>>>       int i, ret;
>>> @@ -40,7 +46,11 @@ static int __init topology_init(void)
>>>       for_each_present_cpu(i) {
>>>           struct cpu *c = &per_cpu(cpu_devices, i);
>>>   -        c->hotpluggable = !!i;
>>> +        if (io_master(i))
>>> +            c->hotpluggable = 0;
>>> +        else
>>> +            c->hotpluggable = 1;
>>> +
>>
>> This is just "c->hotpluggable = !io_master(i);".
>>
>
> Yes, I am OK either way, if it is necessary to send v2,
> please let me know.
>
>>>           ret = register_cpu(c, i);
>>>           if (ret < 0)
>>>               pr_warn("topology_init: register_cpu %d failed (%d)\n",
>>> i, ret);
>> Other changes should be okay as they are in line with the previous MIPS
>> change you referenced, but let's see what Huacai thinks.
>>
>
> Thanks,
> Tiezhu

Re: [PATCH] LoongArch: Do not create sysfs control file for io master CPUs

Posted by Huacai Chen 3 years, 6 months ago

On Sat, Oct 8, 2022 at 6:44 PM Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
>
>
>
> On 10/08/2022 05:51 PM, Tiezhu Yang wrote:
> >
> >
> > On 10/08/2022 05:27 PM, WANG Xuerui wrote:
> >> On 2022/10/8 16:59, Tiezhu Yang wrote:
> >>> Now io master CPUs are not hotpluggable on LoongArch, in the current
> >>> code,
> >>> only /sys/devices/system/cpu/cpu0/online is not created, let us set the
> >>> hotpluggable field of all the io master CPUs as 0, then prevent to
> >>> create
> >>> sysfs control file for the other io master CPUs which confuses some user
> >>> space tools. This is similar with commit 9cce844abf07 ("MIPS: CPU#0 is
> >>> not
> >>> hotpluggable").
> >>>
> >>> Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
> >>> ---
> >>>   arch/loongarch/kernel/smp.c      |  8 --------
> >>>   arch/loongarch/kernel/topology.c | 12 +++++++++++-
> >>>   2 files changed, 11 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/arch/loongarch/kernel/smp.c b/arch/loongarch/kernel/smp.c
> >>> index b5fab30..ef89292 100644
> >>> --- a/arch/loongarch/kernel/smp.c
> >>> +++ b/arch/loongarch/kernel/smp.c
> >>> @@ -240,19 +240,11 @@ void loongson3_smp_finish(void)
> >>>     #ifdef CONFIG_HOTPLUG_CPU
> >>>   -static bool io_master(int cpu)
> >>> -{
> >>> -    return test_bit(cpu, &loongson_sysconf.cores_io_master);
> >>> -}
> >>> -
> >>>   int loongson3_cpu_disable(void)
> >>>   {
> >>>       unsigned long flags;
> >>>       unsigned int cpu = smp_processor_id();
> >>>   -    if (io_master(cpu))
> >>> -        return -EBUSY;
> >>> -
> >>
> >> Could this get invoked from somewhere other than the sysfs entries that
> >> "confuse user-space tools", e.g. from somewhere else in kernel land? If
> >> so (or if we can't rule out the possibility) keeping the guard here
> >> might prove more prudent.
> >>
>
> takedown_cpu()  kernel/cpu.c
> take_cpu_down()  kernel/cpu.c
> __cpu_disable()  arch/loongarch/include/asm/smp.h
> loongson3_cpu_disable()  arch/loongarch/kernel/smp.c
>
> So I think you are right, keeping the guard here might prove more
> prudent, then it is better move io_master() to a header file that
> can be used in smp.c and topology.c.
Agree, please send V2, thanks.


Huacai
>
> Let us wait for more review comments, thank you.
>
> >
> > If c->hotpluggable is 0, it will not generate a control file in sysfs
> > for this CPU, for example:
> >
> > [root@linux loongson]# cat /sys/devices/system/cpu/cpu0/online
> > cat: /sys/devices/system/cpu/cpu0/online: No such file or directory
> > [root@linux loongson]# echo 0 > /sys/devices/system/cpu/cpu0/online
> > bash: /sys/devices/system/cpu/cpu0/online: Permission denied
> >
> > So no need to check it here, just remove the code.
> >
> > This was done in commit cbab54d9c2b2 ("MIPS: No need to check CPU 0 in
> > {loongson3,bmips,octeon}_cpu_disable()").
> >
> >>>   #ifdef CONFIG_NUMA
> >>>       numa_remove_cpu(cpu);
> >>>   #endif
> >>> diff --git a/arch/loongarch/kernel/topology.c
> >>> b/arch/loongarch/kernel/topology.c
> >>> index ab1a75c..7e7a77f 100644
> >>> --- a/arch/loongarch/kernel/topology.c
> >>> +++ b/arch/loongarch/kernel/topology.c
> >>> @@ -5,6 +5,7 @@
> >>>   #include <linux/node.h>
> >>>   #include <linux/nodemask.h>
> >>>   #include <linux/percpu.h>
> >>> +#include <asm/bootinfo.h>
> >>>     static DEFINE_PER_CPU(struct cpu, cpu_devices);
> >>>   @@ -33,6 +34,11 @@ void arch_unregister_cpu(int cpu)
> >>>   EXPORT_SYMBOL(arch_unregister_cpu);
> >>>   #endif
> >>>   +static bool io_master(int cpu)
> >>> +{
> >>> +    return test_bit(cpu, &loongson_sysconf.cores_io_master);
> >>> +}
> >>> +
> >>>   static int __init topology_init(void)
> >>>   {
> >>>       int i, ret;
> >>> @@ -40,7 +46,11 @@ static int __init topology_init(void)
> >>>       for_each_present_cpu(i) {
> >>>           struct cpu *c = &per_cpu(cpu_devices, i);
> >>>   -        c->hotpluggable = !!i;
> >>> +        if (io_master(i))
> >>> +            c->hotpluggable = 0;
> >>> +        else
> >>> +            c->hotpluggable = 1;
> >>> +
> >>
> >> This is just "c->hotpluggable = !io_master(i);".
> >>
> >
> > Yes, I am OK either way, if it is necessary to send v2,
> > please let me know.
> >
> >>>           ret = register_cpu(c, i);
> >>>           if (ret < 0)
> >>>               pr_warn("topology_init: register_cpu %d failed (%d)\n",
> >>> i, ret);
> >> Other changes should be okay as they are in line with the previous MIPS
> >> change you referenced, but let's see what Huacai thinks.
> >>
> >
> > Thanks,
> > Tiezhu
>