[2/3] ntdll: Implement NtQuerySystemInformation/SystemLogicalProcessorInformation (try 6)
Vitaliy Margolen
wine-devel at kievinfo.com
Sat Oct 9 13:05:55 CDT 2010
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.
More information about the wine-devel
mailing list