Re: [PATCH] kernel32: implement GetLogicalProcessorInformation
I have resent the patch as a try 2 (there was a try 1.5 to fix test issues under windows older than XP SP3). The patch contains changes based on your feedback. Some things I stand by, and won't change. If you feel differently, you should argue for their validity a little bit more than don't use access(2). Some things I don't have time to address, for example this NtQueryInformation thing I know nothing about. Sorry. That seems to be something that can be done as a separate commit by someone who cares about that aspect (you?). So this is it. You like, you take. You don't you drop. For my wine tree, it works beautifully and I am good with that. Cheers C.
Re: [PATCH] kernel32: implement GetLogicalProcessorInformation
On Fri, Nov 25, 2011 at 4:07 PM, Claudio Fontana claudio.font...@gmail.com wrote: I have resent the patch as a try 2 (there was a try 1.5 to fix test issues under windows older than XP SP3). The patch contains changes based on your feedback. Which is completely broken, since the U() macro you suggested to use is a wine/test.h feature only, and completely broke the patch. So I fall back to my try 1.5 for my tree.
Re: [PATCH] kernel32: implement GetLogicalProcessorInformation
On Fri, Nov 25, 2011 at 8:01 PM, Charles Davis cda...@mymail.mines.edu wrote: On Nov 25, 2011, at 8:17 AM, Claudio Fontana wrote: On Fri, Nov 25, 2011 at 4:07 PM, Claudio Fontana claudio.font...@gmail.com wrote: I have resent the patch as a try 2 (there was a try 1.5 to fix test issues under windows older than XP SP3). The patch contains changes based on your feedback. Which is completely broken, since the U() macro you suggested to use is a wine/test.h feature only, and completely broke the patch. You are right. The way most files (other than tests) in Wine deal with that is that they unconditionally define NONAMELESSUNION and NONAMELESSSTRUCT. So you wouldn't use U(buffer[i]) (except in the tests, where you can do that), you'd use buffer[i].u . So I fall back to my try 1.5 for my tree. OK, you do that. I'm pretty sure AJ won't take your patch anyway until it's done right (i.e. like I said, in ntdll). I guess I'll take that up, since I have nothing better to do ;). That's great! When it's done it will be one less out of tree patch to maintain, always a win. Ciao, C
Re: [PATCH] kernel32: implement GetLogicalProcessorInformation
On 11/25/2011 08:35 PM, Claudio Fontana wrote: That's great! When it's done it will be one less out of tree patch to maintain, always a win. Ciao, C You're free to keep your patches on your own/private tree but if you're discussing here, I guess you wanted your stuff being committed in the main/official tree. People here take time to read patches and to give useful advices to make patches accepted. Open Source philosophy is I know how to do that better, let's build together something great, not I'll code this part myself and keep it private. Of course, it's my two cents but it's sad people react like you.
Re: [PATCH] kernel32: implement GetLogicalProcessorInformation
On Nov 25, 2011, at 8:17 AM, Claudio Fontana wrote: On Fri, Nov 25, 2011 at 4:07 PM, Claudio Fontana claudio.font...@gmail.com wrote: I have resent the patch as a try 2 (there was a try 1.5 to fix test issues under windows older than XP SP3). The patch contains changes based on your feedback. Which is completely broken, since the U() macro you suggested to use is a wine/test.h feature only, and completely broke the patch. You are right. The way most files (other than tests) in Wine deal with that is that they unconditionally define NONAMELESSUNION and NONAMELESSSTRUCT. So you wouldn't use U(buffer[i]) (except in the tests, where you can do that), you'd use buffer[i].u . So I fall back to my try 1.5 for my tree. OK, you do that. I'm pretty sure AJ won't take your patch anyway until it's done right (i.e. like I said, in ntdll). I guess I'll take that up, since I have nothing better to do ;). Chip
Re: [PATCH] kernel32: implement GetLogicalProcessorInformation
Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=15612 Your paranoid android. === WNT4WSSP6 (32 bit glpi) === Timeout === W2KPROSP4 (32 bit glpi) === Timeout
Re: [PATCH] kernel32: implement GetLogicalProcessorInformation
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
Re: [PATCH] kernel32: implement GetLogicalProcessorInformation
On Nov 24, 2011, at 5:13 PM, Charles Davis wrote: On Nov 24, 2011, at 4:21 AM, Claudio Fontana wrote: +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().) I don't think there's such a general rule. As I understand it, one has to be mindful of the undefined order of evaluation of function arguments. So, the call which may set the last error must not be in an argument list with the call of GetLastError(), because you can't be sure which will be called first. That is, in this case, the call to GetLogicalProcessorInformation() must not be within the ok() argument list along with GetLastError(). Since it is not, the above code is fine. (I also think your paranoia about access(2) is overblown, but I agree it isn't the best means of checking for the existence of a file.) Regards, Ken
Re: [PATCH] kernel32: implement GetLogicalProcessorInformation
Hello,
I saw a bug 27189 in the bug database where Austin English was waiting
for a GLPI implementation.
I had one already (among other patches I keep against the wine tree),
so I attached it, and while I was at it, I thought to submit it to
wine-patches as well.
I am not sure I have the time to work on all items you mentioned,
so if you want to cannibalize it and do things differently, you are
more than welcome to take what you need from my patch.
Some comments on your comments below :)
On Fri, Nov 25, 2011 at 12:13 AM, Charles Davis cda...@mymail.mines.edu wrote:
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.
You understand this more than me. Maybe it could be a subsequent commit?
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.
I see.
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:
the comment is right before the function I am documenting. The
function I am documenting is
As for the formatting, I am not sure what you mean, they look pretty
much the same.
Your formatting adds some more spaces before the function name, and
less before the additional comments below.
/***
* 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]).
good to know. I did not know about the U() macro. ACK for the 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.
This is not 1978, no. And?
glpi_impl_fb
short and sweet and to the point. The right name for a static function
that is glpi implementation fallback.
This isn't java, 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.
I know.
+
+ 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 =
