Обсуждение: BUG #17448: In Windows 10, version 1703 and later, huge_pages doesn't work.

The following bug has been logged on the website:

Bug reference:      17448
Logged by:          Okano Naoki
Email address:      okano.naoki@jp.fujitsu.com
PostgreSQL version: 14.2
Operating system:   Windows
Description:

With huge_pages = on, the postgres process does not appear to use large
pages.
I checked with VMMap if the large pages are used in the following
environment.
Environment
  PostgreSQL version: 14.2
  Operating system  : Windows 10 20H2

On this page (*) says that in Windows 10, version 1703 and later OS
versions,  
you must specify the FILE_MAP_LARGE_PAGES flag with the MapViewOfFile
function 
to map large pages.

I think it seems to be the cause that MapViewOfFile() in
src/backend/port/win32_shmem.c 
does not specify FILE_MAP_LARGE_PAGES flag.

(*) MapViewOfFileEx function
https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-mapviewoffileex

FILE_MAP_LARGE_PAGES
  Starting with Windows 10, version 1703, this flag specifies 
  that the view should be mapped using large page support. 
  The size of the view must be a multiple of the size of a large page 
  reported by the GetLargePageMinimum function, 
  and the file-mapping object must have been created using the
SEC_LARGE_PAGES option. 
  If you provide a non-null value for lpBaseAddress, 
  then the value must be a multiple of GetLargePageMinimum.

On Fri, Mar 25, 2022 at 07:52:57AM +0000, PG Bug reporting form wrote:
> On this page (*) says that in Windows 10, version 1703 and later OS
> versions,
> you must specify the FILE_MAP_LARGE_PAGES flag with the MapViewOfFile
> function
> to map large pages.
>
> I think it seems to be the cause that MapViewOfFile() in
> src/backend/port/win32_shmem.c
> does not specify FILE_MAP_LARGE_PAGES flag.

Hmm.  Okay.  A patch would be straight-forward, as we could just
assign the optional flag in a separate variable at the beginning of
PGSharedMemoryCreate(), similarly to flProtect when we find out that
large pages can be used, then pass it down to MapViewOfFileEx().  I
don't have a Windows 10 machine as recent as that at hand, though..

Perhaps the CI uses Windows machines that would allow to test and
check that, with some logs magically added to debug things.
--
Michael

On Sat, Mar 26, 2022 at 02:46:45PM +0900, Michael Paquier wrote:
> On Fri, Mar 25, 2022 at 07:52:57AM +0000, PG Bug reporting form wrote:
> > On this page (*) says that in Windows 10, version 1703 and later OS
> > versions,
> > you must specify the FILE_MAP_LARGE_PAGES flag with the MapViewOfFile
> > function
> > to map large pages.
> >
> > I think it seems to be the cause that MapViewOfFile() in
> > src/backend/port/win32_shmem.c
> > does not specify FILE_MAP_LARGE_PAGES flag.
>
> I don't have a Windows 10 machine as recent as that at hand, though..

I have a Windows 10 apparently version 20H2 (the versioning doesn't make any
sense) with all needed to compile postgres at hand.  I can have a look next
week.

On Sat, Mar 26, 2022 at 04:24:08PM +0800, Julien Rouhaud wrote:
> I have a Windows 10 apparently version 20H2 (the versioning doesn't make any
> sense) with all needed to compile postgres at hand.  I can have a look next
> week.

Thanks!
--
Michael

On Sat, Mar 26, 2022 at 9:24 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
> On Sat, Mar 26, 2022 at 02:46:45PM +0900, Michael Paquier wrote:
> > On Fri, Mar 25, 2022 at 07:52:57AM +0000, PG Bug reporting form wrote:
> > > On this page (*) says that in Windows 10, version 1703 and later OS
> > > versions,
> > > you must specify the FILE_MAP_LARGE_PAGES flag with the MapViewOfFile
> > > function
> > > to map large pages.
> > >
> > > I think it seems to be the cause that MapViewOfFile() in
> > > src/backend/port/win32_shmem.c
> > > does not specify FILE_MAP_LARGE_PAGES flag.
> >
> > I don't have a Windows 10 machine as recent as that at hand, though..
>
> I have a Windows 10 apparently version 20H2 (the versioning doesn't make any
> sense) with all needed to compile postgres at hand.  I can have a look next
> week.

There are traces of method to the madness:  It's basically YYMM, but
then after 2004 they switched to H1 and H2 (first/second half of the
year) instead of MM, perhaps to avoid confusion with YYYY format year.
Note also that Windows 10 has a 21H2 and Windows 11 has a 21H2.

Hmm, so all versions of Windows that our current coding worked on were
EOL'd 6 months after PostgreSQL 11 came out with huge_pages support
for Windows:

https://en.wikipedia.org/wiki/Windows_10_version_history

Some question I have: is FILE_MAP_LARGE PAGES a macro?  We claim to
support all those ancient zombie OSes like Windows 7, or maybe it's
even XP for 11, and this has to be back-patched to 11, so we might
need to make it conditional.  But conditional on what?  For example,
does something like the attached work (untested)?  What happens if a <
1703 kernel sees this flag, does it reject it or ignore it?

On Sun, Mar 27, 2022 at 12:07:57AM +1300, Thomas Munro wrote:
> Some question I have: is FILE_MAP_LARGE PAGES a macro?  We claim to
> support all those ancient zombie OSes like Windows 7, or maybe it's
> even XP for 11, and this has to be back-patched to 11, so we might
> need to make it conditional.  But conditional on what?  For example,
> does something like the attached work (untested)?  What happens if a <
> 1703 kernel sees this flag, does it reject it or ignore it?

Good question.  I would choose a soft approach here and not insist if
the flag was not known at compilation time, but we could also take a
more aggressive approach and hardcode a value.  Anyway, it seems to me
that the correct solution here would be to compile the code with a
PG_FILE_MAP_LARGE_PAGES that checks if the flag exists at compile
time, and we would set it at run time if we know that we are on a
version of Windows that supports it.  IsWindowsVersionOrGreater()
should be able to do the job.
--
Michael

On Sun, Mar 27, 2022 at 12:07:57AM +1300, Thomas Munro wrote:
> There are traces of method to the madness:  It's basically YYMM, but
> then after 2004 they switched to H1 and H2 (first/second half of the
> year) instead of MM, perhaps to avoid confusion with YYYY format year.
> Note also that Windows 10 has a 21H2 and Windows 11 has a 21H2.
>
> Some question I have: is FILE_MAP_LARGE PAGES a macro?  We claim to
> support all those ancient zombie OSes like Windows 7, or maybe it's
> even XP for 11, and this has to be back-patched to 11, so we might
> need to make it conditional.  But conditional on what?  For example,
> does something like the attached work (untested)?  What happens if a <
> 1703 kernel sees this flag, does it reject it or ignore it?

I don't have an answer about how much Windows gets angry if we pass
down to MapViewOfFileEx() the flag FILE_MAP_LARGE_PAGES when running
the code on a version of Windows that does not support it.

Anyway, I think that we could just play it safe.  See for example the
attached, where I use PG_FILE_MAP_LARGE_PAGES at compile time to find
if the value is set.  Then, at run-time, I am just relying on
IsWindowsVersionOrGreater() to do the job, something useful when
huge_pages=on as I guess we should fail hard if we did not know about
FILE_MAP_LARGE_PAGES at compile-time, but try to use huge pages at run
time with version >= 10.0.1703.

Perhaps there is a better thing to do?
--
Michael

Hi,

On Wed, Mar 30, 2022 at 04:54:50PM +0900, Michael Paquier wrote:
> On Sun, Mar 27, 2022 at 12:07:57AM +1300, Thomas Munro wrote:
> > There are traces of method to the madness:  It's basically YYMM, but
> > then after 2004 they switched to H1 and H2 (first/second half of the
> > year) instead of MM, perhaps to avoid confusion with YYYY format year.
> > Note also that Windows 10 has a 21H2 and Windows 11 has a 21H2.
> >
> > Some question I have: is FILE_MAP_LARGE PAGES a macro?  We claim to
> > support all those ancient zombie OSes like Windows 7, or maybe it's
> > even XP for 11, and this has to be back-patched to 11, so we might
> > need to make it conditional.  But conditional on what?  For example,
> > does something like the attached work (untested)?  What happens if a <
> > 1703 kernel sees this flag, does it reject it or ignore it?
>
> I don't have an answer about how much Windows gets angry if we pass
> down to MapViewOfFileEx() the flag FILE_MAP_LARGE_PAGES when running
> the code on a version of Windows that does not support it.

No idea either, and I don't have old enough Windows machine available to try.

> Anyway, I think that we could just play it safe.  See for example the
> attached, where I use PG_FILE_MAP_LARGE_PAGES at compile time to find
> if the value is set.  Then, at run-time, I am just relying on
> IsWindowsVersionOrGreater() to do the job, something useful when
> huge_pages=on as I guess we should fail hard if we did not know about
> FILE_MAP_LARGE_PAGES at compile-time, but try to use huge pages at run
> time with version >= 10.0.1703.

That approach seems sensible.  For reference the versionhelpers.h seems to be
available starting with VS 2013 / v120, which is ok since that the oldest
version we support AFAICS.

After *a lot of time* I could finally test this patch.  For the record I could
never find a way to allow 'Lock pages in memory' on the Windows 10 home I have,
so I tried on my Windows 11 evaluation I also had around (version 21H2, so it
should be recent enough).  For the record on this one there was gpedit
available, but then I got a 1450 error, and didn't find any information on how
to reserve huge pages or something like that on Windows.  So I just configured
shared_buffers to 10MB, which should still be big enough to need multiple huge
pages, and it seems to work:

postgres=# select version();
                            version
---------------------------------------------------------------
 PostgreSQL 15devel, compiled by Visual C++ build 1929, 64-bit
(1 row)

postgres=# show huge_pages;
 huge_pages
------------
 on
(1 row)

Now, I also have the exact same result without the patch applied so it's hard
to know whether it had any impact at all.  Unfortunately, I didn't find any
information on how to check if "large pages" are used and/or by which program.

On Thu, Mar 31, 2022 at 12:59:08PM +0800, Julien Rouhaud wrote:
> That approach seems sensible.  For reference the versionhelpers.h seems to be
> available starting with VS 2013 / v120, which is ok since that the oldest
> version we support AFAICS.

Hmm.  This points out to a different problem.  The oldest version of
MSVC supported on v10 and v11 is VS2005, so a backpatch means that
those checks would need to be tweaked if we'd want to be perfectly
compatible.  But I'd rather not enter in this game category, limiting
this patch to v12~ :)

> Now, I also have the exact same result without the patch applied so it's hard
> to know whether it had any impact at all.  Unfortunately, I didn't find any
> information on how to check if "large pages" are used and/or by which program.

Okano-san has mentioned VMMap upthread:
https://docs.microsoft.com/en-us/sysinternals/downloads/vmmap
--
Michael

On Thu, Mar 31, 2022 at 04:42:37PM +0900, Michael Paquier wrote:
> On Thu, Mar 31, 2022 at 12:59:08PM +0800, Julien Rouhaud wrote:
> > That approach seems sensible.  For reference the versionhelpers.h seems to be
> > available starting with VS 2013 / v120, which is ok since that the oldest
> > version we support AFAICS.
> 
> Hmm.  This points out to a different problem.  The oldest version of
> MSVC supported on v10 and v11 is VS2005, so a backpatch means that
> those checks would need to be tweaked if we'd want to be perfectly
> compatible.  But I'd rather not enter in this game category, limiting
> this patch to v12~ :)

Ah, I indeed haven't check in back branches.  v12 isn't that bad.

> > Now, I also have the exact same result without the patch applied so it's hard
> > to know whether it had any impact at all.  Unfortunately, I didn't find any
> > information on how to check if "large pages" are used and/or by which program.
> 
> Okano-san has mentioned VMMap upthread:
> https://docs.microsoft.com/en-us/sysinternals/downloads/vmmap

Yes, I totally missed that.  Thomas Munro also mentioned it off-list, and
also found some reference [1] indicating that large pages should show up as
"Locked WS".  I tested with and without the patch and in both case I don't see
any "Locked WS" usage.  I also get the same Page Table size, which seems
consistent with large pages not being used.  Now, this is a vm running with
virtualbox and we're not entirely sure that huge pages can be allocated with
it.  I wish I could test on my windows 10 machine as it's not virtualized, but
I can't give the required privileges.

[1] https://aloiskraus.wordpress.com/2016/10/03/windows-10-memory-compression-and-more/

On Thu, Mar 31, 2022 at 04:00:55PM +0800, Julien Rouhaud wrote:
> > Okano-san has mentioned VMMap upthread:
> > https://docs.microsoft.com/en-us/sysinternals/downloads/vmmap
> 
> Yes, I totally missed that.  Thomas Munro also mentioned it off-list, and
> also found some reference [1] indicating that large pages should show up as
> "Locked WS".  I tested with and without the patch and in both case I don't see
> any "Locked WS" usage.  I also get the same Page Table size, which seems
> consistent with large pages not being used.  Now, this is a vm running with
> virtualbox and we're not entirely sure that huge pages can be allocated with
> it.  I wish I could test on my windows 10 machine as it's not virtualized, but
> I can't give the required privileges.
> 
> [1] https://aloiskraus.wordpress.com/2016/10/03/windows-10-memory-compression-and-more/

So, after more digging it turns out that the patch is supposed to work.  If I
force using the PG_FILE_MAP_LARGE_PAGES, postgres starts and I do see "Locked
WS" usage with VMMap, with a size in the order of magnitude of my
shared_buffers.

What is apparently not working on my VM is IsWindowsVersionOrGreater(10, 0,
1703).  I added some debug around to check what GetVersionEx() [2] is saying,
and I get:

dwMajorVersion == 6
dwMinorVersion == 2
dwBuildNumber == 9200

While winver.exe on the same vm says windows 11, version 21H2, build 22000.493.

I'm therefore extremely confused.  The documentation of
IsWindowsVersionOrGreater() at [3] is also highly confusing:

> TRUE if the specified version matches, or is greater than, the version of the
> current Windows OS; otherwise, FALSE.

Isn't that supposed to be the opposite?

[2] https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-getversionexa
https://docs.microsoft.com/en-us/windows/win32/api/winnt/ns-winnt-osversioninfoexa

[3] https://docs.microsoft.com/en-us/windows/win32/api/versionhelpers/nf-versionhelpers-iswindowsversionorgreater

On Thu, Mar 31, 2022 at 06:46:59PM +0800, Julien Rouhaud wrote:
> On Thu, Mar 31, 2022 at 04:00:55PM +0800, Julien Rouhaud wrote:
> > > Okano-san has mentioned VMMap upthread:
> > > https://docs.microsoft.com/en-us/sysinternals/downloads/vmmap
> > 
> > Yes, I totally missed that.  Thomas Munro also mentioned it off-list, and
> > also found some reference [1] indicating that large pages should show up as
> > "Locked WS".  I tested with and without the patch and in both case I don't see
> > any "Locked WS" usage.  I also get the same Page Table size, which seems
> > consistent with large pages not being used.  Now, this is a vm running with
> > virtualbox and we're not entirely sure that huge pages can be allocated with
> > it.  I wish I could test on my windows 10 machine as it's not virtualized, but
> > I can't give the required privileges.
> > 
> > [1] https://aloiskraus.wordpress.com/2016/10/03/windows-10-memory-compression-and-more/
> 
> So, after more digging it turns out that the patch is supposed to work.  If I
> force using the PG_FILE_MAP_LARGE_PAGES, postgres starts and I do see "Locked
> WS" usage with VMMap, with a size in the order of magnitude of my
> shared_buffers.
> 
> What is apparently not working on my VM is IsWindowsVersionOrGreater(10, 0,
> 1703).  I added some debug around to check what GetVersionEx() [2] is saying,
> and I get:
> 
> dwMajorVersion == 6
> dwMinorVersion == 2
> dwBuildNumber == 9200
> 
> While winver.exe on the same vm says windows 11, version 21H2, build 22000.493.

So, what GetVersionEx returns is actually "it depends", and this is documented:

> With the release of Windows 8.1, the behavior of the GetVersionEx API has
> changed in the value it will return for the operating system version. The
> value returned by the GetVersionEx function now depends on how the
> application is manifested.
>
> Applications not manifested for Windows 8.1 or Windows 10 will return the
> Windows 8 OS version value (6.2). Once an application is manifested for a
> given operating system version, GetVersionEx will always return the version
> that the application is manifested for in future releases. To manifest your
> applications for Windows 8.1 or Windows 10, refer to Targeting your
> application for Windows.

There's no such indication on IsWindowsVersionOrGreater(), but after seeing
various comments on forums from angry people, it may be a hint that it behaves
similarly.  I'm not sure what to do at this point, maybe just always use the
flag (the PG_ version which may be 0), hoping that hopefully windows won't
define it if it can't handle it?

On Thu, Mar 31, 2022 at 06:46:59PM +0800, Julien Rouhaud wrote:
> So, after more digging it turns out that the patch is supposed to work.  If I
> force using the PG_FILE_MAP_LARGE_PAGES, postgres starts and I do see "Locked
> WS" usage with VMMap, with a size in the order of magnitude of my
> shared_buffers.
>
> What is apparently not working on my VM is IsWindowsVersionOrGreater(10, 0,
> 1703).  I added some debug around to check what GetVersionEx() [2] is saying,
> and I get:
>
> dwMajorVersion == 6
> dwMinorVersion == 2
> dwBuildNumber == 9200

Okay.  Well, I'd like to think that the patch written as-is is
correct.  Now your tests are saying the contrary, so I don't really
know what to think about it :)

>> TRUE if the specified version matches, or is greater than, the version of the
>> current Windows OS; otherwise, FALSE.
>
> Isn't that supposed to be the opposite?

I get from the upstream docs that if the runtime version of Windows is
higher than 10.0.1703, IsWindowsVersionOrGreater() should return
true.  Perhaps the issue is in the patch and its argument values, but
it does not look straight-forward to know what those values should
be, and there are no examples in the docs to show that, either :/
--
Michael

On Thu, Mar 31, 2022 at 07:03:09PM +0800, Julien Rouhaud wrote:
> There's no such indication on IsWindowsVersionOrGreater(), but after seeing
> various comments on forums from angry people, it may be a hint that it behaves
> similarly.  I'm not sure what to do at this point, maybe just always use the
> flag (the PG_ version which may be 0), hoping that hopefully windows won't
> define it if it can't handle it?

Looking at the internals of versionhelpers.h, would it work to use as
arguments for IsWindowsVersionOrGreater() the following, in this
order?  As of:
- HIBYTE(_WIN32_WINNT_WINTHRESHOLD)
- LOBYTE(_WIN32_WINNT_WINTHRESHOLD)
- 1703

Just to drop an idea.
--
Michael

On Fri, Apr 01, 2022 at 02:59:22PM +0900, Michael Paquier wrote:
> On Thu, Mar 31, 2022 at 07:03:09PM +0800, Julien Rouhaud wrote:
> > There's no such indication on IsWindowsVersionOrGreater(), but after seeing
> > various comments on forums from angry people, it may be a hint that it behaves
> > similarly.  I'm not sure what to do at this point, maybe just always use the
> > flag (the PG_ version which may be 0), hoping that hopefully windows won't
> > define it if it can't handle it?
>
> Looking at the internals of versionhelpers.h, would it work to use as
> arguments for IsWindowsVersionOrGreater() the following, in this
> order?  As of:
> - HIBYTE(_WIN32_WINNT_WINTHRESHOLD)
> - LOBYTE(_WIN32_WINNT_WINTHRESHOLD)
> - 1703
>
> Just to drop an idea.

I will test that in a bit.  I still think that at least some API exists to give
the real answer since winver and similar report correct info, I just don't know
what those are.

Note that if you want to test yourself you could use this script [1] using the
evaluation virtual machine [2] to automatically setup a windows 11 environment
with everything needed to compile postgres with a extra dependencies.  The
whole process is a bit long though, so I can also give you access to my vm if
you prefer, probably the latency shouldn't be too bad :)

[1] https://github.com/rjuju/pg_msvc_generator/blob/master/bootstrap.ps1
[2] https://developer.microsoft.com/en-us/windows/downloads/virtual-machines/

On Sun, Mar 27, 2022 at 12:07:57AM +1300, Thomas Munro wrote:
> Some question I have: is FILE_MAP_LARGE PAGES a macro?  We claim to
> support all those ancient zombie OSes like Windows 7, or maybe it's
> even XP for 11, and this has to be back-patched to 11, so we might
> need to make it conditional.  But conditional on what?  For example,
> does something like the attached work (untested)?  What happens if a <
> 1703 kernel sees this flag, does it reject it or ignore it?

I have been looking at this thread, and found an answer in an example
of application creating a map object with large pages in [1]:
"This flag is ignored on OS versions before Windows 10, version 1703."

So based on that I think that we could just apply and backpatch what
you have here.  This issue is much easier to reason about on HEAD
where we just care about Win >= 10, and we've be rather careful with
changes like that when it came to Windows.  Any objections about doing
a backpatch?  I'd like to do so after an extra lookup, if there are no
objections.  Or would folks prefer a HEAD-only fix for now?

[1]:
https://docs.microsoft.com/en-us/windows/win32/memory/creating-a-file-mapping-using-large-pages?source=recommendations
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> So based on that I think that we could just apply and backpatch what
> you have here.  This issue is much easier to reason about on HEAD
> where we just care about Win >= 10, and we've be rather careful with
> changes like that when it came to Windows.  Any objections about doing 
> a backpatch?  I'd like to do so after an extra lookup, if there are no
> objections.  Or would folks prefer a HEAD-only fix for now?

Let's just fix it in HEAD.  I think the risk/reward ratio isn't very
good here.

(I'd be particularly against changing this in v10, because 10.23 will
be the last one; there will be no second chance if we ship it broken.)

            regards, tom lane

On Fri, Sep 16, 2022 at 10:29:38AM -0400, Tom Lane wrote:
> Michael Paquier <michael@paquier.xyz> writes:
> > So based on that I think that we could just apply and backpatch what
> > you have here.  This issue is much easier to reason about on HEAD
> > where we just care about Win >= 10, and we've be rather careful with
> > changes like that when it came to Windows.  Any objections about doing
> > a backpatch?  I'd like to do so after an extra lookup, if there are no
> > objections.  Or would folks prefer a HEAD-only fix for now?
>
> Let's just fix it in HEAD.  I think the risk/reward ratio isn't very
> good here.
>
> (I'd be particularly against changing this in v10, because 10.23 will
> be the last one; there will be no second chance if we ship it broken.)

+1

On Fri, Sep 16, 2022 at 10:36:12PM +0800, Julien Rouhaud wrote:
> On Fri, Sep 16, 2022 at 10:29:38AM -0400, Tom Lane wrote:
>> Let's just fix it in HEAD.  I think the risk/reward ratio isn't very
>> good here.
>>
>> (I'd be particularly against changing this in v10, because 10.23 will
>> be the last one; there will be no second chance if we ship it broken.)
>
> +1

Okay, fine by me.  I have applied that only on HEAD, then.
--
Michael