Re: [2/3] ntdll: Implement NtQuerySystemInformation/SystemLogicalProcessorInformation (try 6)
On 10/08/2010 12:22 PM, Rudolf Mayerhofer wrote:
+static SYSTEM_LOGICAL_PROCESSOR_INFORMATION cached_lpi[1024];
You think there are systems with that many CPUs running Wine?
for (i = 0; i < (sizeof(cached_lpi) / sizeof(SYSTEM_LOGICAL_PROCESSOR_INFORMATION)); i++)
To make it easier to understand what you doing here please calculate number of elements in cached_lpi array with: sizeof(cached_lpi) / sizeof(cached_lpi[0]); This way people don't need to look up what type cached_lpi is.
@@ -1069,6 +1193,160 @@ void fill_cpu_info(void) } } fclose(f);
/* Parse Logical Processor Information in SysFS */{int coreflags = (cached_sci.FeatureSet & CPU_FEATURE_HTT) ? 1 : 0;int cpucount = 0, cachecount, nodecount, i;ULONG_PTR fullprocessormask = 0, processormask;
Please don't do this. Don't create a block just so you can declare variables. Put variable declaration at the top of the outer block.
while ((processormask = sysfs_cpu_topology_getmap(cpucount, "thread_siblings")) > 0){/* Core information */{/* Unique cores are found multiple times during iteration, add only once */for (i = 0; i < (sizeof(cached_lpi) / sizeof(SYSTEM_LOGICAL_PROCESSOR_INFORMATION)); i++){
...
}}
...
}
Same thing here.
+static int sysfs_cpucache_getint(int cpu, int cache, const char *type)
/* Cache information */cachecount = 0;while ((processormask = sysfs_cpucache_cpumap(cpucount, cachecount)) > 0){/* Empty Processormasks are invalid, ignore them */
What does this comment refers too? If it's for the above while() condition, then please put comment above it, not inside the block.
BYTE level = sysfs_cpucache_getint(cpucount, cachecount, "level");BYTE associativity = sysfs_cpucache_getint(cpucount, cachecount, "ways_of_associativity");WORD linesize = sysfs_cpucache_getint(cpucount, cachecount, "coherency_line_size");DWORD size = sysfs_cpucache_getint(cpucount, cachecount, "size");
I've already told you before - do not mix signed and unsigned. If result of sysfs_cpucache_getint is always assigned to an unsigned variable then change it's return type to unsigned type too.
/* Determine Cachetype */if (typestring[0] == 'D'){type = CacheData;}else if (typestring[0] == 'I'){type = CacheInstruction;}else if (typestring[0] == 'U'){type = CacheUnified;}else if (typestring[0] == 'T'){type = CacheTrace;}
Either use swith case construct here or use complete words and stricmp.
if ((processormask = sysfs_numanode_cpumap(nodecount)) > 0){/* Numa Nodes are already unique. just add them */do{}while ((processormask = sysfs_numanode_cpumap(nodecount)) > 0);}else{}
People already asked you not to do this. From looking at the code it appears that there is always one node. So get rid of everything except do{} while() block. It will do exactly what you want with extra if()s.
Vitaliy.
+static SYSTEM_LOGICAL_PROCESSOR_INFORMATION cached_lpi[1024];
You think there are systems with that many CPUs running Wine?
I honestly don't know the array is currently large enough to support 146 CPU's with 4 unique Caches (L1i,L1d,L2,L3), each in it's own processorpackage and numa node. This might be a bit of an overkill, any suggestions how many cpu's are used with wine in the worst case?
if ((processormask = sysfs_numanode_cpumap(nodecount)) > 0){/* Numa Nodes are already unique. just add them */do{}while ((processormask =sysfs_numanode_cpumap(nodecount)) > 0); + }
else{}People already asked you not to do this. From looking at the code it appears that there is always one node. So get rid of everything except do{} while() block. It will do exactly what you want with extra if()s.
This assumption is definitely wrong, on non-numa systems there is no node definition in SysFS. Windows on the same system shows one node and MSDN also hints that there should be a dummy node with number 0 and a processormask containing all known processors on the system in that case.
I'll see that i can fix the rest of the things you mentioned and update the patch as soon as i can. Thanks for the quick review and sorry for the trouble.
On 10/09/2010 12:37 PM, Rudolf Mayerhofer wrote:
+static SYSTEM_LOGICAL_PROCESSOR_INFORMATION cached_lpi[1024];
You think there are systems with that many CPUs running Wine?
I honestly don't know the array is currently large enough to support 146 CPU's with 4 unique Caches (L1i,L1d,L2,L3), each in it's own processorpackage and numa node. This might be a bit of an overkill, any suggestions how many cpu's are used with wine in the worst case?
Then you probably want to allocate it from the heap instead of creating a static array.
People already asked you not to do this. From looking at the code it appears that there is always one node. So get rid of everything except do{} while() block. It will do exactly what you want with extra if()s.
This assumption is definitely wrong, on non-numa systems there is no node definition in SysFS. Windows on the same system shows one node and MSDN also hints that there should be a dummy node with number 0 and a processormask containing all known processors on the system in that case.
I'm not talking about the content of the SysFS. I'm talking about what your code does. If you remove that "if{} else{}" part and keep "do{} while();" part as-is you will get exactly the same result.
Vitaliy.
On Sunday 10 October 2010 00:26:45 you wrote:
On 10/09/2010 12:37 PM, Rudolf Mayerhofer wrote:
+static SYSTEM_LOGICAL_PROCESSOR_INFORMATION cached_lpi[1024];
You think there are systems with that many CPUs running Wine?
I honestly don't know the array is currently large enough to support 146 CPU's with 4 unique Caches (L1i,L1d,L2,L3), each in it's own processorpackage and numa node. This might be a bit of an overkill, any suggestions how many cpu's are used with wine in the worst case?
Then you probably want to allocate it from the heap instead of creating a static array.
I haven't found out how to do this properly yet and i think that i definitely need help on that one (like an example) as my skills in C are not that good and all i manage to get when trying to allocate the stuff from the heap is a segfault.
For the time being i reduced the arraysize to 896, which should still be large enough to support 128 CPUs in SysFS in the worst case.
People already asked you not to do this. From looking at the code it appears that there is always one node. So get rid of everything except do{} while() block. It will do exactly what you want with extra if()s.
This assumption is definitely wrong, on non-numa systems there is no node definition in SysFS. Windows on the same system shows one node and MSDN also hints that there should be a dummy node with number 0 and a processormask containing all known processors on the system in that case.
I'm not talking about the content of the SysFS. I'm talking about what your code does. If you remove that "if{} else{}" part and keep "do{} while();" part as-is you will get exactly the same result.
I figured that one out, although it took me a while.
On 10/10/2010 03:42 AM, Rudolf Mayerhofer wrote:
I haven't found out how to do this properly yet and i think that i definitely need help on that one (like an example) as my skills in C are not that good and all i manage to get when trying to allocate the stuff from the heap is a segfault.
That's not an excuse to create static structure of arbitrary size.
I figured that one out, although it took me a while.
No, you didn't. Try again. You duplicated exactly the same code.
Vitaliy.
On Sunday 10 October 2010 18:43:25 Vitaliy Margolen wrote:
On 10/10/2010 03:42 AM, Rudolf Mayerhofer wrote:
I haven't found out how to do this properly yet and i think that i definitely need help on that one (like an example) as my skills in C are not that good and all i manage to get when trying to allocate the stuff from the heap is a segfault.
That's not an excuse to create static structure of arbitrary size.
It is not, but the size should be large enough to support all systems for some time. I already asked for help how to do this with heap properly since i simply can't figure it out, sorry.
I figured that one out, although it took me a while.
No, you didn't. Try again. You duplicated exactly the same code.
Did you look at try 8 ? I'm pretty sure i changed that part to a single while loop and checked the part for adding a dummy on non-numa systems with a separate if. If that's not what you meant could you please explain where the problem is (again) ?
-
Rudolf Mayerhofer -
Vitaliy Margolen