MDEV-25424 my_multi_malloc large to use my_large_malloc #1958
Conversation
Separate out my_psi_key_init / my_psi_key_free functions so large allocations can use the same memory instrumentation implementation.
Change my_multi_malloc_large to use my_large_malloc. The my_multi_malloc_large interface now takes a final argument of a pointer to the size actually allocated. This is needed because my_large_free(munmap) requires the size. Use size_t as its the native type for sizes of memory. Return allocated amount with my_large_free, however first adjusted with my_psi_key_free to ensure the right pointer address is used. Closes: MariaDB#1513
|
|
||
| @return A pointer to the user memory | ||
| */ | ||
| void *my_psi_key_init(PSI_memory_key key, void *ptr, size_t size, myf my_flags) |
There was a problem hiding this comment.
The new function names and function comments need to be more descriptive as
the functions does more than just initialize and free PSI keys.
| @param ptr Pointer to user memory returned by my_psi_key_init. | ||
| @returns ptr to start of memory allocated. | ||
| */ | ||
| void *my_psi_key_free(void *ptr) |
There was a problem hiding this comment.
Return value of my_psi_key_free is not used in the code.
It may be used in new code, but it would be better if the new code
would be part of this commit to show where and how the return value is
used.
| DBUG_ENTER("my_free"); | ||
| DBUG_PRINT("my",("ptr: %p", ptr)); | ||
|
|
||
| if (ptr == NULL) | ||
| DBUG_VOID_RETURN; | ||
|
|
||
| mh= USER_TO_HEADER(ptr); | ||
| #ifndef SAFEMALLOC |
There was a problem hiding this comment.
I prefer the old code where we have one #ifndef SAFEMALLOC just around TRASH_FREE()
Now you duplicate call to my_psi_key_free() which makes the code harder to read.
I don't see why the old approach would not work here. You may have to declare 'old_size' as
attribute((unused)), but that is a small price to pay
| @@ -90,11 +93,15 @@ void *my_multi_malloc_large(PSI_memory_key key, myf myFlags, ...) | |||
| length=va_arg(args,ulonglong); | |||
| tot_length+=ALIGN_SIZE(length); | |||
| } | |||
| ret_total_length= va_arg(args, size_t *); | |||
There was a problem hiding this comment.
Add return length as an explicit argument to the function and not a var_arg parameter.
This allows the compiler to check the type of variable and also that one has not forgot to add it.
| DBUG_RETURN(0); /* purecov: inspected */ | ||
|
|
||
| start= my_psi_key_init(key, start, tot_length, myFlags); |
There was a problem hiding this comment.
Wouldn't it be better to have my_large_malloc() call my_psi_key_init()?
This would benefit all callers of my_large_malloc() !
| @@ -1201,9 +1202,11 @@ void end_pagecache(PAGECACHE *pagecache, my_bool cleanup) | |||
|
|
|||
| if (pagecache->block_mem) | |||
| { | |||
| my_large_free(pagecache->block_mem, pagecache->mem_size); | |||
There was a problem hiding this comment.
This change is not needed (no reason to split a short row into two)
grooverdan
added
the
need feedback
Description
As the default Key cache and page cache are > 2MB, it makes sense to use a large memory pages when these are allocated. As these are rather fixed RAM areas keeping the minimal number of TLB entries is preferred.
The current my_multi_malloc_large doesn't actually do large allocations currently.
How can this PR be tested?
Boot with kernel arguments
hugepagesz=2M hugepages=256or run with the Windows permissionLockPagesInMemory.mysql-test/mtr --mysqld=--large-pages --suite=maria,main --big-test --force
(force to ignore the default values of large pages test in main).
Basing the PR against the correct MariaDB version
Backward compatibility
A user who previous has large-pages enable, will get more large pages used. Not a big problem as large page allocations fall back to normal memoy allocation if insufficient memory.