Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): fix issues, add fields, flags

[Ken Brown <kbrown at cornell dot edu>]

On 10/5/2019 5:42 PM, Brian Inglis wrote:
> On 2019-10-05 15:06, Ken Brown wrote:
>> On 10/4/2019 6:44 AM, Brian Inglis wrote:
>>> fix cache size return code handling and make AMD/Intel code common;
>>> fix cpuid level count as number of non-zero leafs excluding sub-leafs;
>>> fix AMD physical cores count to be documented nc + 1;
>>> round cpu MHz to correct Windows and match Linux cpuinfo;
>>> add microcode from Windows registry Update Revision REG_BINARY;
>>> add bogomips which has been cpu MHz*2 since Pentium MMX;
>>> handle as common former Intel only feature flags also supported on AMD;
>>> add 88 feature flags inc. AVX512 extensions, AES, SHA with 20 cpuid calls;
>>> commented out flags are mostly used but not currently reported in cpuinfo
>>> but some may not currently be used by Linux
>>
>> Thanks!  This must have been a lot of work.
> 
> Already had the info in some of my own code, that pointed out the discrepancies
> between Cygwin and Linux, and prompted the desired to level up.
> 
>> It would be easier to review if you would split it up into smaller patches, each
>> doing one thing, to the extent that this makes sense.  For example, the
>> simplification achieved by using the ftcprint macro could be done in a single
>> patch that's separate from the substantive changes.
> 
> Unfortunately, that was added later to make the got it/add it/skip it flag cross
> checks in Linux order more certain vs my own sequential tabular source.

What I was suggesting was that you go back after the fact and split up your 
patch into smaller, more easily digestible patches.  This simplifies review as 
well as later debugging.  The order in which you did things while developing the 
patch is not really relevant.

>> A few nits:
>>
>>> -      DWORD cpu_mhz = 0;
>>> -      RTL_QUERY_REGISTRY_TABLE tab[2] = {
>>> +      DWORD cpu_mhz;
>>> +      DWORD bogomips;
>>> +      long long microcode = 0xffffffff;	/* at least 8 bytes for AMD */
>>> +      union {
>>> +	  LONG len;
>>> +	  char uc_microcode[16];
>>> +      } uc;
>>> +
>>> +      cpu_mhz = 0;
>>> +      bogomips = 0;
>>> +      microcode = 0;
>>
>> Why change the existing intialization style?  How about
>>
>>         DWORD cpu_mhz = 0;
>>         DWORD bogomips = 0;
>>         long long microcode = 0;	/* at least 8 bytes for AMD */
> 
> Need to ensure they are initialized each time thru the CPU loop, not just once
> on entry, mainly because of what I found out about what I needed to do to get
> the variable length REG_BINARY key.

They get initialized each time through the loop even with the initialization as 
it was originally (DWORD cpu_mhz = 0;).  Or am I missing something?

Ken