Re: [2/3] ntdll: Implement NtQuerySystemInformation/SystemLogicalProcessorInformation (try 6)

[Vitaliy Margolen]

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.