Re: [PATCH v2:3/3]Export cpu topology by sysfs

[Nathan Lynch <ntl@pobox.com>]

Yanmin Zhang wrote:
> 
> Items (attributes) are similar to /proc/cpuinfo.
> 
> 1) /sys/devices/system/cpu/cpuX/topology/physical_package_id:
> represent the physical package id of  cpu X;
> 2) /sys/devices/system/cpu/cpuX/topology/core_id:
> represent the cpu core id to cpu X;
> 3) /sys/devices/system/cpu/cpuX/topology/thread_id:
> represent the cpu thread id  to cpu X;

So what is the format of the *_id attributes?  Looks like this is
determined by the architecture, which is okay, but it doesn't seem
explicit.

What about sane default values for the *_id attributes?  For example,
say I have a uniprocessor PC without HT or multicore -- should all of
these attributes have zero values, or some kind of "special" value to
mean "not applicable"?

Hmm, why should thread_id be exported at all?  Is it useful to
userspace in a way that the logical cpu id is not?

> 4) /sys/devices/system/cpu/cpuX/topology/thread_siblings:
> represent the thread siblings to cpu X in the same core;
> 5) /sys/devices/system/cpu/cpuX/topology/core_siblings:
> represent the thread siblings to cpu X in the same physical package;
> 
> If one architecture wants to support this feature, it just needs to
> implement 5 defines, typically in file include/asm-XXX/topology.h.
> The 5 defines are:
> #define topology_physical_package_id(cpu)
> #define topology_core_id(cpu)
> #define topology_thread_id(cpu)
> #define topology_thread_siblings(cpu)
> #define topology_core_siblings(cpu)
> 
> The type of siblings is cpumask_t.
> 
> If an attribute isn't defined on an architecture, it won't be
> exported.

Okay, but which combinations of attributes are valid?  E.g. I would
think that it's fine for an architecture to define topology_thread_id
and topology_thread_siblings without any of the others, correct?

Also I'd rather the architectures have the ability to define these as
functions instead of macros.

<snip>

> +/* Add/Remove cpu_topology interface for CPU device */
> +static int __cpuinit topology_add_dev(struct sys_device * sys_dev)
> +{
> +	sysfs_create_group(&sys_dev->kobj, &topology_attr_group);
> +	return 0;
> +}

Can't sysfs_create_group fail?

> +
> +static int __cpuinit topology_remove_dev(struct sys_device * sys_dev)
> +{
> +	sysfs_remove_group(&sys_dev->kobj, &topology_attr_group);
> +	return 0;
> +}
> +
> +static int __cpuinit topology_cpu_callback(struct notifier_block *nfb,
> +		unsigned long action, void *hcpu)
> +{
> +	unsigned int cpu = (unsigned long)hcpu;
> +	struct sys_device *sys_dev;
> +
> +	sys_dev = get_cpu_sysdev(cpu);
> +	switch (action) {
> +		case CPU_ONLINE:
> +			topology_add_dev(sys_dev);
> +			break;
> +		case CPU_DEAD:
> +			topology_remove_dev(sys_dev);
> +			break;
> +	}
> +	return NOTIFY_OK;
> +}

I don't think it makes much sense to add and remove the attribute
group for cpu online/offline events.  The topology information for an
offline cpu is potentially valuable -- it could help the admin decide
which processor to online at runtime, for example.

I believe the correct time to update the topology information is when
the topology actually changes (e.g. physical addition or removal of a
processor) -- this is independent of online/offline operations.

In general, I'm still a little uneasy with exporting the cpu topology
in this way.  I can see how the information would be useful, and right
now, Linux does not do a great job of exposing to userspace these
relationships between cpus.  So I see the need.  But the things about
this approach which I don't like are:

- Attributes which are not applicable to the running system will be
  exported anyway.  Discovery at runtime would be less confusing, I
  think.

- This locks us into exporting a three-level topology (thread, core,
  package), with hard-coded names, when it seems probable that there
  will be systems with more levels than that in the future.

Have you considered basing the exported topology on sched domains?


Nathan