Re: Supporting huge pages on Windows

On 2017-04-03 04:56:45 +0000, Tsunakawa, Takayuki wrote:
> +/*
> + * EnableLockPagesPrivilege
> + *
> + * Try to acquire SeLockMemoryPrivilege so we can use large pages.
> + */
> +static bool
> +EnableLockPagesPrivilege(int elevel)
> +{
> + HANDLE hToken;
> + TOKEN_PRIVILEGES tp;
> + LUID luid;
> +
> + if (!OpenProcessToken(GetCurrentProcess(), TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY, &hToken))
> + {
> + ereport(elevel,
> + (errmsg("could not enable Lock Pages in Memory user right: error code %lu", GetLastError()),
> + errdetail("Failed system call was %s.", "OpenProcessToken")));
> + return FALSE;
> + }

I don't think the errdetail is quite right - OpenProcessToken isn't
really a syscall, is it? But then it's a common pattern already in
wind32_shmem.c...

> + if (!LookupPrivilegeValue(NULL, SE_LOCK_MEMORY_NAME, &luid))
> + {
> + ereport(elevel,
> + (errmsg("could not enable Lock Pages in Memory user right: error code %lu", GetLastError()),
> + errdetail("Failed system call was %s.", "LookupPrivilegeValue")));

Other places in the file actually log the arguments to the function...

Wonder if we should quote "Lock Pages in Memory" or add dashes, to make
sure it's clear that that's the right?

> + if (huge_pages == HUGE_PAGES_ON || huge_pages == HUGE_PAGES_TRY)
> + {
> + /* Does the processor support large pages? */
> + largePageSize = GetLargePageMinimum();
> + if (largePageSize == 0)
> + {
> + ereport(huge_pages == HUGE_PAGES_ON ? FATAL : DEBUG1,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("the processor does not support large pages")));
> + ereport(DEBUG1,
> + (errmsg("disabling huge pages")));
> + }
> + else if (!EnableLockPagesPrivilege(huge_pages == HUGE_PAGES_ON ? FATAL : DEBUG1))
> + {
> + ereport(DEBUG1,
> + (errmsg("disabling huge pages")));
> + }
> + else
> + {
> + /* Huge pages available and privilege enabled, so turn on */
> + flProtect = PAGE_READWRITE | SEC_COMMIT | SEC_LARGE_PAGES;

Why don't we just add the relevant flag, instead of overwriting the
previous contents?

> diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
> index c63819b..2358ed0 100644
> --- a/src/bin/pg_ctl/pg_ctl.c
> +++ b/src/bin/pg_ctl/pg_ctl.c
> @@ -1708,11 +1708,6 @@ typedef BOOL (WINAPI * __SetInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, L
> typedef BOOL (WINAPI * __AssignProcessToJobObject) (HANDLE, HANDLE);
> typedef BOOL (WINAPI * __QueryInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS, LPVOID, DWORD, LPDWORD);
>
> -/* Windows API define missing from some versions of MingW headers */
> -#ifndef DISABLE_MAX_PRIVILEGE
> -#define DISABLE_MAX_PRIVILEGE 0x1
> -#endif
> -

> /*
> * Create a restricted token, a job object sandbox, and execute the specified
> * process with it.
> @@ -1794,7 +1789,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
> }
>
> b = _CreateRestrictedToken(origToken,
> - DISABLE_MAX_PRIVILEGE,
> + 0,
> sizeof(dropSids) / sizeof(dropSids[0]),
> dropSids,
> 0, NULL,

Uh - isn't that a behavioural change? Was this discussed?

Greetings,

Andres Freund