Zend, ext/opcache: use PR_SET_VMA_ANON_NAME (Linux 5.17) #8234
Conversation
| static zend_always_inline void zend_mmap_set_name(const void *start, size_t len, const char *name) | ||
| { | ||
| #ifdef __linux__ | ||
| prctl(PR_SET_VMA, PR_SET_VMA_ANON_NAME, (unsigned long)start, len, (unsigned long)name); |
There was a problem hiding this comment.
Is it safe to call prctl with options unsupported by the kernel?
Looks like the answer is yes and it'll just return -1 but the docs aren't very clear so I thought I'd ask.
There was a problem hiding this comment.
Is it safe to call
prctlwith options unsupported by the kernel?
yes,, it will just return -1;
There was a problem hiding this comment.
Not sure it warrants a new file for that.
There was a problem hiding this comment.
I made this a new file to reduce header dependencies, to avoid adding a dependency to sys/prctl.h to almost all sources.
The PHP code base is currently pretty clumsy about header dependencies; you have catch-all headers like zend_portability.h. I don't like that.
My coding style is to have many small and domain-specific headers with only the bare minimum of header-including-other-header. This speeds up compile times and improves code correctness.
But ... since this is not my project, I'll accept any style you desire, and will amend this PR.
There was a problem hiding this comment.
I feel if it had more it maybe it would worth it but that s personal opinion tough, other members might think differently. But what do you make about the other points I made below ?
There was a problem hiding this comment.
I would generally agree that many of our files are too big and mixing too much code of different purposes can make it hard to understand the original intention of the file. But I don't know how the other maintainers feel.
Zend/zend_fibers.c
Outdated
| @@ -211,6 +212,8 @@ static zend_fiber_stack *zend_fiber_stack_allocate(size_t size) | |||
| return NULL; | |||
| } | |||
|
|
|||
| zend_mmap_set_name(pointer, alloc_size, "fiber_stack"); | |||
There was a problem hiding this comment.
nit: I would prefix with zend_ (it takes up to 80 valid chars after all).
|
Looks good in general, little nit I would make the ID more distinct especially this feature is more and more used here and there (e.g. memory allocators). Just thinking but maybe put it behind a configure option too ? |
The new Linux 5.17 feature PR_SET_VMA_ANON_NAME can give names to anonymous private memory, see: https://lwn.net/Articles/867818/ It can be useful while debugging, to identify which portion of the process's memory belongs to which subsystem. This is how /proc/PID/maps can look like: 555ccd400000-555ccdc00000 r-xp 00000000 00:00 0 [anon:huge_code_pages] 7f6ec6600000-7f6ec6800000 rw-p 00000000 00:00 0 [anon:zend_alloc] The first mapping is the PHP executable copied to anonymous memory by option "opcache.huge_code_pages". The second one is a memory area for the "zend_alloc.h" memory allocator library. Unfortunately, it is not possible to give names to shared memory (MAP_SHARED), because Linux MAP_SHARED really maps /dev/zero (see shmem_zero_setup()), which makes madvise_vma_anon_name() believe this is a file mapping, failing the prctl() with EBADF.
I added a
Why would anybody want ot disable the feature? To save a hundred bytes or so of code size? |
|
I do not have concern about file size at all. Just to be more certain there is no negative impact (ZTS build or not) as you said it helps well to assess whether the address ranges come from php or not (and that's a great idea IMHO). While, theoretically, it should not impede really the performance it surely does not help it. |
|
There's no effect on runtime performance. |
|
Alright, thanks, another pair of eyes might be beneficial for this one. |
| static zend_always_inline void zend_mmap_set_name(const void *start, size_t len, const char *name) | ||
| { | ||
| #ifdef __linux__ | ||
| prctl(PR_SET_VMA, PR_SET_VMA_ANON_NAME, (unsigned long)start, len, (unsigned long)name); |
There was a problem hiding this comment.
I would generally agree that many of our files are too big and mixing too much code of different purposes can make it hard to understand the original intention of the file. But I don't know how the other maintainers feel.
The new Linux 5.17 feature PR_SET_VMA_ANON_NAME can give names to
anonymous private memory, see:
https://lwn.net/Articles/867818/
It can be useful while debugging, to identify which portion of the
process's memory belongs to which subsystem.
This is how /proc/PID/maps can look like:
555ccd400000-555ccdc00000 r-xp 00000000 00:00 0 [anon:huge_code_pages]
7f6ec6600000-7f6ec6800000 rw-p 00000000 00:00 0 [anon:zend_alloc]
The first mapping is the PHP executable copied to anonymous memory by
option "opcache.huge_code_pages". The second one is a memory area for
the "zend_alloc.h" memory allocator library.
Unfortunately, it is not possible to give names to shared memory
(MAP_SHARED), because Linux MAP_SHARED really maps /dev/zero (see
shmem_zero_setup()), which makes madvise_vma_anon_name() believe this
is a file mapping, failing the prctl() with EBADF.