Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the macOS 11 build #1885

Closed

Conversation

thomasdeniau
Copy link

getpagesize() was removed in POSIX.1-2001 and is therefore not available from unistd.h anymore:

/* Removed in Issue 6 */
int      getdtablesize(void) __POSIX_C_DEPRECATED(199506L);
int      getpagesize(void) __pure2 __POSIX_C_DEPRECATED(199506L);
char    *getpass(const char *) __POSIX_C_DEPRECATED(199506L);

In addition, on Macs with Apple Silicon, Apple recommends that we get the page size from
the vm_page_size variable, since page size can differ between x86_64 and arm64 apps:

https://developer.apple.com/documentation/apple_silicon/addressing_architectural_differences_in_your_macos_code

@giampaolo
Copy link
Owner

giampaolo commented Dec 11, 2020

It seems there are at least 5 possible solutions to this, but I'm unclear which one is best.

  1. getpagesize(), but as you point out it appears deprecated
  2. vm_page_size, as you did in here, but I have no idea if that's available on older macOS versions, e.g. 10.9
  3. PAGE_SIZE macro, but by Googling it seems sometimes it may be missing and possibly is not standardized
  4. sysconf(_SC_PAGESIZE): in here it says <<it's standardized in POSIX.1, and thus available in all POSIXy operating systems>>
  5. sysconf(_SC_PAGE_SIZE) which appears to be an alias?

Regarding _SC_PAGESIZE vs. _SC_PAGE_SIZE, cPython source code uses both (it does not use vm_page_size).

_SC_PAGE_SIZE: https://github.com/python/cpython/blob/5776663675b48f011d428a5874cc3c79d1deb59e/Modules/resource.c#L316-L317

_SC_PAGESIZE: https://github.com/python/cpython/blob/5776663675b48f011d428a5874cc3c79d1deb59e/Modules/mmapmodule.c#L57-L62

Another consideration: cPython's resource.c module uses getpagesize (if available) else sysconf(_SC_PAGE_SIZE), but it does so by using the pre-processor. That is something which we may not be able to do, since we're gonna distribute pre-compiled wheels for macOS. Maybe it's not a problem, but it's worth mentioning.

Also, one question: what happens with current code using getpagesize? It does not compile or it just issues a deprecation warning?

@thomasdeniau
Copy link
Author

Right now it does not build at all. At least this patch makes it build ;) But thinking of it, this is not the correct patch. It probably requires more work. The problem is that on Apple Silicon machines (the new M1 macs recently announced) arm64 processes have a 16K page size while Intel processes have a 4K page size. So we would need to fetch the page size per-process, not the page size of the process running psutil. Not sure how to do that yet.

@giampaolo
Copy link
Owner

Apparently there's another 2 methods to get the page size:

// according to SO this appears to return an incorrect result
host_page_size(mach_host_self(), &pagesize) 

...and:

vm_size_t pagesize = 0;
int mib[] = { CTL_HW, HW_PAGESIZE };
size_t length = sizeof(pagesize);
const int sysctlResult = sysctl(mib, 2, &pagesize, &length, NULL, 0);

So we would need to fetch the page size per-process, not the page size of the process running psutil. Not sure how to do that yet

Hmm.. to my understanding the page size is related to the system (globally), not to a particular process.

@thomasdeniau
Copy link
Author

So macOS 11 on the new M1 hardware is in a very weird position: the native page size is 16K, but it can also run older x86_64 apps designed for older Macs with a translator that translates the Intel instructions to ARM. For these translated processes, the page size is 4K for compatibility, because it was 4K on the old hardware. So there can be indeed processes running with 4K pages and processes running with 16K pages (I know this is very unusual). I need to look more at what this project is doing exactly, I just ran into a build error in it as part of a long dependency chain, but if it exposes the number of pages an app uses, I guess it’s always dealing with the native page size (16K on ARM)?? It would be weird for the kernel to expose details of the 4K page emulation to other processes. I will check.

@giampaolo
Copy link
Owner

I see. This sounds somewhat similar to the Windows WoW64 system (aka run/emulate 32 processes on a 64 bit Windows OS). This distinction is indeed handled already in some parts of psutil on Windows. I think there's a difference to make here though.

virtual_memory() and swap_memory() requires the page-size, but those are system-wide functions. As such, my impression is that we should probably use sysconf(_SC_PAGE_SIZE) for those (because it's supposed to be a POSIX standard).

The only per-process function requiring the page size is psutil_proc_memory_uss. That looks like a candidate for what you're talking about. If I understood your analysis and if this is supposed to work like WoW64 on Windows, I guess the fix should be something along these lines (pseudo code):

if (is_32bit_process())
    pagesize = 4096;
else
    pagesize = sysconf(_SC_PAGE_SIZE)

But I'm also not sure how to do that. FWIW, 32 bit processes appears to be discontinued though: https://support.apple.com/en-us/HT208436
Since the issue is somewhat critical I'd be OK with using sysconf(_SC_PAGE_SIZE) in psutil_proc_memory_uss for now, and just add a "XXX" comment explaining that the 32/64 bit distinction is missing but it's relevant only on older macOS versions that still support 32 bit processes.

@thomasdeniau
Copy link
Author

Those are 64 bit processes, but x86_64, not arm64. They get a 4K page size. There is API to get the architecture of another process. It’s probably OK to hardcode 4K for these given the lengths Apple went to to avoid changing the page size. But I’d like to make sure this is needed first (I don’t have a M1 Mac yet). It’s possible that these processes are told by the kernel they have a 4K page size but that the statistics about them are reported with a 16K size since that is all emulation and the actual pages are 16K. I need to check.

@thomasdeniau
Copy link
Author

Maybe we should merge that just to get the package building? We can do the correct thing later since this Intel emulation is an edge case.

@giampaolo
Copy link
Owner

Yep, but let's follow cPython's lead and use sysconf(_SC_PAGE_SIZE) instead of vm_page_size.

@thomasdeniau
Copy link
Author

I switched to sysconf and verified there's no issue with psutil_proc_memory_uss: the kernel returns native 16K page sizes for accounting even when asked about a process which uses 4K pages internally.

@giampaolo
Copy link
Owner

Thanks. While you're at it can you also remove/replace this part? (line 508 of _psutil_osx.c)

    if (host_page_size(mach_host_self(), &page_size) != KERN_SUCCESS)
        page_size = PAGE_SIZE;

This SO answer asserts host_page_size returns 4096 (incorrect).

getpagesize() was removed in POSIX.1-2001 and is therefore not available from unistd.h anymore:

```
/* Removed in Issue 6 */
int      getdtablesize(void) __POSIX_C_DEPRECATED(199506L);
int      getpagesize(void) __pure2 __POSIX_C_DEPRECATED(199506L);
char    *getpass(const char *) __POSIX_C_DEPRECATED(199506L);
```

Do like cython and use sysconf(_SC_PAGE_SIZE) instead.

I have checked that psutil_proc_memory_uss() uses the host page size (16K)
to return results even when run on Rosetta processes (which are told
they use 4K pages)

Signed-off-by: Thomas Deniau <thomas@deniau.org>
@thomasdeniau
Copy link
Author

I suspect SO is incorrect: that answer is about iOS devices. On arm64, page size can be 4K or 16K depending on how much memory the device has. Eg I believe iPhone 5S has 4K pages and iPhone 6S has 16K pages even though they are both arm64.

Also, vm_param.h has #define PAGE_SIZE vm_page_size which is the one I had used in the beginning, so it contains the page size of the current process.

We could replace it with sysconf just to get it in the same way everywhere if you prefer, but I believe both sysconf and host_page_size are valid ways to get it. One is POSIX and the other Mach...

@thomasdeniau
Copy link
Author

Oh wait I had misread that answer. Checking.

@giampaolo
Copy link
Owner

We could replace it with sysconf just to get it in the same way everywhere if you prefer but I believe both sysconf and host_page_size are valid ways to get it

+1
I see no reason to do the same thing differently.

giampaolo added a commit that referenced this pull request Dec 17, 2020
Related to #1885. getpagesize() is deprecated on macOS, POSIX.1-2001 and
possibly other UNIXes.

The problem emerged on macOS but getpagesize() is also used in FreeBSD,
NetBSD, OpenBSD and AIX, so it makes sense to do this in one place only,
similarly to Windows which also provide a psutil_getpagesize() utility.

Follow cPython's mmapmodule.c and resourcemodule.c lead and rely on
`sysconf(_SC_PAGESIZE)` instead.

Leave getpagesize() in place as last resort/attemp for systems where it's
not deprecated or they still legitimately rely on it.

Also provide a python wrapper so we can test the return value of this C
function against Python's stdlib.

Signed-off-by: Giampaolo Rodola <g.rodola@gmail.com>
giampaolo added a commit that referenced this pull request Dec 17, 2020
…size() (#1891)

Add a reusable `psutil_getpagesize()` utility common to all UNIXes.

Related to #1885 (`getpagesize()` is deprecated on recent macOS, POSIX.1-2001 and possibly other UNIXes).

The problem emerged on macOS but `getpagesize()` is also used in FreeBSD, NetBSD, OpenBSD and AIX, so it makes sense to do this in one place only, similarly to Windows which also provide a `psutil_getpagesize()` utility.

Follow cPython's `mmapmodule.c` and `resourcemodule.c` lead and rely on `sysconf(_SC_PAGESIZE)` instead, but leave `getpagesize()` in place as last resort/attempt for systems where it's not deprecated and/or they still legitimately rely on it.

Also provide a python wrapper so we can test the return value of this C function against Python's stdlib modules.

Signed-off-by: Giampaolo Rodola <g.rodola@gmail.com>
@giampaolo
Copy link
Owner

Since getpagesize() is used on other UNIX platforms I opted for a more generic solution in #1891 which covers all of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants