Hi,
On Nov 24, 2011, at 4:21 AM, Claudio Fontana wrote:
> First implementation of GetLogicalProcessorInformation.
> Limitations: all logical processors are added to the same NUMA set,
> and all cores are added to the same package.
> Only the linux-specific helper function is implemented, so for now
> everybody else gets the fallback hard-coded results only.
> The fallback is also triggered when the OS-specific APIs fail.
>
> The linux-specific function is based on sysfs.
>
> The new OS-specific functions can be easily added as additional
> ifdefs by following the linux example.
> ---
> dlls/kernel32/process.c | 350 ++++++++++++++++++++++++++++++++++++++-
> dlls/kernel32/tests/Makefile.in | 1 +
> dlls/kernel32/tests/glpi.c | 82 +++++++++
You should really implement this in ntdll.NtQuerySystemInformation (information class SystemLogicalProcessorInformation). In general, kernel32 forwards most of its implementation to ntdll, which reduces code duplication.
> 3 files changed, 430 insertions(+), 3 deletions(-)
> create mode 100644 dlls/kernel32/tests/glpi.c
IMO, you should add the test to dlls/ntdll/tests/info.c, and test NtQuerySystemInformation() with info class SystemLogicalProcessorInformation.
>
> diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c
> index b6c0b9d..de39244 100644
> --- a/dlls/kernel32/process.c
> +++ b/dlls/kernel32/process.c
> @@ -51,6 +51,12 @@
>
> #include "ntstatus.h"
> #define WIN32_NO_STATUS
> +
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winerror.h"
> +#include "winnt.h"
> +
> #include "winternl.h"
> #include "kernel_private.h"
> #include "psapi.h"
> @@ -3705,14 +3711,352 @@ HANDLE WINAPI GetCurrentProcess(void)
> return (HANDLE)~(ULONG_PTR)0;
> }
>
> +/***************
> + * glpi_impl_fb:
> + * a fallback implementation of GetLogicalProcessorInformation
> + * for the systems we do not support yet,
> + * or for when the OS-specific API fails.
> + * Same arguments and return value as glpi.
> + */
This comment should really come right before the function you're documenting. Also, it's formatted wrong. It should look more like this:
/***********************************************************************************
* logical_processor_info_fallback
*
* Fallback implementation of NtQuerySystemInformation() with info class
* SystemLogicalProcessorInfo.
*/
> +
> +#undef SLPI
> +#ifdef NONAMELESSUNION
> +#define SLPI(buffer, idx) buffer[idx].DUMMYUNIONNAME
> +#else
> +#define SLPI(buffer, idx) buffer[idx]
> +#endif /* NONAMELESSUNION */
You could avoid the ifdef if you used the U() macro. In fact, why are you defining a macro at all? Just use U(buffer[idx]).
> +
> +static BOOL
> +glpi_impl_fb(PSYSTEM_LOGICAL_PROCESSOR_INFORMATION buffer, PDWORD pbuflen)
Surely you could use a more descriptive name. (See one example in my example doc comment.) This isn't 1978, you know.
> +{
> + int glpi_n = 5;
> + FIXME("(%p,%p): reporting hard coded information\n", buffer, pbuflen);
> +
> + if (*pbuflen < sizeof(*buffer) * glpi_n)
> + {
> + SetLastError(ERROR_INSUFFICIENT_BUFFER);
> + *pbuflen = sizeof(*buffer) * glpi_n;
> + return FALSE;
> + }
> +
> + buffer[0].ProcessorMask = (ULONG_PTR)0x01;
> + buffer[1].ProcessorMask = (ULONG_PTR)0x01;
> + buffer[2].ProcessorMask = (ULONG_PTR)0x01;
> + buffer[3].ProcessorMask = (ULONG_PTR)0x01;
> + buffer[4].ProcessorMask = (ULONG_PTR)0x01;
These casts aren't necessary.
> +
> + buffer[0].Relationship = RelationProcessorCore;
> + buffer[1].Relationship = RelationNumaNode;
> + buffer[2].Relationship = RelationCache;
> + buffer[3].Relationship = RelationCache;
> + buffer[4].Relationship = RelationProcessorPackage;
> +
> + SLPI(buffer, 0).ProcessorCore.Flags = (BYTE)0x00;
> + SLPI(buffer, 1).NumaNode.NodeNumber = (DWORD)0x00;
Neither are these.
> +
> + SLPI(buffer, 2).Cache.Level = (BYTE)0x01;
> + SLPI(buffer, 2).Cache.Associativity = (BYTE)0x08;
> + SLPI(buffer, 2).Cache.LineSize = (WORD)64;
> + SLPI(buffer, 2).Cache.Size = (DWORD)16 << 10; /* 16 KB */
Or these.
> + SLPI(buffer, 2).Cache.Type = CacheData;
> +
> + SLPI(buffer, 3).Cache.Level = (BYTE)0x02;
> + SLPI(buffer, 3).Cache.Associativity = (BYTE)0x08;
> + SLPI(buffer, 3).Cache.LineSize = (WORD)64;
> + SLPI(buffer, 3).Cache.Size = (DWORD)1024 << 10; /* 1024 KB */
Or these.
> + SLPI(buffer, 3).Cache.Type = CacheUnified;
> + /* no additional info for processor package at buffer[4] needed. */
> +
> + *pbuflen = sizeof(*buffer) * glpi_n;
> + return TRUE;
> +}
> +
> +#ifdef linux
> +/******************
> + * glpi_impl_linux:
> + * an implementation of GetLogicalProcessorInformation
> + * for linux sysfs.
> + * Returns: -1 on API failure (trigger fallback),
> + * otherwise see glpi.
> + */
See above.
> +
> +#define SYS_CPU "/sys/devices/system/cpu/cpu"
> +#define SYS_CPU_MAX 63
I'd criticize your use of a static limit, if only Windows didn't have that limit ;). Actually, you can only have 32 processors on a 32-bit system because a ULONG_PTR is 32 bits there.
> +#define SYS_CPU_COREID "/topology/core_id"
> +#define SYS_CPU_PACKAGEID "/topology/physical_package_id"
> +#define SYS_CPU_CACHE "/cache/index"
> +#define SYS_CPU_CACHE_MAX 12
> +#define SYS_CPU_CACHE_LEVEL "/level"
> +#define SYS_CPU_CACHE_SIZE "/size"
> +#define SYS_CPU_CACHE_TYPE "/type"
> +#define SYS_CPU_CACHE_LINE "/coherency_line_size"
> +#define SYS_CPU_CACHE_ASSOC "/ways_of_associativity"
> +
> +static int glpi_impl_linux(SYSTEM_LOGICAL_PROCESSOR_INFORMATION *buffer,
> + PDWORD pbuflen)
> +{
> + /* remember the glpi results, so that subsequent calls are faster */
> + static int n_cores = 0;
> + static int n_caches = 0;
> + static PSYSTEM_LOGICAL_PROCESSOR_INFORMATION cores = NULL;
> + static PSYSTEM_LOGICAL_PROCESSOR_INFORMATION caches = NULL;
> + static SYSTEM_LOGICAL_PROCESSOR_INFORMATION numa[1];
> + static SYSTEM_LOGICAL_PROCESSOR_INFORMATION package[1];
> +
> + if (!n_cores)
> + {
> + int n_cpus = 0; int cpu_i;
> + char path[PATH_MAX];
> +
> + TRACE("building new cached result.\n");
> +
> + for (n_cpus = 0; n_cpus < SYS_CPU_MAX; n_cpus++)
> + {
> + snprintf(path, sizeof(path), SYS_CPU "%d", n_cpus);
> + if (access(path, R_OK | X_OK) != 0)
Don't use access(2). There's a big fat warning on the man-page telling you not to use it because of a race condition. Another way to solve the problem you're trying to solve might be to scan the "/sys/devices/system/cpu/" directory for the number of entries using the POSIX readdir(3) interface. You could scan them all up front, or you could take each one as you go, re-allocating the table for each new entry.
> + break;
> + }
> +
> + TRACE("detected %d logical cpus.\n", n_cpus);
> + if (n_cpus < 1)
> + {
> + TRACE("requesting fallback implementation.\n");
> + return -1;
> + }
> +
> + cores = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*cores) * n_cpus);
> + caches = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*caches) * 1);
> +
> + for (cpu_i = 0; cpu_i < n_cpus; cpu_i++)
> + {
> + FILE *f; int core_id; int i;
> + snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_COREID, cpu_i);
> + cores[n_cores].ProcessorMask = (ULONG_PTR)0x01 << cpu_i;
This cast might be needed if there are more than 32 CPUs, so that's OK.
> + cores[n_cores].Relationship = RelationProcessorCore;
> +
> + if (!(f = fopen(path, "r")) || fscanf(f, "%d", &core_id) != 1)
> + {
> + /* topology info not available, provide default cpuid=cpu_i */
> + ERR("could not read cpu%d topology.\n", cpu_i);
> + SLPI(cores, n_cores).Reserved[0] = cpu_i;
> + n_cores++;
> + if (f)
> + fclose(f);
> + continue; /* next cpu */
> + }
> +
> + /* normal sane case */
> + fclose(f);
> + /* store the core_id to catch cpus with same core_id */
> + SLPI(cores, n_cores).Reserved[0] = core_id;
> +
> + /* look for this core_id in previous elements */
> + for (i = 0; i < n_cores; i++)
> + if (SLPI(cores, i).Reserved[0] == SLPI(cores, n_cores).Reserved[0])
> + break;
> +
> + if (i < n_cores)
> + {
> + TRACE("found existing coreid %d for cpu%d.\n", core_id, cpu_i);
> + /* add this logical processor to the existing mask */
> + cores[i].ProcessorMask |= cores[n_cores].ProcessorMask;
> + /* do not advance n_cores: this element can be overwritten. */
> + }
> + else
> + {
> + TRACE("found new coreid %d for cpu%d.\n", core_id, cpu_i);
> +
> + /* look for caches */
> + for (i = 0; i < SYS_CPU_CACHE_MAX; i++)
> + {
> + unsigned long value;
> + char string[80];
> +
> + snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d",
> + cpu_i, i);
> + if (access(path, R_OK | X_OK) != 0)
Don't use access(2).
> + break; /* no more caches found. */
> +
> + caches = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> + caches, sizeof(*caches) * (n_caches + 1));
> +
> + /* we keep the index into the cores array to set
> + the right value at the end. */
> + caches[n_caches].ProcessorMask = (ULONG_PTR)n_cores;
This cast, however, is definitely not needed.
> + caches[n_caches].Relationship = RelationCache;
> +
> + snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> + SYS_CPU_CACHE_LEVEL, cpu_i, i);
> + if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> + SLPI(caches, n_caches).Cache.Level = (BYTE)0x01;
Neither is this.
> + else
> + SLPI(caches, n_caches).Cache.Level = (BYTE)value;
> + if (f)
> + fclose(f);
> +
> + snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> + SYS_CPU_CACHE_ASSOC, cpu_i, i);
> + if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> + SLPI(caches, n_caches).Cache.Associativity = 0x08;
> + else
> + SLPI(caches, n_caches).Cache.Associativity = (BYTE)value;
> + if (f)
> + fclose(f);
> +
> + snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> + SYS_CPU_CACHE_LINE, cpu_i, i);
> + if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> + SLPI(caches, n_caches).Cache.LineSize = (WORD)64;
This cast is not needed.
> + else
> + SLPI(caches, n_caches).Cache.LineSize = (WORD)value;
> + if (f)
> + fclose(f);
> +
> + snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> + SYS_CPU_CACHE_SIZE, cpu_i, i);
> + if (!(f = fopen(path, "r")) || fscanf(f, "%lu", &value) != 1)
> + SLPI(caches, n_caches).Cache.Size = (DWORD)16 << 10;
This cast isn't needed unless an int is less than 32 bits in size.
> + else
> + SLPI(caches, n_caches).Cache.Size = (DWORD)value << 10;
> + if (f)
> + fclose(f);
> +
> + snprintf(path, sizeof(path), SYS_CPU "%d" SYS_CPU_CACHE "%d"
> + SYS_CPU_CACHE_TYPE, cpu_i, i);
> + if (!(f = fopen(path, "r")) || !fgets(string, sizeof(string), f))
> + SLPI(caches, n_caches).Cache.Type = CacheData;
> + else switch (string[0])
> + {
> + case 'D':
> + SLPI(caches, n_caches).Cache.Type = CacheData; break;
> + case 'I':
> + SLPI(caches, n_caches).Cache.Type = CacheInstruction; break;
> + case 'U': default:
> + SLPI(caches, n_caches).Cache.Type =
> + string[2] == 'i' ? CacheUnified : CacheTrace; break;
> + }
> +
> + if (f)
> + fclose(f);
> +
> + n_caches++;
> + }
> + n_cores++;
> + TRACE("detected %d caches for coreid %d\n", i, core_id);
> + }
> + }
> +
> + if (n_caches < 1)
> + {
> + HeapFree(GetProcessHeap(), 0, caches);
> + caches = NULL;
> + }
> +
> + /* trim leftovers */
> + cores = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY,
> + cores, sizeof(*cores) * n_cores);
> +
> + if (1) /* final cleanup work */
Why the if (1)?
> + {
> + int i;
> + /* set the real processor mask in the caches. */
> + for (i = 0; i < n_caches; i++)
> + {
> + int cores_i;
> + cores_i = caches[i].ProcessorMask;
> + caches[i].ProcessorMask = cores[cores_i].ProcessorMask;
> + }
> +
> + /* remove the reserved values from the cores, set flags,
> + build numa and package. */
> +
> + numa[0].ProcessorMask = 0x00;
> + numa[0].Relationship = RelationNumaNode;
> + SLPI(numa, 0).NumaNode.NodeNumber = 0x00;
> + package[0].Relationship = RelationProcessorPackage;
> +
> + for (i = 0; i < n_cores; i++)
> + {
> + SLPI(cores, i).Reserved[0] = 0;
> + SLPI(cores, i).ProcessorCore.Flags = 0x00; /* ?unclear */
> + numa[0].ProcessorMask |= cores[i].ProcessorMask;
> + }
> +
> + package[0].ProcessorMask = numa[0].ProcessorMask;
> + }
> + }
> +
> + TRACE("getting cached result.\n");
> +
> + if (1) /* final check for required size, final result */
Same here.
> + {
> + DWORD required; required = sizeof(*cores) * (n_cores + n_caches + 2);
> + if (*pbuflen < required)
> + {
> + TRACE("return with failure (ERROR_INSUFFICIENT_BUFFER).\n");
> + SetLastError(ERROR_INSUFFICIENT_BUFFER);
> + *pbuflen = required;
> + return 0;
> + }
> +
> + *pbuflen = required;
> +
> + /* beware: buffer is SYSTEM_LOGICAL_PROCESSOR_INFORMATION *buffer; */
> + memcpy(buffer, cores, sizeof(*cores) * n_cores);
> + buffer += n_cores;
> +
> + if (n_caches > 0)
> + {
> + memcpy(buffer, caches, sizeof(*caches) * n_caches);
> + buffer += n_caches;
> + }
> +
> + *buffer++ = package[0];
> + *buffer++ = numa[0];
> + }
> +
> + TRACE("return success!\n");
Is this really necessary? IMO it's generally better to be quiet if you have nothing interesting to say (in typical Unix fashion). And a function succeeding isn't interesting.
> + return 1;
> +}
> +
> +#undef SYS_CPU
> +#undef SYS_CPU_MAX
> +#undef SYS_CPU_COREID
> +#undef SYS_CPU_PACKAGEID
> +#undef SYS_CPU_CACHE
> +#undef SYS_CPU_CACHE_MAX
> +#undef SYS_CPU_CACHE_LEVEL
> +#undef SYS_CPU_CACHE_SIZE
> +#undef SYS_CPU_CACHE_TYPE
> +#undef SYS_CPU_CACHE_LINE
> +#undef SYS_CPU_CACHE_ASSOC
> +
> +#endif /* linux */
> +
> /***********************************************************************
> * GetLogicalProcessorInformation (KERNEL32.@)
> */
> BOOL WINAPI GetLogicalProcessorInformation(PSYSTEM_LOGICAL_PROCESSOR_INFORMATION buffer, PDWORD pBufLen)
> {
> - FIXME("(%p,%p): stub\n", buffer, pBufLen);
> - SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
> - return FALSE;
> + int rv;
> +
> + if (pBufLen == NULL || buffer == NULL)
> + {
> + SetLastError(ERROR_BAD_ARGUMENTS);
> + return FALSE;
> + }
> +
> +#if defined linux
> + rv = glpi_impl_linux(buffer, pBufLen);
> +#else /* no supported OS found */
> + rv = -1;
> +#endif
> +
> + /* fallback hard-coded result */
> + if (rv < 0)
> + rv = glpi_impl_fb(buffer, pBufLen);
> +
> + return rv;
> }
>
> /***********************************************************************
> diff --git a/dlls/kernel32/tests/Makefile.in b/dlls/kernel32/tests/Makefile.in
> index dce27db..1505944 100644
> --- a/dlls/kernel32/tests/Makefile.in
> +++ b/dlls/kernel32/tests/Makefile.in
> @@ -17,6 +17,7 @@ C_SRCS = \
> file.c \
> format_msg.c \
> generated.c \
> + glpi.c \
> heap.c \
> loader.c \
> locale.c \
> diff --git a/dlls/kernel32/tests/glpi.c b/dlls/kernel32/tests/glpi.c
> new file mode 100644
> index 0000000..a81d1d5
> --- /dev/null
> +++ b/dlls/kernel32/tests/glpi.c
Like I said, this should be integrated into ntdll:info instead of being its own test in kernel32. Note that the style of reporting errors is different between ntdll and kernel32. In kernel32, the function returns a BOOL, and if it's false, you have to check the LastError. In ntdll, however, functions simply return the status code directly. Also, ntdll functions return an NTSTATUS instead of a constant from <winerror.h>.
> @@ -0,0 +1,82 @@
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <time.h>
> +#include <stdio.h>
> +
> +#include "wine/test.h"
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winerror.h"
> +
> +static void test_glpi(void)
> +{
> + SYSTEM_LOGICAL_PROCESSOR_INFORMATION buffer[200];
> + DWORD buflen = sizeof(buffer);
> + BOOL ret; int i, n;
> +
> + ret = GetLogicalProcessorInformation(NULL, &buflen);
> + ok(!ret, "Passing NULL as buffer should not be ok.\n");
> +
> + ret = GetLogicalProcessorInformation(buffer, NULL);
> + ok(!ret, "Passing NULL as pbuflen should not be ok.\n");
> +
> + ret = GetLogicalProcessorInformation(buffer, &buflen);
> + ok(ret, "Normal glpi call (%d)\n", GetLastError());
Don't call GetLastError() inside an ok(). (Actually, that's moot, because this test belongs in ntdll:process, but in general, you shouldn't call GetLastError() inside an ok().)
> +
> + ret = GetLogicalProcessorInformation(buffer, &buflen);
> + ok(ret, "glpi call with the resulting buflen (%d)\n", buflen);
Showing the last error (ntdll: return status) here might help diagnosing any problems with this test. (But don't call GetLastError() inside the ok().)
> +
> + buflen--;
> +
> + ret = GetLogicalProcessorInformation(buffer, &buflen);
> + ok(!ret, "glpi call with insufficient buflen (%d)\n", buflen);
Here you should make sure the LastError is ERROR_INSUFFICIENT_BUFFER (corresponding NTSTATUS: STATUS_BUFFER_TOO_SMALL, or possibly STATUS_INVALID_BUFFER_SIZE), and not some other error.
> +
> + ret = GetLogicalProcessorInformation(buffer, &buflen);
> + ok(ret, "glpi call with resulting buflen (%d)\n", buflen);
Trace the LastError (ntdll: return status) here, too.
> +
> + n = buflen / sizeof(SYSTEM_LOGICAL_PROCESSOR_INFORMATION);
> +
> + printf("glpi result: %d array elements.\n", n);
Don't use printf(3); use trace().
> +
> +#ifdef NONAMELESSUNION
> +#define BUFFER_I buffer[i].u
> +#else
> +#define BUFFER_I buffer[i]
> +#endif /* NONAMELESSUNION */
Like I said above, just use U(buffer[i]).
> +
> + for (i = 0; i < n; i++)
> + {
> + switch (buffer[i].Relationship)
> + {
> + case RelationProcessorCore:
> + printf("ProcessorCore idx=%03d mask=%lxh flags=%u\n",
> + i, buffer[i].ProcessorMask, BUFFER_I.ProcessorCore.Flags);
> + break;
> + case RelationNumaNode:
> + printf("NumaNode idx=%03d mask=%lxh noden=%u\n",
> + i, buffer[i].ProcessorMask, BUFFER_I.NumaNode.NodeNumber);
> + break;
> + case RelationCache:
> + printf("Cache idx=%03d mask=%lxh "
> + "l=%u, a=%u, ls=%u, sz=%u, ty=%u\n",
> + i, buffer[i].ProcessorMask,
> + BUFFER_I.Cache.Level, BUFFER_I.Cache.Associativity,
> + BUFFER_I.Cache.LineSize, BUFFER_I.Cache.Size,
> + BUFFER_I.Cache.Type);
> + break;
> + case RelationProcessorPackage:
> + printf("ProcessorPack idx=%03d mask=%lxh\n",
> + i, buffer[i].ProcessorMask);
For all of these, use trace() instead of printf(3).
> + break;
> + default:
> + ok(0, "invalid relationship %d", buffer[i].Relationship);
> + }
> + }
> +#undef BUFFER_I
> +}
> +
> +START_TEST(glpi)
> +{
> + test_glpi();
> +}
> +
> --
> 1.6.4
>
>
>
One general note about your patch: use spaces, not tabs. The rest of the file only uses tabs in doc comments.
Chip
