Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Dave Hansen]

On Fri, 2004-06-04 at 15:49, Ashok Raj wrote:
> place a file in /sys/devices/system/cpuinfo, which when one does a cat on
> can display
> 
> cpu0:	online, not offlinable
> cpu1:	online
> cpu2:	offline (meaning present)

This might be a little bit more than is intended for a sysfs file,
especially if the information can be obtained some other way.  Here's
what I was thinking of:

cat /sys/devices/system/cpu/cpuX/online
doesn't exist: online, not offlinable
	    1: online
	    0: offline (meaning present)

If you want to get information like you have above, it's pretty trivial
to write a little script to do it.  

> the reason this cannot be determined at early time, is say if a set of cpu's are
> in a special mode, i.e only those 4 cpu's can retrieve a set of platform registers.
> for error processing. And i can offline each of those, until i run into the last 
> of those 4 cpus. 

Are you saying that you don't know if a CPU can be offlined at the time
that the sysfs files are created?  If so, I bet we could delay the
creation of the kobjects until a more appropriate time.  I don't think
anything in early boot actually needs them.

> I would prefer to not have a macro to preventy its creation, but have a 
> facility to know the offlineable status via a single information file, and 
> let __cpu_disable() determine if offline should fail based on current ability 
> to offline a cpu.
> 
> what do you think?

I just worry that any single information file doesn't fit in well with
the sysfs model.  

-- Dave

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Ashok Raj]

On Fri, Jun 04, 2004 at 04:04:17PM -0700, Dave Hansen wrote:

Dave,

I think your solution can be easily solved if all you want to do is
"your platform does not support cpuhotplug", which case just dont create 
for any cpu, or even better dont do anything in register_cpu() related
to control file creation. Forcing all implementations to behave one way just to 
do what you want seems not right approach, and limiting all to least common
denominator.

The preferable way to this would be.

- platfform_supports_cpuhotplug(), like in your case, dont create the 
  online file for any.
- __cpu_is_hotpluggable(cpu#) can also exist if this is a static decision to 
   be made, say if the platform says that a certain cpu cannot be removed. 

But you should not always assume this is a static decision just because its the
case in one platform. In my earlier example, if you have 8 cpu system, 
but cpu0-3 are special, and atleast 1 needs to exist for the system to work.

I can remove any of of cpu0-3 until i have atleast one  of the cpu0-3 left. The
last one is not removable.

Keep it simple please:-)

Cheers,
ashok
> On Fri, 2004-06-04 at 15:49, Ashok Raj wrote:
> > place a file in /sys/devices/system/cpuinfo, which when one does a cat on
> > can display
> > 
> > cpu0:	online, not offlinable
> > cpu1:	online
> > cpu2:	offline (meaning present)
> 
> This might be a little bit more than is intended for a sysfs file,
> especially if the information can be obtained some other way.  Here's
> what I was thinking of:
> 
> cat /sys/devices/system/cpu/cpuX/online
> doesn't exist: online, not offlinable
> 	    1: online
> 	    0: offline (meaning present)
> 
> If you want to get information like you have above, it's pretty trivial
> to write a little script to do it.  
> 
> > the reason this cannot be determined at early time, is say if a set of cpu's are
> > in a special mode, i.e only those 4 cpu's can retrieve a set of platform registers.
> > for error processing. And i can offline each of those, until i run into the last 
> > of those 4 cpus. 
> 
> Are you saying that you don't know if a CPU can be offlined at the time
> that the sysfs files are created?  If so, I bet we could delay the
> creation of the kobjects until a more appropriate time.  I don't think
> anything in early boot actually needs them.
> 
> > I would prefer to not have a macro to preventy its creation, but have a 
> > facility to know the offlineable status via a single information file, and 
> > let __cpu_disable() determine if offline should fail based on current ability 
> > to offline a cpu.
> > 
> > what do you think?
> 
> I just worry that any single information file doesn't fit in well with
> the sysfs model.  
> 
> -- Dave
> 

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Dave Hansen]

On Fri, 2004-06-04 at 16:17, Ashok Raj wrote:
> I think your solution can be easily solved if all you want to do is
> "your platform does not support cpuhotplug", which case just dont create 
> for any cpu, or even better dont do anything in register_cpu() related
> to control file creation. Forcing all implementations to behave one way just to 
> do what you want seems not right approach, and limiting all to least common
> denominator.

That's originally what I did.  A single global function to see if the
platform supported cpu hotplug at runtime.  The addition of passing the
'struct cpu' to it was so trivial that I figured it might be useful to
someone down the line.  I'm regretting my "foresight" now :)

> The preferable way to this would be.
> 
> - platfform_supports_cpuhotplug(), like in your case, dont create the 
>   online file for any.
> - __cpu_is_hotpluggable(cpu#) can also exist if this is a static decision to 
>    be made, say if the platform says that a certain cpu cannot be removed. 

That seems to be a pretty sensible way to do it.  However, we keep the
number of interfaces from generic to arch code down if we keep the
interface confined to 1 function.  It would be trivial to make any
architecture that needs it do this:

int __cpu_is_hotpluggable(struct cpu *cpu)
{
	if (!platfform_supports_cpuhotplug())
		return 0;

	lots();
	of_complex_arch_code();
	here();
	...
}

That ensures that there's only 1 function that needs to be defined
globally: __cpu_is_hotpluggable().

But, I definitely see the merit in what you want.

> But you should not always assume this is a static decision just because its the
> case in one platform. In my earlier example, if you have 8 cpu system, 
> but cpu0-3 are special, and atleast 1 needs to exist for the system to work.
> 
> I can remove any of of cpu0-3 until i have atleast one  of the cpu0-3 left. The
> last one is not removable.
> 
> Keep it simple please:-)

That's what I'm trying to to :)

I was thinking that cpuX/online is only there to say whether hotplug
_operations_ are supported on the cpu, not if it can be hotplugged right
now.  The "can currently be hotplugged" question is another can of worms
that can't really be answered until the hotplug request is made anyway,
so I'd prefer to keep from trying to decide that by the presence of the
file.

The purpose of the patch is originally only to keep ppc64 systems from
making firmware calls that they couldn't support.  (Someone tried to
offline a CPU on a system when it wasn't supported and the firmware got
mad)

-- Dave

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Ashok Raj]

On Fri, Jun 04, 2004 at 04:41:05PM -0700, Dave Hansen wrote:
> 
> That's originally what I did.  A single global function to see if the
> platform supported cpu hotplug at runtime.  The addition of passing the
> 'struct cpu' to it was so trivial that I figured it might be useful to
> someone down the line.  I'm regretting my "foresight" now :)
> 
> > The preferable way to this would be.
> > 
> > - platfform_supports_cpuhotplug(), like in your case, dont create the 
> >   online file for any.
> > - __cpu_is_hotpluggable(cpu#) can also exist if this is a static decision to 
> >    be made, say if the platform says that a certain cpu cannot be removed. 
> 
> That seems to be a pretty sensible way to do it.  However, we keep the
> number of interfaces from generic to arch code down if we keep the
> interface confined to 1 function.  It would be trivial to make any
> architecture that needs it do this:
> 
> int __cpu_is_hotpluggable(struct cpu *cpu)
> {
> 	if (!platfform_supports_cpuhotplug())
> 		return 0;
> 
> 	lots();
> 	of_complex_arch_code();
> 	here();
> 	...
> }
> 
> That ensures that there's only 1 function that needs to be defined
> globally: __cpu_is_hotpluggable().
> 

I feel the __cpu_disable() should be just sufficient to be the only
function interface from generic to arch code. You run this
__cpu_is_hotpluggable(cpu) only in ppc64, where you check and return error.
maybe also printing to console saying the platform doesnt support it.

you are adding an extra arch function just for a trivial thing, not to create a 
control file. 

> > 
> > Keep it simple please:-)
> 
> That's what I'm trying to to :)
> 
> I was thinking that cpuX/online is only there to say whether hotplug
> _operations_ are supported on the cpu, not if it can be hotplugged right
> now.  The "can currently be hotplugged" question is another can of worms
> that can't really be answered until the hotplug request is made anyway,
> so I'd prefer to keep from trying to decide that by the presence of the
> file.
My recommendation is to not do anything, and just let __cpu_disable() handle it.
print some verbose message for the operator that this aint going to work should
be more than sufficient. There is not a whole lot of realusefullness for this 
to work.

we if overdo something here, then memory hotplug, node hotplug would all need
to do the same hack, __is_node_hotpluggable(node), is_memsection_hotpluggabe(mem)
and so on....

Cheers,
ashok

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Dave Hansen]

On Sat, 2004-06-05 at 07:38, Ashok Raj wrote:
> I feel the __cpu_disable() should be just sufficient to be the only
> function interface from generic to arch code. You run this
> __cpu_is_hotpluggable(cpu) only in ppc64, where you check and return error.
> maybe also printing to console saying the platform doesnt support it.

Actually __cpu_is_hotpluggable(cpu) does get called on ia64, it's just a
trivially-defined 'return 1' for now.  Are there ever any plans to run
an kernel with CONFIG_HOTPLUG_CPU on an ia64 machine that doesn't really
support cpu hotplug?  If so, I'd be happy to include the same
functionality on ia64 that I put for ppc64.

BTW, the reason that this is done on ppc64 is that we can run the same
kernel on a wide variety of hardware, so it makes the distributions'
jobs a bit easier.  

> you are adding an extra arch function just for a trivial thing, not to create a 
> control file. 
...
> My recommendation is to not do anything, and just let __cpu_disable() handle it.
> print some verbose message for the operator that this aint going to work should
> be more than sufficient. There is not a whole lot of realusefullness for this 
> to work.

The non-trivial thing that this patch tries to do is give the user some
knowledge about the system from the pure layout of sysfs.  Waiting until
__cpu_disable() to tell the user that there was no possibility of the
cpu being offlined seems a bit late in the process.  Your idea about the
cpuinfo file in sysfs is definitely right; it has *exactly* the
information that I'm trying to present.  But, the current sysfs
guidelines tend to discourage single files with lots of information like
those in /proc.  

-- Dave

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Ashok Raj]

[-- Attachment #1: Type: text/plain, Size: 963 bytes --]

On Sat, Jun 05, 2004 at 12:22:21PM -0700, Dave Hansen wrote:
> knowledge about the system from the pure layout of sysfs.  Waiting until
> __cpu_disable() to tell the user that there was no possibility of the
> cpu being offlined seems a bit late in the process.  Your idea about the
> cpuinfo file in sysfs is definitely right; it has *exactly* the
> information that I'm trying to present.  But, the current sysfs
> guidelines tend to discourage single files with lots of information like
> those in /proc.  

How does the attached patch look? I would try to keep away from proliferation of
common->arch->platform code as little as possible. What i have done is
send a hint for suppressing the control file creation based on what was
set in the struct cpu, before calling register_cpu() by the arch specific 
topology_init() functions. No new __arch/__platform functions.

here is the untested patch for PPC64, does this seem to do what you need?

Cheers,
ashok

[-- Attachment #2: p00001_cpu_control_file.patch --]
[-- Type: text/plain, Size: 2235 bytes --]



D: This file provides ability for caller of register_cpu() to either create
D: a control file, or not. This can be handy if a particular platform decides
D: that certain CPU's are not removable. Hence would like to not create
D: a control file.


---

 linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c |   10 ++++++++++
 linux-2.6.7-rc2-root/drivers/base/cpu.c        |    2 +-
 linux-2.6.7-rc2-root/include/linux/cpu.h       |    1 +
 3 files changed, 12 insertions(+), 1 deletion(-)

diff -puN include/linux/cpu.h~cpu_control_file include/linux/cpu.h
--- linux-2.6.7-rc2/include/linux/cpu.h~cpu_control_file	2004-06-06 12:54:02.319017387 -0700
+++ linux-2.6.7-rc2-root/include/linux/cpu.h	2004-06-06 12:56:01.663805054 -0700
@@ -27,6 +27,7 @@
 
 struct cpu {
 	int node_id;		/* The node which contains the CPU */
+	int not_removable;	/* This cpu is not removable */
 	struct sys_device sysdev;
 };
 
diff -puN drivers/base/cpu.c~cpu_control_file drivers/base/cpu.c
--- linux-2.6.7-rc2/drivers/base/cpu.c~cpu_control_file	2004-06-06 12:56:47.349375320 -0700
+++ linux-2.6.7-rc2-root/drivers/base/cpu.c	2004-06-06 12:58:12.390434486 -0700
@@ -75,7 +75,7 @@ int __init register_cpu(struct cpu *cpu,
 		error = sysfs_create_link(&root->sysdev.kobj,
 					  &cpu->sysdev.kobj,
 					  kobject_name(&cpu->sysdev.kobj));
-	if (!error)
+	if (!error && !cpu->not_removable)
 		register_cpu_control(cpu);
 	return error;
 }
diff -puN arch/ppc64/kernel/sysfs.c~cpu_control_file arch/ppc64/kernel/sysfs.c
--- linux-2.6.7-rc2/arch/ppc64/kernel/sysfs.c~cpu_control_file	2004-06-06 13:12:06.467033408 -0700
+++ linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c	2004-06-06 13:12:43.805919713 -0700
@@ -325,6 +325,16 @@ static int __init topology_init(void)
 #ifdef CONFIG_NUMA
 		parent = &node_devices[cpu_to_node(cpu)];
 #endif
+		/*
+		 * For now, we just see if the system supports making
+		 * the RTAS calls for CPU hotplug.  But, there may be a
+		 * more comprehensive way to do this for an individual
+		 * CPU.  For instance, the boot cpu might never be valid
+		 * for hotplugging.
+		 */
+		if (systemcfg->platform == PLATFORM_PSERIES_LPAR)
+			cpu->not_removable=1;
+
 		register_cpu(c, cpu, parent);
 
 		register_cpu_pmc(&c->sysdev);

_

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

On Sun, 2004-06-06 at 13:27, Ashok Raj wrote:
> On Sat, Jun 05, 2004 at 12:22:21PM -0700, Dave Hansen wrote:
> > knowledge about the system from the pure layout of sysfs.  Waiting until
> > __cpu_disable() to tell the user that there was no possibility of the
> > cpu being offlined seems a bit late in the process.  Your idea about the
> > cpuinfo file in sysfs is definitely right; it has *exactly* the
> > information that I'm trying to present.  But, the current sysfs
> > guidelines tend to discourage single files with lots of information like
> > those in /proc.  
> 
> How does the attached patch look? I would try to keep away from proliferation of
> common->arch->platform code as little as possible. What i have done is
> send a hint for suppressing the control file creation based on what was
> set in the struct cpu, before calling register_cpu() by the arch specific 
> topology_init() functions. No new __arch/__platform functions.
> 
> here is the untested patch for PPC64, does this seem to do what you need?

I like that patch a lot.  It certainly removes any argument about
function names :)

Can we maybe change the name of the new field a bit?

-- Dave

[-- Attachment #2: cpuhotplug-online-2.6.7-rc2-mm2-5.patch --]
[-- Type: text/x-patch, Size: 1871 bytes --]

Signed-off-by: Dave Hansen <haveblue@us.ibm.com>

diff -urp linux-2.6.7-rc2-mm2-clean/arch/ppc64/kernel/sysfs.c linux-2.6.7-rc2-mm2-cpuonline2/arch/ppc64/kernel/sysfs.c
--- linux-2.6.7-rc2-mm2-clean/arch/ppc64/kernel/sysfs.c	Fri Jun  4 13:27:13 2004
+++ linux-2.6.7-rc2-mm2-cpuonline2/arch/ppc64/kernel/sysfs.c	Sun Jun  6 21:58:55 2004
@@ -325,6 +325,16 @@ static int __init topology_init(void)
 #ifdef CONFIG_NUMA
 		parent = &node_devices[cpu_to_node(cpu)];
 #endif
+		/*
+		 * For now, we just see if the system supports making
+		 * the RTAS calls for CPU hotplug.  But, there may be a
+		 * more comprehensive way to do this for an individual
+		 * CPU.  For instance, the boot cpu might never be valid
+		 * for hotplugging.
+		 */
+		if (systemcfg->platform == PLATFORM_PSERIES_LPAR)
+			cpu->can_control = 1;
+
 		register_cpu(c, cpu, parent);
 
 		register_cpu_pmc(&c->sysdev);
diff -urp linux-2.6.7-rc2-mm2-clean/drivers/base/cpu.c linux-2.6.7-rc2-mm2-cpuonline2/drivers/base/cpu.c
--- linux-2.6.7-rc2-mm2-clean/drivers/base/cpu.c	Fri Jun  4 13:27:09 2004
+++ linux-2.6.7-rc2-mm2-cpuonline2/drivers/base/cpu.c	Sun Jun  6 21:59:35 2004
@@ -75,7 +75,7 @@ int __init register_cpu(struct cpu *cpu,
 		error = sysfs_create_link(&root->sysdev.kobj,
 					  &cpu->sysdev.kobj,
 					  kobject_name(&cpu->sysdev.kobj));
-	if (!error)
+	if (!error && !cpu->can_control)
 		register_cpu_control(cpu);
 	return error;
 }
diff -urp linux-2.6.7-rc2-mm2-clean/include/linux/cpu.h linux-2.6.7-rc2-mm2-cpuonline2/include/linux/cpu.h
--- linux-2.6.7-rc2-mm2-clean/include/linux/cpu.h	Fri Jun  4 13:27:11 2004
+++ linux-2.6.7-rc2-mm2-cpuonline2/include/linux/cpu.h	Sun Jun  6 21:58:35 2004
@@ -27,6 +27,7 @@
 
 struct cpu {
 	int node_id;		/* The node which contains the CPU */
+	int can_control;	/* Should the sysfs control file be created? */
 	struct sys_device sysdev;
 };
 

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Nathan Lynch]

Dave Hansen wrote:
> diff -urp linux-2.6.7-rc2-mm2-clean/drivers/base/cpu.c linux-2.6.7-rc2-mm2-cpuonline2/drivers/base/cpu.c
> --- linux-2.6.7-rc2-mm2-clean/drivers/base/cpu.c	Fri Jun  4 13:27:09 2004
> +++ linux-2.6.7-rc2-mm2-cpuonline2/drivers/base/cpu.c	Sun Jun  6 21:59:35 2004
> @@ -75,7 +75,7 @@ int __init register_cpu(struct cpu *cpu,
>  		error = sysfs_create_link(&root->sysdev.kobj,
>  					  &cpu->sysdev.kobj,
>  					  kobject_name(&cpu->sysdev.kobj));
> -	if (!error)
> +	if (!error && !cpu->can_control)

Should be:
+	if (!error && cpu->can_control)

Right?


> diff -urp linux-2.6.7-rc2-mm2-clean/include/linux/cpu.h linux-2.6.7-rc2-mm2-cpuonline2/include/linux/cpu.h
> --- linux-2.6.7-rc2-mm2-clean/include/linux/cpu.h	Fri Jun  4 13:27:11 2004
> +++ linux-2.6.7-rc2-mm2-cpuonline2/include/linux/cpu.h	Sun Jun  6 21:58:35 2004
> @@ -27,6 +27,7 @@
>  
>  struct cpu {
>  	int node_id;		/* The node which contains the CPU */
> +	int can_control;	/* Should the sysfs control file be created? */

Minor nit -- could we change this comment to "Could this cpu ever be 
hotpluggable?"  Or just name the field "hotpluggable"?  That would 
better document the intent here, wouldn't it?


Nathan

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Ashok Raj]

On Sun, Jun 06, 2004 at 10:05:35PM -0700, Dave Hansen wrote:
> I like that patch a lot.  It certainly removes any argument about
> function names :)
> 
> Can we maybe change the name of the new field a bit?

I strategically (?) changed the name to not_removable, so the field name
can indicate the purpose as well clearly. Oh well, i changed the field name
to no_control, to indicate "not to create" a control file. the "can_remove"
when i read it in isolation, seems to indicate "this cpu is removable" and
for correcness i think we would change the normal case to set as 1 instead.

picky picky picky :-)

i added a note in the function doc as well for clarity.



Signed-off-by: Ashok Raj <ashok.raj@intel.com>

D: This file provides ability for caller of register_cpu() to either create
D: a control file, or not. This can be handy if a particular platform decides
D: that certain CPU's are not removable. Hence would like to not create
D: a control file.


---

 linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c |   10 ++++++++++
 linux-2.6.7-rc2-root/drivers/base/cpu.c        |    4 +++-
 linux-2.6.7-rc2-root/include/linux/cpu.h       |    1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff -puN include/linux/cpu.h~cpu_control_file include/linux/cpu.h
--- linux-2.6.7-rc2/include/linux/cpu.h~cpu_control_file	2004-06-06 12:54:02.319017387 -0700
+++ linux-2.6.7-rc2-root/include/linux/cpu.h	2004-06-07 06:25:44.667732023 -0700
@@ -27,6 +27,7 @@
 
 struct cpu {
 	int node_id;		/* The node which contains the CPU */
+	int no_control;		/* Should the sysfs control file be created? */
 	struct sys_device sysdev;
 };
 
diff -puN drivers/base/cpu.c~cpu_control_file drivers/base/cpu.c
--- linux-2.6.7-rc2/drivers/base/cpu.c~cpu_control_file	2004-06-06 12:56:47.349375320 -0700
+++ linux-2.6.7-rc2-root/drivers/base/cpu.c	2004-06-07 07:03:12.624937664 -0700
@@ -58,6 +58,8 @@ static inline void register_cpu_control(
 
 /*
  * register_cpu - Setup a driverfs device for a CPU.
+ * @cpu - Callers can set the cpu->no_control field to 1, to indicate not to
+ *		  generate a control file in sysfs for this CPU.
  * @num - CPU number to use when creating the device.
  *
  * Initialize and register the CPU device.
@@ -75,7 +77,7 @@ int __init register_cpu(struct cpu *cpu,
 		error = sysfs_create_link(&root->sysdev.kobj,
 					  &cpu->sysdev.kobj,
 					  kobject_name(&cpu->sysdev.kobj));
-	if (!error)
+	if (!error && !cpu->no_control)
 		register_cpu_control(cpu);
 	return error;
 }
diff -puN arch/ppc64/kernel/sysfs.c~cpu_control_file arch/ppc64/kernel/sysfs.c
--- linux-2.6.7-rc2/arch/ppc64/kernel/sysfs.c~cpu_control_file	2004-06-06 13:12:06.467033408 -0700
+++ linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c	2004-06-07 06:29:35.126834393 -0700
@@ -325,6 +325,16 @@ static int __init topology_init(void)
 #ifdef CONFIG_NUMA
 		parent = &node_devices[cpu_to_node(cpu)];
 #endif
+		/*
+		 * For now, we just see if the system supports making
+		 * the RTAS calls for CPU hotplug.  But, there may be a
+		 * more comprehensive way to do this for an individual
+		 * CPU.  For instance, the boot cpu might never be valid
+		 * for hotplugging.
+		 */
+		if (systemcfg->platform = PLATFORM_PSERIES_LPAR)
+			cpu->no_control=1;
+
 		register_cpu(c, cpu, parent);
 
 		register_cpu_pmc(&c->sysdev);

_

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Ashok Raj]

On Mon, Jun 07, 2004 at 09:07:38AM -0500, Nathan Lynch wrote:
> Dave Hansen wrote:
> > -	if (!error)
> > +	if (!error && !cpu->can_control)
> 
> Should be:
> +	if (!error && cpu->can_control)
> 

Glad someone else also read it the same way as i did 

the name is now fixed in the new patch posted.... you caught me just 
few mins before.

Cheers,
ashok

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Dave Hansen]

On Mon, 2004-06-07 at 07:08, Ashok Raj wrote:plugging.
> +		if (systemcfg->platform = PLATFORM_PSERIES_LPAR)
> +			cpu->no_control=1;

That condition is also inverted.  Should read:

if (systemcfg->platform != PLATFORM_PSERIES_LPAR)
	cpu->no_control = 1;



-- Dave

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Ashok Raj]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

On Mon, Jun 07, 2004 at 09:41:49AM -0700, Dave Hansen wrote:
> On Mon, 2004-06-07 at 07:08, Ashok Raj wrote:plugging.
> > +		if (systemcfg->platform == PLATFORM_PSERIES_LPAR)
> > +			cpu->no_control=1;
> 
> That condition is also inverted.  Should read:
> 
> if (systemcfg->platform != PLATFORM_PSERIES_LPAR)
> 	cpu->no_control = 1;

Good catch...I knew i mentioned untested :-)

fixed patch attached...

Cheers,
ashok

[-- Attachment #2: p00001_cpu_control_file.patch --]
[-- Type: text/plain, Size: 2646 bytes --]


Signed-off-by: Ashok Raj <ashok.raj@intel.com>

D: This file provides ability for caller of register_cpu() to either create
D: a control file, or not. This can be handy if a particular platform decides
D: that certain CPU's are not removable. Hence would like to not create
D: a control file.


---

 linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c |   10 ++++++++++
 linux-2.6.7-rc2-root/drivers/base/cpu.c        |    4 +++-
 linux-2.6.7-rc2-root/include/linux/cpu.h       |    1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff -puN include/linux/cpu.h~cpu_control_file include/linux/cpu.h
--- linux-2.6.7-rc2/include/linux/cpu.h~cpu_control_file	2004-06-06 12:54:02.319017387 -0700
+++ linux-2.6.7-rc2-root/include/linux/cpu.h	2004-06-07 06:25:44.667732023 -0700
@@ -27,6 +27,7 @@
 
 struct cpu {
 	int node_id;		/* The node which contains the CPU */
+	int no_control;		/* Should the sysfs control file be created? */
 	struct sys_device sysdev;
 };
 
diff -puN drivers/base/cpu.c~cpu_control_file drivers/base/cpu.c
--- linux-2.6.7-rc2/drivers/base/cpu.c~cpu_control_file	2004-06-06 12:56:47.349375320 -0700
+++ linux-2.6.7-rc2-root/drivers/base/cpu.c	2004-06-07 07:03:12.624937664 -0700
@@ -58,6 +58,8 @@ static inline void register_cpu_control(
 
 /*
  * register_cpu - Setup a driverfs device for a CPU.
+ * @cpu - Callers can set the cpu->no_control field to 1, to indicate not to
+ *		  generate a control file in sysfs for this CPU.
  * @num - CPU number to use when creating the device.
  *
  * Initialize and register the CPU device.
@@ -75,7 +77,7 @@ int __init register_cpu(struct cpu *cpu,
 		error = sysfs_create_link(&root->sysdev.kobj,
 					  &cpu->sysdev.kobj,
 					  kobject_name(&cpu->sysdev.kobj));
-	if (!error)
+	if (!error && !cpu->no_control)
 		register_cpu_control(cpu);
 	return error;
 }
diff -puN arch/ppc64/kernel/sysfs.c~cpu_control_file arch/ppc64/kernel/sysfs.c
--- linux-2.6.7-rc2/arch/ppc64/kernel/sysfs.c~cpu_control_file	2004-06-06 13:12:06.467033408 -0700
+++ linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c	2004-06-07 10:12:45.999901454 -0700
@@ -325,6 +325,16 @@ static int __init topology_init(void)
 #ifdef CONFIG_NUMA
 		parent = &node_devices[cpu_to_node(cpu)];
 #endif
+		/*
+		 * For now, we just see if the system supports making
+		 * the RTAS calls for CPU hotplug.  But, there may be a
+		 * more comprehensive way to do this for an individual
+		 * CPU.  For instance, the boot cpu might never be valid
+		 * for hotplugging.
+		 */
+		if (systemcfg->platform != PLATFORM_PSERIES_LPAR)
+			cpu->no_control=1;
+
 		register_cpu(c, cpu, parent);
 
 		register_cpu_pmc(&c->sysdev);

_

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

On Mon, 2004-06-07 at 10:22, Ashok Raj wrote:
> On Mon, Jun 07, 2004 at 09:41:49AM -0700, Dave Hansen wrote:
> > On Mon, 2004-06-07 at 07:08, Ashok Raj wrote:plugging.
> > > +		if (systemcfg->platform == PLATFORM_PSERIES_LPAR)
> > > +			cpu->no_control=1;
> > 
> > That condition is also inverted.  Should read:
> > 
> > if (systemcfg->platform != PLATFORM_PSERIES_LPAR)
> > 	cpu->no_control = 1;
> 
> Good catch...I knew i mentioned untested :-)
> 
> fixed patch attached...

The smallest of nitpicks:

	cpu->no_control=1;

needs to be

	cpu->no_control = 1;

Would you like to submit this now?

-- Dave

[-- Attachment #2: p00002_cpu_control_file.patch --]
[-- Type: text/x-patch, Size: 2648 bytes --]


Signed-off-by: Ashok Raj <ashok.raj@intel.com>

D: This file provides ability for caller of register_cpu() to either create
D: a control file, or not. This can be handy if a particular platform decides
D: that certain CPU's are not removable. Hence would like to not create
D: a control file.


---

 linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c |   10 ++++++++++
 linux-2.6.7-rc2-root/drivers/base/cpu.c        |    4 +++-
 linux-2.6.7-rc2-root/include/linux/cpu.h       |    1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff -puN include/linux/cpu.h~cpu_control_file include/linux/cpu.h
--- linux-2.6.7-rc2/include/linux/cpu.h~cpu_control_file	2004-06-06 12:54:02.319017387 -0700
+++ linux-2.6.7-rc2-root/include/linux/cpu.h	2004-06-07 06:25:44.667732023 -0700
@@ -27,6 +27,7 @@
 
 struct cpu {
 	int node_id;		/* The node which contains the CPU */
+	int no_control;		/* Should the sysfs control file be created? */
 	struct sys_device sysdev;
 };
 
diff -puN drivers/base/cpu.c~cpu_control_file drivers/base/cpu.c
--- linux-2.6.7-rc2/drivers/base/cpu.c~cpu_control_file	2004-06-06 12:56:47.349375320 -0700
+++ linux-2.6.7-rc2-root/drivers/base/cpu.c	2004-06-07 07:03:12.624937664 -0700
@@ -58,6 +58,8 @@ static inline void register_cpu_control(
 
 /*
  * register_cpu - Setup a driverfs device for a CPU.
+ * @cpu - Callers can set the cpu->no_control field to 1, to indicate not to
+ *		  generate a control file in sysfs for this CPU.
  * @num - CPU number to use when creating the device.
  *
  * Initialize and register the CPU device.
@@ -75,7 +77,7 @@ int __init register_cpu(struct cpu *cpu,
 		error = sysfs_create_link(&root->sysdev.kobj,
 					  &cpu->sysdev.kobj,
 					  kobject_name(&cpu->sysdev.kobj));
-	if (!error)
+	if (!error && !cpu->no_control)
 		register_cpu_control(cpu);
 	return error;
 }
diff -puN arch/ppc64/kernel/sysfs.c~cpu_control_file arch/ppc64/kernel/sysfs.c
--- linux-2.6.7-rc2/arch/ppc64/kernel/sysfs.c~cpu_control_file	2004-06-06 13:12:06.467033408 -0700
+++ linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c	2004-06-07 10:12:45.999901454 -0700
@@ -325,6 +325,16 @@ static int __init topology_init(void)
 #ifdef CONFIG_NUMA
 		parent = &node_devices[cpu_to_node(cpu)];
 #endif
+		/*
+		 * For now, we just see if the system supports making
+		 * the RTAS calls for CPU hotplug.  But, there may be a
+		 * more comprehensive way to do this for an individual
+		 * CPU.  For instance, the boot cpu might never be valid
+		 * for hotplugging.
+		 */
+		if (systemcfg->platform != PLATFORM_PSERIES_LPAR)
+			cpu->no_control = 1;
+
 		register_cpu(c, cpu, parent);
 
 		register_cpu_pmc(&c->sysdev);

_

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Ashok Raj]

On Mon, Jun 07, 2004 at 12:25:57PM -0700, Dave Hansen wrote:
> 
> 	cpu->no_control = 1;
> 
> Would you like to submit this now?
> 
Sounds good.

Cheers,
ashok

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Andrew Morton]

Ashok Raj <ashok.raj@intel.com> wrote:
>
> On Mon, Jun 07, 2004 at 09:41:49AM -0700, Dave Hansen wrote:
>  > On Mon, 2004-06-07 at 07:08, Ashok Raj wrote:plugging.
>  > > +		if (systemcfg->platform = PLATFORM_PSERIES_LPAR)
>  > > +			cpu->no_control=1;
>  > 
>  > That condition is also inverted.  Should read:
>  > 
>  > if (systemcfg->platform != PLATFORM_PSERIES_LPAR)
>  > 	cpu->no_control = 1;
> 
>  Good catch...I knew i mentioned untested :-)
> 
>  fixed patch attached...
> 
> ...
>  --- linux-2.6.7-rc2/arch/ppc64/kernel/sysfs.c~cpu_control_file	2004-06-06 13:12:06.467033408 -0700
>  +++ linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c	2004-06-07 10:12:45.999901454 -0700
>  @@ -325,6 +325,16 @@ static int __init topology_init(void)
>   #ifdef CONFIG_NUMA
>   		parent = &node_devices[cpu_to_node(cpu)];
>   #endif
>  +		/*
>  +		 * For now, we just see if the system supports making
>  +		 * the RTAS calls for CPU hotplug.  But, there may be a
>  +		 * more comprehensive way to do this for an individual
>  +		 * CPU.  For instance, the boot cpu might never be valid
>  +		 * for hotplugging.
>  +		 */
>  +		if (systemcfg->platform != PLATFORM_PSERIES_LPAR)
>  +			cpu->no_control=1;
>  +
>   		register_cpu(c, cpu, parent);
>   
>   		register_cpu_pmc(&c->sysdev);
> 

The ppc64 part doesn't compile - `cpu' is an integer.

Re: [lhcs-devel] Re: [RFC] don't create cpu/online sysfs file

[Ashok Raj]

On Wed, Jun 09, 2004 at 12:10:36AM -0700, Andrew Morton wrote:
> Ashok Raj <ashok.raj@intel.com> wrote:
> >
> > On Mon, Jun 07, 2004 at 09:41:49AM -0700, Dave Hansen wrote:
> >  > On Mon, 2004-06-07 at 07:08, Ashok Raj wrote:plugging.
> >  > > +		if (systemcfg->platform = PLATFORM_PSERIES_LPAR)
> >  > > +			cpu->no_control=1;
> >  > 
> The ppc64 part doesn't compile - `cpu' is an integer.

Sorry about that... I dont have a PPC64 platform....

ashokr



Signed-off-by: Ashok Raj <ashok.raj@intel.com>

D: This file provides ability for caller of register_cpu() to either create
D: a control file, or not. This can be handy if a particular platform decides
D: that certain CPU's are not removable. Hence would like to not create
D: a control file.


---

 linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c |   10 ++++++++++
 linux-2.6.7-rc2-root/drivers/base/cpu.c        |    4 +++-
 linux-2.6.7-rc2-root/include/linux/cpu.h       |    1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff -puN include/linux/cpu.h~cpu_control_file include/linux/cpu.h
--- linux-2.6.7-rc2/include/linux/cpu.h~cpu_control_file	2004-06-06 12:54:02.000000000 -0700
+++ linux-2.6.7-rc2-root/include/linux/cpu.h	2004-06-07 06:25:44.000000000 -0700
@@ -27,6 +27,7 @@
 
 struct cpu {
 	int node_id;		/* The node which contains the CPU */
+	int no_control;		/* Should the sysfs control file be created? */
 	struct sys_device sysdev;
 };
 
diff -puN drivers/base/cpu.c~cpu_control_file drivers/base/cpu.c
--- linux-2.6.7-rc2/drivers/base/cpu.c~cpu_control_file	2004-06-06 12:56:47.000000000 -0700
+++ linux-2.6.7-rc2-root/drivers/base/cpu.c	2004-06-07 07:03:12.000000000 -0700
@@ -58,6 +58,8 @@ static inline void register_cpu_control(
 
 /*
  * register_cpu - Setup a driverfs device for a CPU.
+ * @cpu - Callers can set the cpu->no_control field to 1, to indicate not to
+ *		  generate a control file in sysfs for this CPU.
  * @num - CPU number to use when creating the device.
  *
  * Initialize and register the CPU device.
@@ -75,7 +77,7 @@ int __init register_cpu(struct cpu *cpu,
 		error = sysfs_create_link(&root->sysdev.kobj,
 					  &cpu->sysdev.kobj,
 					  kobject_name(&cpu->sysdev.kobj));
-	if (!error)
+	if (!error && !cpu->no_control)
 		register_cpu_control(cpu);
 	return error;
 }
diff -puN arch/ppc64/kernel/sysfs.c~cpu_control_file arch/ppc64/kernel/sysfs.c
--- linux-2.6.7-rc2/arch/ppc64/kernel/sysfs.c~cpu_control_file	2004-06-06 13:12:06.000000000 -0700
+++ linux-2.6.7-rc2-root/arch/ppc64/kernel/sysfs.c	2004-06-09 07:22:12.206182184 -0700
@@ -325,6 +325,16 @@ static int __init topology_init(void)
 #ifdef CONFIG_NUMA
 		parent = &node_devices[cpu_to_node(cpu)];
 #endif
+		/*
+		 * For now, we just see if the system supports making
+		 * the RTAS calls for CPU hotplug.  But, there may be a
+		 * more comprehensive way to do this for an individual
+		 * CPU.  For instance, the boot cpu might never be valid
+		 * for hotplugging.
+		 */
+		if (systemcfg->platform != PLATFORM_PSERIES_LPAR)
+			c->no_control = 1;
+
 		register_cpu(c, cpu, parent);
 
 		register_cpu_pmc(&c->sysdev);

_