Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): fix issues, add fields, flags
On 10/6/2019 12:23 PM, Brian Inglis wrote:
> On 2019-10-06 08:31, Ken Brown wrote:
>> 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.
>
> How on earth does one accomplish that?
Think about how to logically divide your work into several tasks. Then do one
task at a time, making a commit after each one. For example, "code
simplification via ftcprint" might be task 1.
You can do this by hand, but there are also tools that can help. Achim
mentioned Emacs/magit, but that doesn't make sense for you unless you're
already
an Emacs user and willing to learn magit. Another tool that I've occasionally
found useful is splitpatch. Finally, git itself provides a tool
(https://git-scm.com/book/en/v2/Git-Tools-Interactive-Staging).
A few nits:
> - DWORD cpu_mhz = 0;
> - RTL_QUERY_REGISTRY_TABLE tab[2] = {
> + DWORD cpu_mhz;
> + DWORD bogomips;
> + long long microcode = 0x; /* 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?
>
> I assume that initializations are done as if at compile time, so do
> assignments
> at the appropriate points.
> I have no idea what C++ does for local non-object data type initialization.
$ cat a.cc
#include
using namespace std;
int
main ()
{
for (int i = 0; i < 5; i++)
{
int n = 0;
cout << i << " " << n << endl;
n++;
}
}
$ g++ a.cc
$ ./a
0 0
1 0
2 0
3 0
4 0
Ken
Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): fix issues, add fields, flags
On 2019-10-06 08:31, Ken Brown wrote:
> 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.
How on earth does one accomplish that?
My model of using source control workflows, and how git actually works, differ
so radically that using git commands generates more and more strident
accusations and complaints, until repos are unrecoverable, and I resort to:
$ find . -mtime -7 | xargs cp -p -t ../; cd ..; rm -rf repo; \
git clone git://.../repo.git; find . -mtime -7 | xargs cp -p -t repo/
so I now skip the other git commands that get it into an unrecoverable state.
>>> A few nits:
>>>
- DWORD cpu_mhz = 0;
- RTL_QUERY_REGISTRY_TABLE tab[2] = {
+ DWORD cpu_mhz;
+ DWORD bogomips;
+ long long microcode = 0x; /* 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?
I assume that initializations are done as if at compile time, so do assignments
at the appropriate points.
I have no idea what C++ does for local non-object data type initialization.
--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada
This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): fix issues, add fields, flags
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 = 0x;/* 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
Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): fix issues, add fields, flags
Brian Inglis writes: >> 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. I usually use Emacs/magit to split up a bunch of changes into a more comprehensible series of commits post factum, but there are other ways to achieve that same goal. Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ Wavetables for the Waldorf Blofeld: http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables
Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): fix issues, add fields, flags
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.
> A few nits:
>
>> - DWORD cpu_mhz = 0;
>> - RTL_QUERY_REGISTRY_TABLE tab[2] = {
>> + DWORD cpu_mhz;
>> + DWORD bogomips;
>> + long long microcode = 0x; /* 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.
>> + memset(, 0, sizeof(uc.uc_microcode));
>^ ^
> (Space before parenthesis, here and in several other places.)
>
>> + cpu_mhz = ((cpu_mhz - 1)/10 + 1)*10; /* round up a digit */
>
> Please surround '/' and '*' by spaces (and similarly in what follows). Also,
> the comment is confusing. Maybe "round up to a multiple of 10"?
>
>> + for (uint32_t l = maxf; 1 < l; --l) {
> ^
> (Brace on next line, and also in one other place.)
Sorry missed the expected style in those places.
Will tweak and submit v2.
--
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada
This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): fix issues, add fields, flags
Hi Brian,
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.
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.
A few nits:
> - DWORD cpu_mhz = 0;
> - RTL_QUERY_REGISTRY_TABLE tab[2] = {
> + DWORD cpu_mhz;
> + DWORD bogomips;
> + long long microcode = 0x; /* 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 */
> + memset(, 0, sizeof(uc.uc_microcode));
^ ^
(Space before parenthesis, here and in several other places.)
> + cpu_mhz = ((cpu_mhz - 1)/10 + 1)*10; /* round up a digit */
Please surround '/' and '*' by spaces (and similarly in what follows). Also,
the comment is confusing. Maybe "round up to a multiple of 10"?
> + for (uint32_t l = maxf; 1 < l; --l) {
^
(Brace on next line, and also in one other place.)
Ken
Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): fix issues, add fields, flags
On 2019-10-05 00:30, ASSI wrote: > Brian Inglis writes: >> For informal comparison, attached are Cygwin, WSL, and test release cpuinfo >> output, with diffs against the test release output, and the Windows registry >> CentralProcessor dump (be careful not to double click on Windows systems!) > The easiest way to prevent that problem would have been to give that > file a .txt extension, no? Could have stripped the header line to disable it doing anything too, and both would have helped with org milters. I've had most file types: graphics, PDF, text, etc. treated like HTML, opening in browser tabs for so long, I just thought of mentioning it right before sending. -- Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada This email may be disturbing to some readers as it contains too much technical detail. Reader discretion is advised.
Re: [PATCH] fhandler_proc.cc(format_proc_cpuinfo): fix issues, add fields, flags
Brian Inglis writes: > For informal comparison, attached are Cygwin, WSL, and test release cpuinfo > output, with diffs against the test release output, and the Windows registry > CentralProcessor dump (be careful not to double click on Windows > systems!) The easiest way to prevent that problem would have been to give that file a .txt extension, no? Regards, Achim. -- +<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+ SD adaptation for Waldorf rackAttack V1.04R1: http://Synth.Stromeko.net/Downloads.html#WaldorfSDada
