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.