Re: [Xen-staging] [xen-unstable] Added some more fields to host_cpu.

Re: [Xen-staging] [xen-unstable] Added some more fields to host_cpu.

[Alex Williamson]

Hi Ewan,

   There are a couple problems with this patch for non-x86 (and maybe
even on x86), see below:

On Tue, 2007-02-27 at 01:56 +0000, Xen staging patchbot-unstable wrote:
> diff -r e7b2a282c9e7 -r 50e0616fd012 tools/python/xen/xend/XendNode.py
> --- a/tools/python/xen/xend/XendNode.py	Mon Feb 26 17:20:36 2007 +0000
> +++ b/tools/python/xen/xend/XendNode.py	Tue Feb 27 00:37:27 2007 +0000
> @@ -81,7 +81,7 @@ class XendNode:
>          for cpu_uuid, cpu in saved_cpus.items():
>              self.cpus[cpu_uuid] = cpu
>  
> -        # verify we have enough cpus here
> +        cpuinfo = parse_proc_cpuinfo()
>          physinfo = self.physinfo_dict()
>          cpu_count = physinfo['nr_cpus']
>          cpu_features = physinfo['hw_caps']
> @@ -91,12 +91,23 @@ class XendNode:
>          if cpu_count != len(self.cpus):
>              self.cpus = {}
>              for i in range(cpu_count):
> -                cpu_uuid = uuid.createString()
> -                cpu_info = {'uuid': cpu_uuid,
> -                            'host': self.uuid,
> -                            'number': i,
> -                            'features': cpu_features}
> -                self.cpus[cpu_uuid] = cpu_info
> +                u = uuid.createString()
> +                self.cpus[u] = {'uuid': u, 'number': i }
> +
> +        for u in self.cpus.keys():
> +            log.error(self.cpus[u])
> +            number = self.cpus[u]['number']
> +            log.error(number)
> +            log.error(cpuinfo)
> +            self.cpus[u].update(
> +                { 'host'     : self.uuid,
> +                  'features' : cpu_features,
> +                  'speed'    : int(float(cpuinfo[number]['cpu MHz'])),
> +                  'vendor'   : cpuinfo[number]['vendor_id'],
> +                  'modelname': cpuinfo[number]['model name'],
> +                  'stepping' : cpuinfo[number]['stepping'],
> +                  'flags'    : cpuinfo[number]['flags'],
> +                })

   On ia64, dom0 doesn't automatically get vcpus for each physical cpu,
so the first problem is that we're not going to have a /proc/cpuinfo
entry for every cpu in self.cpus.keys.  I think it's likely x86 could
run into this problem too if a cpu was hotplugged or booted with the
dom0_max_vcpus options.

   The second problem is that /proc/cpuinfo fields are very architecture
specific.  I'd suggest importing arch and having separate cases for x86,
ia64, and powerpc.  For ia64, think the most appropriate mapping would
be:

                self.cpus[u].update(
                    { 'host'     : self.uuid,
                      'features' : cpu_features,
                      'speed'    : int(float(cpuinfo[0]['cpu MHz'])),
                      'vendor'   : cpuinfo[0]['vendor'],
                      'modelname': cpuinfo[0]['family'],
                      'stepping' : cpuinfo[0]['model'],
                      'flags'    : cpuinfo[0]['features'],
                    })

Hollis or Jimi might be able to chime in with identifiers that would
work on powerpc.  Thanks,

	Alex
-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: Re: [Xen-staging] [xen-unstable] Added some more fields to host_cpu.

[Keir Fraser]

On 2/3/07 20:39, "Alex Williamson" <alex.williamson@hp.com> wrote:

>   On ia64, dom0 doesn't automatically get vcpus for each physical cpu,
> so the first problem is that we're not going to have a /proc/cpuinfo
> entry for every cpu in self.cpus.keys.  I think it's likely x86 could
> run into this problem too if a cpu was hotplugged or booted with the
> dom0_max_vcpus options.

We have indeed hit this problem and I put a patch in this afternoon to
duplicate cpu0's info for any non-existent cpu. Given that the dom0 cpus
could be migrating around on arbitrary physical cpus (even across the
multiple CPUID invocations that the kernel will have made to build the
information for a single 'cpu' in /proc!) this is fine -- x86 multiprocessor
systems are supposed to be symmetric (homogeneous CPUs down to the same
stepping in some cases) anyway.

 -- Keir

>    The second problem is that /proc/cpuinfo fields are very architecture
> specific.  I'd suggest importing arch and having separate cases for x86,
> ia64, and powerpc.  For ia64, think the most appropriate mapping would
> be:

Re: Re: [Xen-staging] [xen-unstable] Added some more fields to host_cpu.

[John Levon]

On Fri, Mar 02, 2007 at 01:39:46PM -0700, Alex Williamson wrote:

>    The second problem is that /proc/cpuinfo fields are very architecture
> specific.  I'd suggest importing arch and having separate cases for x86,
> ia64, and powerpc.  For ia64, think the most appropriate mapping would
> be:

And that there's other OS's that don't stuff things in /proc at all.
Sigh.

john

Re: Re: [Xen-staging] [xen-unstable] Added some more fields to host_cpu.

[John Levon]

On Fri, Mar 02, 2007 at 08:47:21PM +0000, John Levon wrote:

> >    The second problem is that /proc/cpuinfo fields are very architecture
> > specific.  I'd suggest importing arch and having separate cases for x86,
> > ia64, and powerpc.  For ia64, think the most appropriate mapping would
> > be:
> 
> And that there's other OS's that don't stuff things in /proc at all.
> Sigh.

Talking of which, XendMonitor seems to re-implement libxenstat's VBD/VIF
code in Python. Couldn't we use one or the other?

regards
john

Re: Re: [Xen-staging] [xen-unstable] Added some more fields to host_cpu.

[Alex Williamson]

On Fri, 2007-03-02 at 20:46 +0000, Keir Fraser wrote:
> On 2/3/07 20:39, "Alex Williamson" <alex.williamson@hp.com> wrote:
> 
> >   On ia64, dom0 doesn't automatically get vcpus for each physical cpu,
> > so the first problem is that we're not going to have a /proc/cpuinfo
> > entry for every cpu in self.cpus.keys.  I think it's likely x86 could
> > run into this problem too if a cpu was hotplugged or booted with the
> > dom0_max_vcpus options.
> 
> We have indeed hit this problem and I put a patch in this afternoon to
> duplicate cpu0's info for any non-existent cpu. Given that the dom0 cpus
> could be migrating around on arbitrary physical cpus (even across the
> multiple CPUID invocations that the kernel will have made to build the
> information for a single 'cpu' in /proc!) this is fine -- x86 multiprocessor
> systems are supposed to be symmetric (homogeneous CPUs down to the same
> stepping in some cases) anyway.

   Cool, I see it in staging.  This is the approach I used to work
around the problem temporarily, but what happens if cpu0 is hot
un-plugged?  ISTR x86 Linux doesn't support cpu0 hotplug, but on ia64 we
can hotplug cpu0.  I'd guess powerpc could too.  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.

Re: Re: [Xen-staging] [xen-unstable] Added some more fields to host_cpu.

[Keir Fraser]

On 2/3/07 20:55, "Alex Williamson" <alex.williamson@hp.com> wrote:

>> We have indeed hit this problem and I put a patch in this afternoon to
>> duplicate cpu0's info for any non-existent cpu. Given that the dom0 cpus
>> could be migrating around on arbitrary physical cpus (even across the
>> multiple CPUID invocations that the kernel will have made to build the
>> information for a single 'cpu' in /proc!) this is fine -- x86 multiprocessor
>> systems are supposed to be symmetric (homogeneous CPUs down to the same
>> stepping in some cases) anyway.
> 
>    Cool, I see it in staging.  This is the approach I used to work
> around the problem temporarily, but what happens if cpu0 is hot
> un-plugged?  ISTR x86 Linux doesn't support cpu0 hotplug, but on ia64 we
> can hotplug cpu0.  I'd guess powerpc could too.  Thanks,

Then picking another number than zero would be a good idea. :-)

 -- Keir

Re: [Xen-staging] [xen-unstable] Added some more fields to host_cpu.

[Hollis Blanchard]

On Fri, 2007-03-02 at 13:39 -0700, Alex Williamson wrote:
> 
>    On ia64, dom0 doesn't automatically get vcpus for each physical cpu,
> so the first problem is that we're not going to have a /proc/cpuinfo
> entry for every cpu in self.cpus.keys.  I think it's likely x86 could
> run into this problem too if a cpu was hotplugged or booted with the
> dom0_max_vcpus options.

Not sure if this affects us.

>    The second problem is that /proc/cpuinfo fields are very architecture
> specific.  I'd suggest importing arch and having separate cases for x86,
> ia64, and powerpc.  For ia64, think the most appropriate mapping would
> be:
> 
>                 self.cpus[u].update(
>                     { 'host'     : self.uuid,
>                       'features' : cpu_features,
>                       'speed'    : int(float(cpuinfo[0]['cpu MHz'])),
>                       'vendor'   : cpuinfo[0]['vendor'],
>                       'modelname': cpuinfo[0]['family'],
>                       'stepping' : cpuinfo[0]['model'],
>                       'flags'    : cpuinfo[0]['features'],
>                     })

This absolutely does. Ewan, what are you trying to do here, and could
you please revert it until we figure out a solution that will work?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Xen-staging] [xen-unstable] Added some more fields to host_cpu.

[Hollis Blanchard]

Hi Keir, I see the patch you've committed for this issue
(http://xenbits2.xensource.com/staging/xen-unstable.hg?rev/ae203b55e7c8), but my concern goes a littler further.

What will this data be used for? Will higher-level tools expect to find
"stepping" and break if they don't? Will XenSource's management tools
(which I assume will make use of this data) function properly in this
case? I agree that hardware-specific data needs to be visible somehow to
higher-level tools, but it makes me nervous...

(I don't have any particular objection to your fix; just voicing my
concerns.)

On Fri, 2007-03-02 at 13:39 -0700, Alex Williamson wrote:
> Hi Ewan,
> 
>    There are a couple problems with this patch for non-x86 (and maybe
> even on x86), see below:
> 
> On Tue, 2007-02-27 at 01:56 +0000, Xen staging patchbot-unstable wrote:
> > diff -r e7b2a282c9e7 -r 50e0616fd012 tools/python/xen/xend/XendNode.py
> > --- a/tools/python/xen/xend/XendNode.py	Mon Feb 26 17:20:36 2007 +0000
> > +++ b/tools/python/xen/xend/XendNode.py	Tue Feb 27 00:37:27 2007 +0000
> > @@ -81,7 +81,7 @@ class XendNode:
> >          for cpu_uuid, cpu in saved_cpus.items():
> >              self.cpus[cpu_uuid] = cpu
> >  
> > -        # verify we have enough cpus here
> > +        cpuinfo = parse_proc_cpuinfo()
> >          physinfo = self.physinfo_dict()
> >          cpu_count = physinfo['nr_cpus']
> >          cpu_features = physinfo['hw_caps']
> > @@ -91,12 +91,23 @@ class XendNode:
> >          if cpu_count != len(self.cpus):
> >              self.cpus = {}
> >              for i in range(cpu_count):
> > -                cpu_uuid = uuid.createString()
> > -                cpu_info = {'uuid': cpu_uuid,
> > -                            'host': self.uuid,
> > -                            'number': i,
> > -                            'features': cpu_features}
> > -                self.cpus[cpu_uuid] = cpu_info
> > +                u = uuid.createString()
> > +                self.cpus[u] = {'uuid': u, 'number': i }
> > +
> > +        for u in self.cpus.keys():
> > +            log.error(self.cpus[u])
> > +            number = self.cpus[u]['number']
> > +            log.error(number)
> > +            log.error(cpuinfo)
> > +            self.cpus[u].update(
> > +                { 'host'     : self.uuid,
> > +                  'features' : cpu_features,
> > +                  'speed'    : int(float(cpuinfo[number]['cpu MHz'])),
> > +                  'vendor'   : cpuinfo[number]['vendor_id'],
> > +                  'modelname': cpuinfo[number]['model name'],
> > +                  'stepping' : cpuinfo[number]['stepping'],
> > +                  'flags'    : cpuinfo[number]['flags'],
> > +                })
> 
>    On ia64, dom0 doesn't automatically get vcpus for each physical cpu,
> so the first problem is that we're not going to have a /proc/cpuinfo
> entry for every cpu in self.cpus.keys.  I think it's likely x86 could
> run into this problem too if a cpu was hotplugged or booted with the
> dom0_max_vcpus options.
> 
>    The second problem is that /proc/cpuinfo fields are very architecture
> specific.  I'd suggest importing arch and having separate cases for x86,
> ia64, and powerpc.  For ia64, think the most appropriate mapping would
> be:
> 
>                 self.cpus[u].update(
>                     { 'host'     : self.uuid,
>                       'features' : cpu_features,
>                       'speed'    : int(float(cpuinfo[0]['cpu MHz'])),
>                       'vendor'   : cpuinfo[0]['vendor'],
>                       'modelname': cpuinfo[0]['family'],
>                       'stepping' : cpuinfo[0]['model'],
>                       'flags'    : cpuinfo[0]['features'],
>                     })
> 
> Hollis or Jimi might be able to chime in with identifiers that would
> work on powerpc.  Thanks,
> 
> 	Alex
-- 
Hollis Blanchard
IBM Linux Technology Center