Re: [PATCH] kernel32: implement GetLargePageMinimum

[Sebastian Lackner]

On 14.06.2016 09:09, Marcus Meissner wrote:
> From: Austin English
>
> https://bugs.winehq.org/show_bug.cgi?id=18745
>
> Report 2MB minimum page size on x86 32bit and 64bit on Linux.
>
> This assumes that all x86 processors and the 64/32bit emulation mode
> have hugetable support and it is always enabled.
>
> Signed-off-by: Marcus Meissner 
> Signed-off-by: Austin English

As pointed out on IRC, the order of the sign-offs is wrong here. Your own signoff
should always be the last because you have to take responsibility when you send a patch.
> dlls/kernel32/cpu.c           | 15 +++++++++++++++
>  dlls/kernel32/kernel32.spec   |  2 +-
>  dlls/kernel32/tests/process.c | 16 ++++++++++++++++
>  3 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/kernel32/cpu.c b/dlls/kernel32/cpu.c
> index f48fcf0..f8b2c3d 100644
> --- a/dlls/kernel32/cpu.c
> +++ b/dlls/kernel32/cpu.c
> @@ -291,3 +291,18 @@ err:
>      }
>      return TRUE;
>  }
> -
>
> +/***********************************************************************
> -
> -
>       GetLargePageMinimum (KERNEL32.@)
> - */
>
> +SIZE_T WINAPI GetLargePageMinimum(void)
> +{
> +#ifdef linux

I do not see any need for that #ifdef linux. Its more architecture specific
than linux specific. Dmitry already mentioned the same concern on the mailing list.
> +#if defined(__i386___) || defined(__x86_64__)
> - /* Linux x86 hugepages are minimum 2 MB */
> - return 2*1024*1024;
>
> +#endif
> +#endif
> - FIXME("stub: not implemented on your platform/architecture.\n");
> - return 0;
>
> +}
> diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
> index ec7155b6e..0fc4dcb 100644
> --- a/dlls/kernel32/kernel32.spec
> +++ b/dlls/kernel32/kernel32.spec
> @@ -696,7 +696,7 @@
>  @ stdcall GetHandleInformation(long ptr)
>  @ stub -i386 GetLSCallbackTarget
>  @ stub -i386 GetLSCallbackTemplate
> -# @ stub GetLargePageMinimum
> +@ stdcall GetLargePageMinimum()
>  @ stdcall GetLargestConsoleWindowSize(long)
>  @ stdcall GetLastError()
>  @ stub GetLinguistLangSize
> diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c
> index 61dcdc2..e9acfd3 100644
> --- a/dlls/kernel32/tests/process.c
> +++ b/dlls/kernel32/tests/process.c
> @@ -88,6 +88,7 @@ static BOOL   (WINAPI *pProcess32Next)(HANDLE, PROCESSENTRY32*);
>  static BOOL   (WINAPI *pThread32First)(HANDLE, THREADENTRY32*);
>  static BOOL   (WINAPI *pThread32Next)(HANDLE, THREADENTRY32*);
>  static BOOL   (WINAPI *pGetLogicalProcessorInformationEx)(LOGICAL_PROCESSOR_RELATIONSHIP,SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX*,DWORD*);
> +static SIZE_T (WINAPI *pGetLargePageMinimum)(void);
>
> /* ############################### */
>  static char     base[MAX_PATH];
> @@ -252,6 +253,7 @@ static BOOL init(void)
>      pThread32First = (void *)GetProcAddress(hkernel32, "Thread32First");
>      pThread32Next = (void *)GetProcAddress(hkernel32, "Thread32Next");
>      pGetLogicalProcessorInformationEx = (void *)GetProcAddress(hkernel32, "GetLogicalProcessorInformationEx");
> -
>
> pGetLargePageMinimum = (void *)GetProcAddress(hkernel32, "GetLargePageMinimum");
>
> return TRUE;
>
> }
> @@ -3138,6 +3140,19 @@ static void test_GetLogicalProcessorInformationEx(void)
>      HeapFree(GetProcessHeap(), 0, info);
>  }
>
> +static void test_largepages(void)
> +{
> - SIZE_T size;
> -
> - if (!pGetLargePageMinimum) {
> -
>    skip("No GetLargePageMinimum support.\n"); 
> -
>    return;
> - }
> - size = pGetLargePageMinimum();
> -
> - ok((size == 0) || (size == 2*1024*1024) || (size == 4*1024*1024), "GetLargePageMinimum reports %ld size\n", size);
>
> +}
> -
>
> START_TEST(process)
>  {
>      HANDLE job;
> @@ -3210,6 +3225,7 @@ START_TEST(process)
>      test_GetNumaProcessorNode();
>      test_session_info();
>      test_GetLogicalProcessorInformationEx();
> -
>
> test_largepages();
>
> /* things that can be tested:
> - lookup:         check the way program to be executed is searched

[PATCH] kernel32: implement GetLargePageMinimum

[Austin English]

On Jun 14, 2016 9:33 AM, "Sebastian Lackner"  wrote:
> On 14.06.2016 09:09, Marcus Meissner wrote:
> > From: Austin English
> >
> > https://bugs.winehq.org/show_bug.cgi?id=18745
> >
> > Report 2MB minimum page size on x86 32bit and 64bit on Linux.
> >
> > This assumes that all x86 processors and the 64/32bit emulation mode
> > have hugetable support and it is always enabled.
> >
> > Signed-off-by: Marcus Meissner 
> > Signed-off-by: Austin English
>
> As pointed out on IRC, the order of the sign-offs is wrong here. Your own

signoff
> should always be the last because you have to take responsibility when

you send a patch.
> > dlls/kernel32/cpu.c           | 15 +++++++++++++++
> >  dlls/kernel32/kernel32.spec   |  2 +-
> >  dlls/kernel32/tests/process.c | 16 ++++++++++++++++
> >  3 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/dlls/kernel32/cpu.c b/dlls/kernel32/cpu.c
> > index f48fcf0..f8b2c3d 100644
> > --- a/dlls/kernel32/cpu.c
> > +++ b/dlls/kernel32/cpu.c
> > @@ -291,3 +291,18 @@ err:
> >      }
> >      return TRUE;
> >  }
> > -

+/***********************************************************************
> > -
> > -
> >       GetLargePageMinimum (KERNEL32.@)
> > - */
> >
> > +SIZE_T WINAPI GetLargePageMinimum(void)
> > +{
> > +#ifdef linux
>
> I do not see any need for that #ifdef linux. Its more architecture

specific
> than linux specific. Dmitry already mentioned the same concern on the

mailing list.
> > +#if defined(__i386___) || defined(__x86_64__)
> > - /* Linux x86 hugepages are minimum 2 MB */
> > - return 2*1024*1024;
> >
> > +#endif
> > +#endif
> > - FIXME("stub: not implemented on your platform/architecture.\n");
> > - return 0;
> >
> > +}
> > diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
> > index ec7155b6e..0fc4dcb 100644
> > --- a/dlls/kernel32/kernel32.spec
> > +++ b/dlls/kernel32/kernel32.spec
> > @@ -696,7 +696,7 @@
> >  @ stdcall GetHandleInformation(long ptr)
> >  @ stub -i386 GetLSCallbackTarget
> >  @ stub -i386 GetLSCallbackTemplate
> > -# @ stub GetLargePageMinimum
> > +@ stdcall GetLargePageMinimum()
> >  @ stdcall GetLargestConsoleWindowSize(long)
> >  @ stdcall GetLastError()
> >  @ stub GetLinguistLangSize
> > diff --git a/dlls/kernel32/tests/process.c

b/dlls/kernel32/tests/process.c
> > index 61dcdc2..e9acfd3 100644
> > --- a/dlls/kernel32/tests/process.c
> > +++ b/dlls/kernel32/tests/process.c
> > @@ -88,6 +88,7 @@ static BOOL   (WINAPI *pProcess32Next)(HANDLE,

PROCESSENTRY32*);
> > static BOOL   (WINAPI *pThread32First)(HANDLE, THREADENTRY32*);
> >  static BOOL   (WINAPI *pThread32Next)(HANDLE, THREADENTRY32*);
> >  static BOOL   (WINAPI

*pGetLogicalProcessorInformationEx)(LOGICAL_PROCESSOR_RELATIONSHIP,SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX*,DWORD*);
> > +static SIZE_T (WINAPI *pGetLargePageMinimum)(void);
> >
> > /* ############################### */
> >  static char     base[MAX_PATH];
> > @@ -252,6 +253,7 @@ static BOOL init(void)
> >      pThread32First = (void *)GetProcAddress(hkernel32,

"Thread32First");
> >  pThread32Next = (void *)GetProcAddress(hkernel32, "Thread32Next");
> >  pGetLogicalProcessorInformationEx = (void

*)GetProcAddress(hkernel32, "GetLogicalProcessorInformationEx");
> > - pGetLargePageMinimum = (void *)GetProcAddress(hkernel32,

"GetLargePageMinimum");
> >  return TRUE;
> >
> > }
> > @@ -3138,6 +3140,19 @@ static void

test_GetLogicalProcessorInformationEx(void)
> >  HeapFree(GetProcessHeap(), 0, info);
> >
> > }
> >
> > +static void test_largepages(void)
> > +{
> > - SIZE_T size;
> > -
> > - if (!pGetLargePageMinimum) {
> > -
> >    skip("No GetLargePageMinimum support.\n");
> > -
> >    return;
> > - }
> > - size = pGetLargePageMinimum();
> > -
> > - ok((size == 0) || (size == 2*1024*1024) || (size == 4*1024*1024),

"GetLargePageMinimum reports %ld size\n", size);
> > +}
> > -
> >
> > START_TEST(process)
> >  {
> >      HANDLE job;
> > @@ -3210,6 +3225,7 @@ START_TEST(process)
> >      test_GetNumaProcessorNode();
> >      test_session_info();
> >      test_GetLogicalProcessorInformationEx();
> > -
> >
> > test_largepages();
> >
> > /* things that can be tested:
> > - lookup:         check the way program to be executed is

searched

Perhaps ARM should be listed as well.

[PATCH] kernel32: implement GetLargePageMinimum

[Marcus Meissner]

Hi,

I fixed that order, and also made a smaller patch that just adds the 
initial function without large page support.

Ciao, Marcus

On Tue, Jun 14, 2016 at 04:33:15PM +0200, Sebastian Lackner wrote:
> On 14.06.2016 09:09, Marcus Meissner wrote:
> > From: Austin English
> >
> > https://bugs.winehq.org/show_bug.cgi?id=18745
> >
> > Report 2MB minimum page size on x86 32bit and 64bit on Linux.
> >
> > This assumes that all x86 processors and the 64/32bit emulation mode
> > have hugetable support and it is always enabled.
> >
> > Signed-off-by: Marcus Meissner 
> > Signed-off-by: Austin English
>
> As pointed out on IRC, the order of the sign-offs is wrong here. Your own signoff
> should always be the last because you have to take responsibility when you send a patch.
> > dlls/kernel32/cpu.c           | 15 +++++++++++++++
> >  dlls/kernel32/kernel32.spec   |  2 +-
> >  dlls/kernel32/tests/process.c | 16 ++++++++++++++++
> >  3 files changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/dlls/kernel32/cpu.c b/dlls/kernel32/cpu.c
> > index f48fcf0..f8b2c3d 100644
> > --- a/dlls/kernel32/cpu.c
> > +++ b/dlls/kernel32/cpu.c
> > @@ -291,3 +291,18 @@ err:
> >      }
> >      return TRUE;
> >  }
> > -
> >
> > +/***********************************************************************
> > -
> > -
> >       GetLargePageMinimum (KERNEL32.@)
> > - */
> >
> > +SIZE_T WINAPI GetLargePageMinimum(void)
> > +{
> > +#ifdef linux
>
> I do not see any need for that #ifdef linux. Its more architecture specific
> than linux specific. Dmitry already mentioned the same concern on the mailing list.
> > +#if defined(__i386___) || defined(__x86_64__)
> > - /* Linux x86 hugepages are minimum 2 MB */
> > - return 2*1024*1024;
> >
> > +#endif
> > +#endif
> > - FIXME("stub: not implemented on your platform/architecture.\n");
> > - return 0;
> >
> > +}
> > diff --git a/dlls/kernel32/kernel32.spec b/dlls/kernel32/kernel32.spec
> > index ec7155b6e..0fc4dcb 100644
> > --- a/dlls/kernel32/kernel32.spec
> > +++ b/dlls/kernel32/kernel32.spec
> > @@ -696,7 +696,7 @@
> >  @ stdcall GetHandleInformation(long ptr)
> >  @ stub -i386 GetLSCallbackTarget
> >  @ stub -i386 GetLSCallbackTemplate
> > -# @ stub GetLargePageMinimum
> > +@ stdcall GetLargePageMinimum()
> >  @ stdcall GetLargestConsoleWindowSize(long)
> >  @ stdcall GetLastError()
> >  @ stub GetLinguistLangSize
> > diff --git a/dlls/kernel32/tests/process.c b/dlls/kernel32/tests/process.c
> > index 61dcdc2..e9acfd3 100644
> > --- a/dlls/kernel32/tests/process.c
> > +++ b/dlls/kernel32/tests/process.c
> > @@ -88,6 +88,7 @@ static BOOL   (WINAPI *pProcess32Next)(HANDLE, PROCESSENTRY32*);
> >  static BOOL   (WINAPI *pThread32First)(HANDLE, THREADENTRY32*);
> >  static BOOL   (WINAPI *pThread32Next)(HANDLE, THREADENTRY32*);
> >  static BOOL   (WINAPI *pGetLogicalProcessorInformationEx)(LOGICAL_PROCESSOR_RELATIONSHIP,SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX*,DWORD*);
> > +static SIZE_T (WINAPI *pGetLargePageMinimum)(void);
> >
> > /* ############################### */
> >  static char     base[MAX_PATH];
> > @@ -252,6 +253,7 @@ static BOOL init(void)
> >      pThread32First = (void *)GetProcAddress(hkernel32, "Thread32First");
> >      pThread32Next = (void *)GetProcAddress(hkernel32, "Thread32Next");
> >      pGetLogicalProcessorInformationEx = (void *)GetProcAddress(hkernel32, "GetLogicalProcessorInformationEx");
> > -
> >
> > pGetLargePageMinimum = (void *)GetProcAddress(hkernel32, "GetLargePageMinimum");
> >
> > return TRUE;
> >
> > }
> > @@ -3138,6 +3140,19 @@ static void test_GetLogicalProcessorInformationEx(void)
> >      HeapFree(GetProcessHeap(), 0, info);
> >  }
> >
> > +static void test_largepages(void)
> > +{
> > - SIZE_T size;
> > -
> > - if (!pGetLargePageMinimum) {
> > -
> >    skip("No GetLargePageMinimum support.\n"); 
> > -
> >    return;
> > - }
> > - size = pGetLargePageMinimum();
> > -
> > - ok((size == 0) || (size == 2*1024*1024) || (size == 4*1024*1024), "GetLargePageMinimum reports %ld size\n", size);
> >
> > +}
> > -
> >
> > START_TEST(process)
> >  {
> >      HANDLE job;
> > @@ -3210,6 +3225,7 @@ START_TEST(process)
> >      test_GetNumaProcessorNode();
> >      test_session_info();
> >      test_GetLogicalProcessorInformationEx();
> > -
> >
> > test_largepages();
> >
> > /* things that can be tested:
> > - lookup:         check the way program to be executed is searched

[PATCH] kernel32: implement GetLargePageMinimum

[Sebastian Lackner]

On 15.06.2016 08:18, Marcus Meissner wrote:
> Hi,
>
> I fixed that order, and also made a smaller patch that just adds the 
> initial function without large page support.
>
> Ciao, Marcus

According to the people on the bug report, a return value of zero is
not sufficient to fix this issue (see comment 8).