Re: [libvirt] [Resending][PATCH v2 2/2] x86: Allow sysinfo to fall back on /proc/cpuinfo if demidecode is absent

Hi Daniel, Thank you for taking a look. On 03/14/2012 02:29 PM, Daniel Veillard wrote: > On Tue, Mar 13, 2012 at 11:10:16AM +0530, Prerna Saxena wrote: >> From: Prerna Saxena <prerna(a)linux.vnet.ibm.com&gt; >> Date: Tue, 13 Mar 2012 15:33:43 +0530 >> Subject: [PATCH 2/2] Sysinfo : Allow x86 to fetch sysinfo from >> /proc/cpuinfo in the event 'dmidecode' is absent in the system. >> >> Until now, libvirt on x86 flags an error message if dmidecode is not >> found. With this patch, the following is a sample output on x86 when >> dmidecode is absent: >> >> virsh # sysinfo >> <sysinfo type='smbios'> >> <processor> >> <entry name='socket_destination'>0</entry> >> <entry name='type'>Intel(R) Xeon(R) CPU X5570 @ 2.93GHz</entry> >> <entry name='family'>6</entry> >> <entry name='manufacturer'>GenuineIntel</entry> >> </processor> >> <processor> >> <entry name='socket_destination'>1</entry> >> <entry name='type'>Intel(R) Xeon(R) CPU X5570 @ 2.93GHz</entry> >> <entry name='family'>6</entry> >> <entry name='manufacturer'>GenuineIntel</entry> >> </processor> >> ... (listing for all online CPUs) >> </sysinfo> >> >> Based on suggestion from Eric: >> (http://www.redhat.com/archives/libvir-list/2012-February/msg00509.html) >> >> Acked-by: Daniel P Berrange <berrange(a)redhat.com&gt; >> Signed-off-by: Prerna Saxena <prerna(a)linux.vnet.ibm.com&gt; >> --- >> src/util/sysinfo.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 files changed, 93 insertions(+), 4 deletions(-) >> >> ... [snip].. > > Hi Prerna, > > that sounds like a good idea, and the patch seems to work but I have > doubt with the usefulness in its current form. Let me explain: > > with dmidecode available on my system I get: > > ... > <processor> > <entry name='socket_destination'>Socket 775</entry> > <entry name='type'>Central Processor</entry> > <entry name='family'>Other</entry> > <entry name='manufacturer'>Intel</entry> > <entry name='signature'>Type 0, Family 6, Model 15, Stepping > 11</entry> > <entry name='version'>Intel(R) Core(TM)2 Duo CPU E6550 @ > 2.33GHz</entry> > <entry name='external_clock'>333 MHz</entry> > <entry name='max_speed'>4000 MHz</entry> > <entry name='status'>Populated, Enabled</entry> > </processor> > ... > > without dmidecode and your patch plugged in I get > > <processor> > <entry name='socket_destination'>0</entry> > <entry name='type'>Intel(R) Core(TM)2 Duo CPU E6550 @ > 2.33GHz</entry> > <entry name='family'>6</entry> > <entry name='manufacturer'>GenuineIntel</entry> > </processor> > <processor> > <entry name='socket_destination'>1</entry> > <entry name='type'>Intel(R) Core(TM)2 Duo CPU E6550 @ > 2.33GHz</entry> > <entry name='family'>6</entry> > <entry name='manufacturer'>GenuineIntel</entry> > </processor> > > so basically we get informations, some are available in both case but > differently, and worse, in the fallback case we get 2 physical processor > entries (I have only one) which is of course different from the single > processor that we get with dmidecode. > > So 1/ is seems to me the fallback data can't be parsed > programmatically as a replacement of the original ones > 2/ the data may be misunderstood and lead to erroneous > decision for example a schedule may start to stack 2 time more > load on my machine based on the difference of report. > > So I'm a bit worried about applying it as-is, I'm afraid we need > to reconcile the output (as much as possible considering there > is less data) between both cases. > Thanks for pointing this out. I investigated this discrepancy, and discovered that 'dmidecode' presents a listing of processor *cores*. However, for /proc/cpuinfo, all hardware threads in a processor show up as independent processors. So, while dmidecode correctly reads that my system has a single core, /proc/cpuinfo reports two hardware threads in the core as two independent logical CPUs. To sort this out, one alternative would be to parse the physical_id in /proc/cpuinfo -- this would be identical for all logical processors in a given core, and thus can be used to report the number of cores in the system. Will send a modified patch asap. > That said I think patch 1/2 looks fine to me, and could probably be > applied as-is, > Thanks! Would you want to apply it as-is, or shall I send a rebased version ?